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

[Piergiorgio Beruto <piergiorgio.beruto@gmail.com>]

On Tue, Dec 06, 2022 at 02:57:04PM +0000, Russell King (Oracle) wrote:
> 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.
Ok, I must have misunderstood what the problem was. This is clear to me
now, I'm going to add this back in the read_status() method.

Thanks,
Piergiorgio