Messages in this thread | | | Date | Tue, 31 Oct 2023 09:08:49 +0000 | From | "Russell King (Oracle)" <> | 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/qlogic/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.
> > 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?
> > 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.
> > 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.
Thanks.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
| |