Re: [PATCH v4 net-next 5/5] drivers/net/phy: add driver for the onsemi NCN26000 10BASE-T1S PHY

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
	Oleksij Rempel <o.rempel@pengutronix.de>
Subject: Re: [PATCH v4 net-next 5/5] drivers/net/phy: add driver for the onsemi NCN26000 10BASE-T1S PHY
Date: Tue, 6 Dec 2022 14:57:04 +0000	[thread overview]
Message-ID: <Y49YQOOhAslQQ9zt@shell.armlinux.org.uk> (raw)
In-Reply-To: <Y49THkXZdLBR6Mxv@gvm01>

On Tue, Dec 06, 2022 at 03:35:10PM +0100, Piergiorgio Beruto wrote:
> On Tue, Dec 06, 2022 at 02:47:49PM +0100, Andrew Lunn wrote:
> > > +static int ncn26000_read_status(struct phy_device *phydev)
> > > +{
> > > +	// The NCN26000 reports NCN26000_LINK_STATUS_BIT if the link status of
> > > +	// the PHY is up. It further reports the logical AND of the link status
> > > +	// and the PLCA status in the BMSR_LSTATUS bit. Thus, report the link
> > > +	// status by testing the appropriate BMSR bit according to the module's
> > > +	// parameter configuration.
> > > +	const int lstatus_flag = link_status_plca ?
> > > +		BMSR_LSTATUS : NCN26000_BMSR_LINK_STATUS_BIT;
> > > +
> > > +	int ret;
> > > +
> > > +	ret = phy_read(phydev, MII_BMSR);
> > > +	if (unlikely(ret < 0))
> > > +		return ret;
> > > +
> > > +	// update link status
> > > +	phydev->link = (ret & lstatus_flag) ? 1 : 0;
> > 
> > What about the latching behaviour of LSTATUS?
> See further down.
> 
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L2289
> > 
> > > +
> > > +	// handle more IRQs here
> > 
> > You are not in an IRQ handler...
> Right, this is just a left-over when I moved the code from the ISR to
> this functions. Fixed.
> 
> > You should also be setting speed and duplex. I don't think they are
> > guaranteed to have any specific value if you don't set them.
> Ah, I got that before, but I removed it after comment from Russell
> asking me not to do this. Testing on my HW, this seems to work, although
> I'm not sure whether this is correct or it is working 'by chance' ?

I asked you to get rid of them in the config function, which was
setting them to "unknown" values. I thought I explained why it was
wrong to set them there - but again...

If you force the values in the config function, then when userspace
does a read-modify-write of the settings via ethtool, you will end
up wiping out the PHYs link settings, despite maybe nothing having
actually been changed. It is also incorrect to set them in the
config function, because those writes to those variables can race
with users reading them - the only place they should be set by a
PHY driver is in the .read_status method.

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