Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for 10/100Mbps

[Vladimir Oltean]

Hi Ismail,

On Fri, Apr 23, 2021 at 10:03:58PM +0000, Ismail, Mohammad Athari wrote:
> Hi Vladimir,
> 
> > -----Original Message-----
> > From: Vladimir Oltean <olteanv@xxxxxxxxx>
> > Sent: Saturday, April 24, 2021 2:12 AM
> > To: Ismail, Mohammad Athari <mohammad.athari.ismail@xxxxxxxxx>
> > Cc: Alexandre Torgue <alexandre.torgue@xxxxxx>; Jose Abreu
> > <joabreu@xxxxxxxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>; Jakub
> > Kicinski <kuba@xxxxxxxxxx>; Andrew Lunn <andrew@xxxxxxx>; Heiner Kallweit
> > <hkallweit1@xxxxxxxxx>; Russell King <linux@xxxxxxxxxxxxxxx>; Ong, Boon
> > Leong <boon.leong.ong@xxxxxxxxx>; Voon, Weifeng
> > <weifeng.voon@xxxxxxxxx>; Wong, Vee Khee <vee.khee.wong@xxxxxxxxx>;
> > netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet for
> > 10/100Mbps
> > 
> > On Fri, Apr 23, 2021 at 09:30:07AM +0000, Ismail, Mohammad Athari wrote:
> > > Hi Vladimir,
> > >
> > > > -----Original Message-----
> > > > From: Vladimir Oltean <olteanv@xxxxxxxxx>
> > > > Sent: Friday, April 23, 2021 8:53 AM
> > > > To: Ismail, Mohammad Athari <mohammad.athari.ismail@xxxxxxxxx>
> > > > Cc: Alexandre Torgue <alexandre.torgue@xxxxxx>; Jose Abreu
> > > > <joabreu@xxxxxxxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>;
> > > > Jakub Kicinski <kuba@xxxxxxxxxx>; Andrew Lunn <andrew@xxxxxxx>;
> > > > Heiner Kallweit <hkallweit1@xxxxxxxxx>; Russell King
> > > > <linux@xxxxxxxxxxxxxxx>; Ong, Boon Leong <boon.leong.ong@xxxxxxxxx>;
> > > > Voon, Weifeng <weifeng.voon@xxxxxxxxx>; Wong, Vee Khee
> > > > <vee.khee.wong@xxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> > > > linux-kernel@xxxxxxxxxxxxxxx
> > > > Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption packet
> > > > for 10/100Mbps
> > > >
> > > > On Fri, Apr 23, 2021 at 12:45:25AM +0000, Ismail, Mohammad Athari wrote:
> > > > > Hi Vladimir,
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Vladimir Oltean <olteanv@xxxxxxxxx>
> > > > > > Sent: Friday, April 23, 2021 7:53 AM
> > > > > > To: Ismail, Mohammad Athari <mohammad.athari.ismail@xxxxxxxxx>
> > > > > > Cc: Alexandre Torgue <alexandre.torgue@xxxxxx>; Jose Abreu
> > > > > > <joabreu@xxxxxxxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>;
> > > > > > Jakub Kicinski <kuba@xxxxxxxxxx>; Andrew Lunn <andrew@xxxxxxx>;
> > > > > > Heiner Kallweit <hkallweit1@xxxxxxxxx>; Russell King
> > > > > > <linux@xxxxxxxxxxxxxxx>; Ong, Boon Leong
> > > > > > <boon.leong.ong@xxxxxxxxx>; Voon, Weifeng
> > > > > > <weifeng.voon@xxxxxxxxx>; Wong, Vee Khee
> > > > > > <vee.khee.wong@xxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> > > > > > linux-kernel@xxxxxxxxxxxxxxx
> > > > > > Subject: Re: [PATCH v2 net-next] net: pcs: Enable pre-emption
> > > > > > packet for 10/100Mbps
> > > > > >
> > > > > > Hi Mohammad,
> > > > > >
> > > > > > On Fri, Apr 23, 2021 at 07:06:45AM +0800,
> > > > > > mohammad.athari.ismail@xxxxxxxxx
> > > > > > wrote:
> > > > > > > From: Mohammad Athari Bin Ismail
> > > > > > > <mohammad.athari.ismail@xxxxxxxxx>
> > > > > > >
> > > > > > > Set VR_MII_DIG_CTRL1 bit-6(PRE_EMP) to enable pre-emption
> > > > > > > packet for 10/100Mbps by default. This setting doesn`t impact
> > > > > > > pre-emption capability for other speeds.
> > > > > > >
> > > > > > > Signed-off-by: Mohammad Athari Bin Ismail
> > > > > > > <mohammad.athari.ismail@xxxxxxxxx>
> > > > > > > ---
> > > > > >
> > > > > > What is a "pre-emption packet"?
> > > > >
> > > > > In IEEE 802.1 Qbu (Frame Preemption), pre-emption packet is used
> > > > > to differentiate between MAC Frame packet, Express Packet,
> > > > > Non-fragmented Normal Frame Packet, First Fragment of Preemptable
> > > > > Packet, Intermediate Fragment of Preemptable Packet and Last
> > > > > Fragment of Preemptable Packet.
> > > >
> > > > Citation needed, which clause are you referring to?
> > >
> > > Cited from IEEE802.3-2018 Clause 99.3.
> > 
> > Aha, you know that what you just said is not what's in the "MAC Merge sublayer"
> > clause, right? There is no such thing as "pre-emption packet"
> > in the standard, this is a made-up name, maybe preemptable packets, but the
> > definition of preemptable packets is not that, hence my question.
> > 
> 
> Thank you for the knowledge sharing. My guess, this "pre-emption
> packet" might be referring to "preamble" byte in Ethernet frame. 
> 
> > > >
> > > > >
> > > > > This bit "VR_MII_DIG_CTRL1 bit-6(PRE_EMP)" defined in DesignWare
> > > > > Cores Ethernet PCS Databook is to allow the IP to properly
> > > > > receive/transmit pre-emption packets in SGMII 10M/100M Modes.
> > > >
> > > > Shouldn't everything be handled at the MAC merge sublayer? What
> > > > business does the PCS have in frame preemption?
> > >
> > > There is no further detail explained in the databook w.r.t to
> > > VR_MII_DIG_CTRL1 bit-6(PRE_EMP). The only statement it mentions is
> > > "This bit should be set to 1 to allow the DWC_xpcs to properly
> > > receive/transmit pre-emption packets in SGMII 10M/100M Modes".
> > 
> > Correct, I see this too. I asked our hardware design team, and at least on NXP
> > LS1028A (no Synopsys PCS), the PCS layer has nothing to do with frame
> > preemption, as mentioned.
> > 
> > But indeed, I do see this obscure bit in the Digital Control 1 register too, I've no
> > idea what it does. I'll ask around. Odd anyway. If you have to set it, you have to
> > set it, I guess. But it is interesting to see why is it even a configurable bit, why it
> > is not enabled by default, what is the drawback of enabling it?!
> 
> The databook states that the default value is 0. We don`t see any
> drawback of enabling it. As the databook mentions that, enabling the
> bit will allow SGMII 10/100M to receive/transmit preamble properly, so
> I think it is recommended to enable it for IP that support SGMII
> 10/100M speed.

Why do you need this patch, exactly? Is there anything that doesn't work
if you don't make the change? For example, if you leave the PRE_EMP bit
in the PCS set to zero, you set the link to 100 Mbps, configure all
queues to go to the pMAC and stress the interface with some iperf3
traffic for a while, do you see any issues at all?