Re: [PATCH v2 2/2] net: phy: microchip: enable downshift by default on LAN88xx

[Nicolai Buchwitz <nb@tipi-net.de>]

On 31.3.2026 14:01, Russell King (Oracle) wrote:
> On Tue, Mar 31, 2026 at 01:58:55PM +0200, Nicolai Buchwitz wrote:
>> On 31.3.2026 13:32, Russell King (Oracle) wrote:
>> > On Tue, Mar 31, 2026 at 12:46:27AM +0200, Nicolai Buchwitz wrote:
>> > > Enable auto-downshift from 1000BASE-T to 100BASE-TX after 2 failed
>> > > auto-negotiation attempts by default. This ensures that links with
>> > > faulty or missing cable pairs (C and D) fall back to 100Mbps without
>> > > requiring userspace configuration.
>> > >
>> > > Users can override or disable downshift at runtime:
>> > >
>> > >   ethtool --set-phy-tunable eth0 downshift off
>> > >
>> > > Signed-off-by: Nicolai Buchwitz <nb@tipi-net.de>
>> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>> >
>> > I'm slightly concerned by this commit. ->config_init() is called when
>> > the netdev attaches the PHY, and also during the resume path - and it's
>> > the second one which I believe is a problem here.
>> >
>> > If the user has configured the downshift, it is reasonable for the user
>> > to expect the setting to be preserved over a suspend/resume. However,
>> > by placing this code in ->config_init(), you will overwrite the user's
>> > setting when the system resumes.
>> 
>> You have a valid point. Looking at other drivers, Marvell has the
>> same issue: m88e1112_config_init() unconditionally sets downshift to 3
>> on every config_init call.
>> 
>> I see two options:
>> 
>> 1. Save the user's setting in the driver's priv struct and restore it
>> in config_init instead of blindly applying the default.
>> 
>> 2. Handle it generically in the PHY core, saving/restoring tunable
>> state across suspend/resume for all drivers.
>> 
>> I'd lean towards (1) to keep this series simple. (2) could be a
>> follow-up that fixes Marvell and others too. What do you think?
> 
> Or (3) configure the default it in the probe function?

Unfortunately the driver has .soft_reset = genphy_soft_reset which
runs before config_init on resume and would clear the setting? So
config_init still needs to set it.

Nicolai