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

[Serge Semin]

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!