Re: [PATCH 1/1] net: phy: marvell: avoid bringing down fibre link when autoneg is bypassed

On Fri, Oct 04, 2024 at 11:35:30AM +1000, Qingtao Cao wrote:

Hi Andrew,

Please see my inline replies.

On Fri, Oct 4, 2024 at 12:30 AM Andrew Lunn [off-list ref] wrote:

    On Thu, Oct 03, 2024 at 12:25:12PM +1000, Qingtao Cao wrote:
    > On 88E151x the SGMII autoneg bypass mode defaults to be enabled. When it
    is
    > activated, the device assumes a link-up status with existing
    configuration
    > in BMCR, avoid bringing down the fibre link in this case
    >
    > Test case:
    > 1. Two 88E151x connected with SFP, both enable autoneg, link is up with
    speed
    >    1000M
    > 2. Disable autoneg on one device and explicitly set its speed to 1000M
    > 3. The fibre link can still up with this change, otherwise not.

    What is actually wrong here?

    If both ends are performing auto-neg, i would expect a link at the
    highest speeds both link peers support.

    If one peer is doing autoneg, the other not, i expect link down, this
    is not a valid configuration, since one peer is going to fail to
    auto-neg.


Well, technically speaking, thanks to the 88E151X's bypass mode, in such case
with one end using autoneg but the other is using 1000M explicitly, the link
could still be up, but not with the current code.

So we can make an invalid configuration work. Question is, should we?

Are we teaching users they can wrongly configure their system and
expect it to work? They then think it is actually a valid
configuration and try the same on some other board with other PHYs,
and find it does not work?

Does Marvell document why this bypass mode exists? When it should be
used? What do they see as its use cases?

	Andrew

On Fri, Oct 04, 2024 at 03:26:33PM +0200, Andrew Lunn wrote:

On Fri, Oct 04, 2024 at 11:35:30AM +1000, Qingtao Cao wrote:

quoted

Hi Andrew,

Please see my inline replies.

On Fri, Oct 4, 2024 at 12:30 AM Andrew Lunn [off-list ref] wrote:

    On Thu, Oct 03, 2024 at 12:25:12PM +1000, Qingtao Cao wrote:
    > On 88E151x the SGMII autoneg bypass mode defaults to be enabled. When it
    is
    > activated, the device assumes a link-up status with existing
    configuration
    > in BMCR, avoid bringing down the fibre link in this case
    >
    > Test case:
    > 1. Two 88E151x connected with SFP, both enable autoneg, link is up with
    speed
    >    1000M
    > 2. Disable autoneg on one device and explicitly set its speed to 1000M
    > 3. The fibre link can still up with this change, otherwise not.

    What is actually wrong here?

    If both ends are performing auto-neg, i would expect a link at the
    highest speeds both link peers support.

    If one peer is doing autoneg, the other not, i expect link down, this
    is not a valid configuration, since one peer is going to fail to
    auto-neg.


Well, technically speaking, thanks to the 88E151X's bypass mode, in such case
with one end using autoneg but the other is using 1000M explicitly, the link
could still be up, but not with the current code.

So we can make an invalid configuration work. Question is, should we?

Are we teaching users they can wrongly configure their system and
expect it to work? They then think it is actually a valid
configuration and try the same on some other board with other PHYs,
and find it does not work?

Does Marvell document why this bypass mode exists? When it should be
used? What do they see as its use cases?

The paragraph about it is couched in terms of "if the MAC or the PHY
implements the auto-negotiation function and the other end does not".

That seems to point towards a MAC <-> PHY link rather than across a
media. So I tend to agree with you that we should not be enabling
bypass mode on a media side link.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!