Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 6 Dec 2022 19:06:02 +0100
From:   Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
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
Message-ID: <Y4+Eigd6PZ3tgFzG@gvm01>
References: <cover.1670329232.git.piergiorgio.beruto@gmail.com>
 <1816cb14213fc2050b1a7e97a68be7186340d994.1670329232.git.piergiorgio.beruto@gmail.com>
 <Y49IBR8ByMQH6oVt@lunn.ch>
 <Y49THkXZdLBR6Mxv@gvm01>
 <Y49YQOOhAslQQ9zt@shell.armlinux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y49YQOOhAslQQ9zt@shell.armlinux.org.uk>
Precedence: bulk

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