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

[Russell King (Oracle)]

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@xxxxxxxxxxx>
> > > Reviewed-by: Andrew Lunn <andrew@xxxxxxx>
> > 
> > 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?

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