Re: [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii

[Andrew Lunn <andrew@lunn.ch>]

On Mon, Jul 02, 2018 at 11:54:54PM +0200, Heiner Kallweit wrote:
> On 02.07.2018 23:21, Andrew Lunn wrote:
> >> -		auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
> > 
> > This bit you probably want to keep. The PHY never says it support
> > Pause. The MAC needs to enable pause if the MAC supports pause.
> > 
> Actually I assumed that phylib would do this for me. But:
> In phy_probe() first phydev->supported is copied to
> phydev->advertising, and only after this both pause flags are added
> to phydev->supported. Therefore I think they are not advertised.
> Is this intentional? It sounds a little weird to me to add the
> pause flags to the supported features per default, but not
> advertise them.

phylib has no idea if the MAC supports Pause. So it should not enable
it by default. The MAC needs to enable it. And a lot of MAC drivers
get this wrong...

> Except e.g. we call by chance phy_set_max_speed(), which copies
> phydev->supported to phydev->advertising after having adjusted
> the supported speeds.

As you correctly pointed out, phy_set_max_speed() is masking out too
much.

> If this is not a bug, then where would be the right place to add
> the pause flags to phydev->advertising?

Before you call phy_start().

       Andrew