Re: [PATCH RFC net-next 01/10] net: pcs: xpcs: move PCS reset to .pcs_pre_config()

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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().

> > +	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.

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

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!