From: Maxime Chevallier <maxime.chevallier@bootlin•com>
To: "Russell King (Oracle)" <linux@armlinux•org.uk>
Cc: "Andrew Lunn" <andrew@lunn•ch>,
"Heiner Kallweit" <hkallweit1@gmail•com>,
"Alexander Couzens" <lynxis@fe80•eu>,
"Alexander Duyck" <alexanderduyck@fb•com>,
"Andrew Lunn" <andrew+netdev@lunn•ch>,
"AngeloGioacchino Del Regno"
<angelogioacchino.delregno@collabora•com>,
"Arınç ÜNAL" <arinc.unal@arinc9•com>,
"Claudiu Beznea" <claudiu.beznea@tuxon•dev>,
"Daniel Golle" <daniel@makrotopia•org>,
"Daniel Machon" <daniel.machon@microchip•com>,
"David S. Miller" <davem@davemloft•net>,
"DENG Qingfang" <dqfext@gmail•com>,
"Eric Dumazet" <edumazet@google•com>,
"Eric Woudstra" <ericwouds@gmail•com>,
"Florian Fainelli" <florian.fainelli@broadcom•com>,
"Horatiu Vultur" <horatiu.vultur@microchip•com>,
"Ioana Ciornei" <ioana.ciornei@nxp•com>,
"Jakub Kicinski" <kuba@kernel•org>,
"Jose Abreu" <Jose.Abreu@synopsys•com>,
kernel-team@meta•com, "Lars Povlsen" <lars.povlsen@microchip•com>,
linux-arm-kernel@lists•infradead.org,
linux-mediatek@lists•infradead.org,
"Madalin Bucur" <madalin.bucur@nxp•com>,
"Marcin Wojtas" <marcin.s.wojtas@gmail•com>,
"Matthias Brugger" <matthias.bgg@gmail•com>,
"Michal Simek" <michal.simek@amd•com>,
netdev@vger•kernel.org,
"Nicolas Ferre" <nicolas.ferre@microchip•com>,
"Paolo Abeni" <pabeni@redhat•com>,
"Radhey Shyam Pandey" <radhey.shyam.pandey@amd•com>,
"Sean Anderson" <sean.anderson@seco•com>,
"Sean Wang" <sean.wang@mediatek•com>,
"Steen Hegelund" <Steen.Hegelund@microchip•com>,
"Taras Chornyi" <taras.chornyi@plvision•eu>,
UNGLinuxDriver@microchip•com,
"Vladimir Oltean" <olteanv@gmail•com>
Subject: Re: [PATCH net-next v2 0/5] net: phylink: fix PCS without autoneg
Date: Mon, 13 Jan 2025 17:22:55 +0100 [thread overview]
Message-ID: <20250113172255.3772dd6a@fedora.home> (raw)
In-Reply-To: <Z4TbR93B-X8A8iHe@shell.armlinux.org.uk>
Hello Russell,
On Mon, 13 Jan 2025 09:22:15 +0000
"Russell King (Oracle)" <linux@armlinux•org.uk> wrote:
> Hi,
>
> Eric Woudstra reported that a PCS attached using 2500base-X does not
> see link when phylink is using in-band mode, but autoneg is disabled,
> despite there being a valid 2500base-X signal being received. We have
> these settings:
>
> act_link_an_mode = MLO_AN_INBAND
> pcs_neg_mode = PHYLINK_PCS_NEG_INBAND_DISABLED
>
> Eric diagnosed it to phylink_decode_c37_word() setting state->link
> false because the full-duplex bit isn't set in the non-existent link
> partner advertisement word (which doesn't exist because in-band
> autoneg is disabled!)
>
> The test in phylink_mii_c22_pcs_decode_state() is supposed to catch
> this state, but since we converted PCS to use neg_mode, testing the
> Autoneg in the local advertisement is no longer sufficient - we need
> to be looking at the neg_mode, which currently isn't provided.
>
> We need to provide this via the .pcs_get_state() method, and this
> will require modifying all PCS implementations to add the extra
> argument to this method.
>
> Patch 1 uses the PCS neg_mode in phylink_mac_pcs_get_state() to correct
> the now obsolute usage of the Autoneg bit in the advertisement.
>
> Patch 2 passes neg_mode into the .pcs_get_state() method, and updates
> all users.
>
> Patch 3 adds neg_mode as an argument to the various clause 22 state
> decoder functions in phylink, modifying drivers to pass the neg_mode
> through.
>
> Patch 4 makes use of phylink_mii_c22_pcs_decode_state() rather than
> using the Autoneg bit in the advertising field.
>
> Patch 5 may be required for Eric's case - it ensures that we report
> the correct state for interface types that we support only one set
> of modes for when autoneg is disabled.
>
> Changes in v2:
> - Add test for NULL pcs in patch 1
>
> I haven't added Eric's t-b because I used a different fix in patch 1.
I stumbled on that issue last friday as well, with a MCBin and a
device I'm working on, using 1000BaseX with autoneg disabled. I didn't
get time to investigate back then, but reading this series it was
definitely that exact problem I was facing.
I missed your V1 and I just tested that V2, the problem is gone :)
Thanks !
The code LGTM to the best of my knowledge, so
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin•com>
Tested-by: Maxime Chevallier <maxime.chevallier@bootlin•com>
Thanks,
Maxime
next prev parent reply other threads:[~2025-01-13 16:23 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-13 9:22 [PATCH net-next v2 0/5] net: phylink: fix PCS without autoneg Russell King (Oracle)
2025-01-13 9:22 ` [PATCH net-next v2 1/5] net: phylink: use pcs_neg_mode in phylink_mac_pcs_get_state() Russell King (Oracle)
2025-01-13 9:22 ` [PATCH net-next v2 2/5] net: phylink: pass neg_mode into .pcs_get_state() method Russell King (Oracle)
2025-01-13 9:22 ` [PATCH net-next v2 3/5] net: phylink: pass neg_mode into c22 state decoder Russell King (Oracle)
2025-01-13 9:22 ` [PATCH net-next v2 4/5] net: phylink: use neg_mode in phylink_mii_c22_pcs_decode_state() Russell King (Oracle)
2025-01-13 9:22 ` [PATCH net-next v2 5/5] net: phylink: provide fixed state for 1000base-X and 2500base-X Russell King (Oracle)
2025-01-14 12:57 ` Vladimir Oltean
2025-01-14 13:31 ` Russell King (Oracle)
2025-01-13 16:22 ` Maxime Chevallier [this message]
2025-01-15 21:30 ` [PATCH net-next v2 0/5] net: phylink: fix PCS without autoneg patchwork-bot+netdevbpf
-- strict thread matches above, loose matches on Subject: below --
2025-01-09 15:15 [PATCH net-next " Russell King (Oracle)
2025-01-13 9:21 ` [PATCH net-next v2 " Russell King (Oracle)
2025-01-13 10:14 ` Russell King (Oracle)
2025-01-14 10:59 ` Eric Woudstra
2025-01-14 11:14 ` Russell King (Oracle)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250113172255.3772dd6a@fedora.home \
--to=maxime.chevallier@bootlin$(echo .)com \
--cc=Jose.Abreu@synopsys$(echo .)com \
--cc=Steen.Hegelund@microchip$(echo .)com \
--cc=UNGLinuxDriver@microchip$(echo .)com \
--cc=alexanderduyck@fb$(echo .)com \
--cc=andrew+netdev@lunn$(echo .)ch \
--cc=andrew@lunn$(echo .)ch \
--cc=angelogioacchino.delregno@collabora$(echo .)com \
--cc=arinc.unal@arinc9$(echo .)com \
--cc=claudiu.beznea@tuxon$(echo .)dev \
--cc=daniel.machon@microchip$(echo .)com \
--cc=daniel@makrotopia$(echo .)org \
--cc=davem@davemloft$(echo .)net \
--cc=dqfext@gmail$(echo .)com \
--cc=edumazet@google$(echo .)com \
--cc=ericwouds@gmail$(echo .)com \
--cc=florian.fainelli@broadcom$(echo .)com \
--cc=hkallweit1@gmail$(echo .)com \
--cc=horatiu.vultur@microchip$(echo .)com \
--cc=ioana.ciornei@nxp$(echo .)com \
--cc=kernel-team@meta$(echo .)com \
--cc=kuba@kernel$(echo .)org \
--cc=lars.povlsen@microchip$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-mediatek@lists$(echo .)infradead.org \
--cc=linux@armlinux$(echo .)org.uk \
--cc=lynxis@fe80$(echo .)eu \
--cc=madalin.bucur@nxp$(echo .)com \
--cc=marcin.s.wojtas@gmail$(echo .)com \
--cc=matthias.bgg@gmail$(echo .)com \
--cc=michal.simek@amd$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nicolas.ferre@microchip$(echo .)com \
--cc=olteanv@gmail$(echo .)com \
--cc=pabeni@redhat$(echo .)com \
--cc=radhey.shyam.pandey@amd$(echo .)com \
--cc=sean.anderson@seco$(echo .)com \
--cc=sean.wang@mediatek$(echo .)com \
--cc=taras.chornyi@plvision$(echo .)eu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox