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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 6 Dec 2022 19:06:02 +0100
From:   Piergiorgio Beruto <piergiorgio.beruto@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, Oleksij Rempel <o.rempel@...gutronix.de>
Subject: Re: [PATCH v4 net-next 5/5] drivers/net/phy: add driver for the
 onsemi NCN26000 10BASE-T1S PHY

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