On Wed, 15 Jul 2020 09:03:45 +0200 Helmut Grohne wrote:
> 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?
Is
Case C: driver does not initialize advertised at all and depends on
phy_remove_link_mode() to do it
possible?
> 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?
Hm. I think it's clear that the change may uncover other bugs, but
perhaps indeed those should be addressed elsewhere.
Andrew, WDYT?
