[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y4+Eigd6PZ3tgFzG@gvm01>
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
Powered by blists - more mailing lists