RE: [PATCH V2 net-next] net: fec: add CBS offload support

[Wei Fang]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: 2023年2月13日 21:32
> To: Wei Fang <wei.fang@xxxxxxx>
> Cc: Shenwei Wang <shenwei.wang@xxxxxxx>; Clark Wang
> <xiaoning.wang@xxxxxxx>; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
> simon.horman@xxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V2 net-next] net: fec: add CBS offload support
> 
> > 3. According to Andrew's comments, the speed may be equal to 0 when the
> >    link is not up, so added a check to see if speed is equal to 0. In
> >    addtion, the change in link speed also need to be taken into account.
> >    Considering that the change of link speed has invalidated the original
> >    configuration, so we just fall back to the default setting.
> 
> I don't think that is what you have actually implemented. A link status change
> causes a fec_restart. And in fac_restart, you now reprogram the hardware. So if
> the link speed is sufficient to support the request, the hardware should be
> setup to support it.
> 
I also considered that as long as the current link speed meets the idleslope, the
hardware should be set to support it. But I noticed that the description in IEEE
802.Q-2018 Section 8.6.8.2 item:
	sendSlope = (idleSlope ? portTransmitRate)
Therefore, I think that the above formula will not hold true if the link speed changes.
Based on this consideration, my current implementation is restore to the default
setting if the speed changes. For other cases, such as link down/up, transmit timeout,
and so on, the hardware will be reconfigured to support it.
Of course, if you insist that if the link speed is sufficient to support the request , the
hardware should be setup to support it, I will continue to improve this patch.

> What are the real uses cases here? VoIP? Video streaming?

Yes, this feature is usually used for Audio and Video streams.

>So 128kbps,
> 2Mbps. Both of those are fine over a 10Half limk. So i think you should try to
> configure the hardware whenever possible, after link change or any other
> condition which causes a reset of the hardware.
> 
This is what the current patch implements except for link speed changes.

> What i have not seen addresses here is my comment/question about what tc
> shows when it is not possible to perform the request after a link change? Did
> you look at how other drivers handle this? Maybe you need to ask Jamal?

For most drivers, the settings for hardware CBS do not change as the link speed
changes. I looked the stmmac and enetc drivers, they just kept the registers'
setting when the link speed had been changed.
About the tc show command, do you mean the "tc qdisc show dev devname"
command? If so, I think this command just get the previously saved data from
the user space. So it is not possible to indicate in the tc show command that
the configuration is no longer possible. Unless there is an interface provided
for the driver to get the current CBS configuration.

> 
> > +static int fec_enet_setup_tc_cbs(struct net_device *ndev, void
> > +*type_data) {
> > +	struct fec_enet_private *fep = netdev_priv(ndev);
> > +	struct tc_cbs_qopt_offload *cbs = type_data;
> > +	int queue = cbs->queue;
> > +	int speed = fep->speed;
> > +	int queue2;
> > +
> > +	if (!(fep->quirks & FEC_QUIRK_HAS_AVB))
> > +		return -EOPNOTSUPP;
> > +
> > +	/* Queue 1 for Class A, Queue 2 for Class B, so the ENET must
> > +	 * have three queues.
> > +	 */
> > +	if (fep->num_tx_queues != FEC_ENET_MAX_TX_QS)
> > +		return -EOPNOTSUPP;
> > +
> > +	if (!speed) {
> > +		netdev_err(ndev, "Link speed is 0!\n");
> > +		return -ECANCELED;
> 
> ECANCLED? First time i've seen that one used. I had to go look it up to see
> what it means. It does not really give the user any idea why it failed. How
> about -ENETDOWN?
> 
I've been thinking about it for a long time, and I thought this might be more
appropriate. But now I think that use the !fep->link here is more better
than !speed, so -ENETDOWN is more proper.