RE: [PATCH net-next 1/1] net: stmmac: add check for advertising linkmode request for set-eee

[Gan, Yi Fang]

> -----Original Message-----
> From: Russell King <linux@xxxxxxxxxxxxxxx>
> Sent: Tuesday, October 31, 2023 5:09 PM
> To: Gan, Yi Fang <yi.fang.gan@xxxxxxxxx>
> Cc: Alexandre Torgue <alexandre.torgue@xxxxxxxxxxx>; Jose Abreu
> <joabreu@xxxxxxxxxxxx>; David S . Miller <davem@xxxxxxxxxxxxx>; Eric
> Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo
> Abeni <pabeni@xxxxxxxxxx>; Maxime Coquelin
> <mcoquelin.stm32@xxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-stm32@st-md-
> mailman.stormreply.com; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Looi, Hong Aun <hong.aun.looi@xxxxxxxxx>; Voon,
> Weifeng <weifeng.voon@xxxxxxxxx>; Song, Yoong Siang
> <yoong.siang.song@xxxxxxxxx>; Ahmad Tarmizi, Noor Azura
> <noor.azura.ahmad.tarmizi@xxxxxxxxx>
> Subject: Re: [PATCH net-next 1/1] net: stmmac: add check for advertising
> linkmode request for set-eee
> 
> On Tue, Oct 31, 2023 at 08:44:23AM +0000, Gan, Yi Fang wrote:
> > Hi Russell King,
> >
> > > Why should this functionality be specific to stmmac?
> > This functionality is not specific to stmmac but other drivers can
> > have their  own implementation.
> > (e.g.
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/ql
> > ogic/qede/qede_ethtool.c#L1855)
> 
> This is probably wrong (see below.)
> 
> >
> > > Why do we need this?
> > Current implementation will not take any effect if user enters
> > unsupported value but user might not aware. With this, an error will be
> prompted if unsupported value is given.
> 
> Why can't the user read back what settings were actually set like the other
> ethtool APIs? This is how ETHTOOL_GLINKSETTINGS works.
> 
For current implementation, the behaviour of "fail to set" and
"set successfully" are the same from user's perspective. 
But yes, user have responsibility to read back and check the setting.

> > > What is wrong with the checking and masking that phylib is doing?
> > Nothing wrong with the phylib but there is no error return back to
> > ethtool commands if unsupported value is given.
> 
> Maybe because that is the correct implementation?
> 
Yes, I agree with this.

> > > Why should we trust the value in edata->supported provided by the user?
> > The edata->supported is getting from the current setting and the value is set
> upon bootup.
> > Users are not allowed to change it.
> 
> "not allowed" but there is nothing that prevents it. So an easy way to bypass
> your check is:
> 
> 	struct ethtool_eee eeecmd;
> 
> 	eeecmd.cmd = ETHTOOL_GEEE;
> 	send_ioctl(..., &eeecmd);
> 
> 	eeecmd.cmd = ETHTOOL_SEEE;
> 	eeecmd.supported = ~0;
> 	eeecmd.advertised = ~0;
> 	error = send_ioctl(..., &eeecmd);
> 
> and that won't return any error. So your check is weak at best, and relies upon
> the user doing the right thing.
> 
Thank you for the example.

> > > Sorry, but no. I see no reason that this should be done, especially not in the
> stmmac driver.
> > I understand your reasoning. From your point of view, is this kind of
> > error message/ error handling not needed?
> 
> It is not - ethtool APIs don't return errors if the advertise mask is larger than the
> supported mask - they merely limit to what is supported and set that. When
> subsequently querying the settings, they return what is actually set (so the
> advertise mask will always be a subset of the supported mask at that point.)
> 
> So, if in userspace you really want to know if some modes were dropped, then
> you have to do a set-get-check sequence.
>
Thank you for all the feedbacks and explanations. It was a great discussion.
I understand your concerns and reasonings. Let's close the review for this patch. 

Thanks.
Best Regards,
Fang