netdev - Re: [PATCH RFC net-next 01/10] net: pcs: xpcs: move PCS reset to .pcs_pre

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] [day] [month] [year] [list]

Message-ID: <7drrlatwrjf3x77k3fswobcmbquye5luu2thoxwahqmodsm3ur@c6vqwcgtwnot>
Date: Tue, 1 Oct 2024 23:20:51 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>, 
	Alexandre Torgue <alexandre.torgue@...s.st.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Florian Fainelli <f.fainelli@...il.com>, 
	Jakub Kicinski <kuba@...nel.org>, Jiawen Wu <jiawenwu@...stnetic.com>, 
	Jose Abreu <joabreu@...opsys.com>, Jose Abreu <Jose.Abreu@...opsys.com>, 
	linux-arm-kernel@...ts.infradead.org, linux-stm32@...md-mailman.stormreply.com, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, Mengyuan Lou <mengyuanlou@...-swift.com>, netdev@...r.kernel.org, 
	Paolo Abeni <pabeni@...hat.com>, Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH RFC net-next 01/10] net: pcs: xpcs: move PCS reset to
 .pcs_pre_config()

On Mon, Sep 30, 2024 at 11:14:15AM GMT, Russell King (Oracle) wrote:
> On Mon, Sep 30, 2024 at 01:16:57AM +0300, Serge Semin wrote:
> > Hi Russell
> > 
> > On Mon, Sep 23, 2024 at 03:00:59PM GMT, Russell King (Oracle) wrote:
> > > +static void xpcs_pre_config(struct phylink_pcs *pcs, phy_interface_t interface)
> > > +{
> > > +	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
> > > +	const struct dw_xpcs_compat *compat;
> > > +	int ret;
> > > +
> > > +	if (!xpcs->need_reset)
> > > +		return;
> > > +
> > 
> > > +	compat = xpcs_find_compat(xpcs->desc, interface);
> > > +	if (!compat) {
> > > +		dev_err(&xpcs->mdiodev->dev, "unsupported interface %s\n",
> > > +			phy_modes(interface));
> > > +		return;
> > > +	}
> > 
> > Please note, it's better to preserve the xpcs_find_compat() call even
> > if the need_reset flag is false, since it makes sure that the
> > PHY-interface is actually supported by the PCS.
>
 
> Sorry, but I strongly disagree. xpcs_validate() will already have dealt
> with that, so we can be sure at this point that the interface is always
> valid. The NULL check is really only there because it'll stop the
> "you've forgotten to check for NULL on this function that can return
> NULL" brigade endlessly submitting patches to add something there -
> just like xpcs_get_state() and xpcs_do_config().

Thanks for the detailed answer. Indeed, I missed the part that the
pcs_validate() already does the interface check.

> 
> > > +	bool need_reset;
> > 
> > If you still prefer the PCS-reset being done in the pre_config()
> > function, then what about just directly checking the PMA id in there?
> > 
> > 	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
> > 		return 0;
> > 
> > 	return xpcs_soft_reset(xpcs);
> 
> I think you've missed what "need_reset" is doing as you seem to
> think it's just to make it conditional on the PMA ID. That's only
> part of the story.
> 
> In the existing code, the reset only happens _once_ when the create
> happens, not every time the PCS is configured. I am preserving this
> behaviour, because I do _NOT_ wish to incorporate multiple functional
> changes into one patch - and certainly in a cleanup series keep the
> number of functional changes to a minimum.

Ok. So the goal is to preserve the semantics. Seems reasonable. But... 

> 
> So, all in all, I don't see the need to change anything in my patch.

I'll get back to this patch discussion in the v1 series since you have
already submitted it.

-Serge(y)

> 
> Thanks for the feedback anyway.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!