On Tue, Jul 14, 2020 at 11:07:10PM +0200, David Miller wrote:
> From: Helmut Grohne <[email protected]>
> Date: Tue, 14 Jul 2020 10:25:42 +0200
>
> > When doing "ip link set dev ... up" for a ksz9477 backed link,
> > ksz9477_phy_setup is called and it calls phy_remove_link_mode to remove
> > 1000baseT HDX. During phy_remove_link_mode, phy_advertise_supported is
> > called.
> >
> > If one wants to advertise fewer modes than the supported ones, one
> > usually reduces the advertised link modes before upping the link (e.g.
> > by passing an appropriate .link file to udev). However upping
> > overrwrites the advertised link modes due to the call to
> > phy_advertise_supported reverting to the supported link modes.
> >
> > It seems unintentional to have phy_remove_link_mode enable advertising
> > bits and it does not match its description in any way. Instead of
> > calling phy_advertise_supported, we should simply clear the link mode to
> > be removed from both supported and advertising.
>
> The problem is that we can't allow the advertised setting to exceed
> what is in the supported list.
>
> That's why this helper is coded this way from day one.
Would you mind going into a little more detail here?
I think you have essentially two possible cases with respect to that
assertion.
Case A: advertised does not exceed supported before the call to
phy_remove_link_mode.
In this case, the relevant link mode is removed from both supported
and advertised after my patch and therefore the requested invariant
is still ok.
Case B: advertised exceeds supported prior to the call to
phy_remove_link_mode.
You said that we cannot allow this to happen. So it would seem to be
a bug somewhere else. Do you see phy_remove_link_mode as a tool to
fix up a violated invariant?
It also is not true that the current code ensures your assertion.
Specifically, phy_advertise_supported copies the pause bits from the old
advertised to the new one regardless of whether they're set in
supported. I believe this is expected, but it means that your invariant
needs to be:
We cannot allow advertised to exceed the supported list for
non-pause bits.
In any case, having a helper called "phy_remove_link_mode" enable bits
in the advertised bit field is fairly unexpected. Do you disagree with
this being a bug?
Helmut
