Re: [PATCH net-next v4 3/5] net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations

[Lei Wei]

On 1/10/2025 9:38 AM, Daniel Golle wrote:
> Hi Lei,
>
> On Wed, Jan 08, 2025 at 10:50:26AM +0800, Lei Wei wrote:
> > ...
> > +/**
> > + * ipq_pcs_get() - Get the IPQ PCS MII instance
> > + * @np: Device tree node to the PCS MII
> > + *
> > + * Description: Get the phylink PCS instance for the given PCS MII node @np.
> > + * This instance is associated with the specific MII of the PCS and the
> > + * corresponding Ethernet netdevice.
> > + *
> > + * Return: A pointer to the phylink PCS instance or an error-pointer value.
> > + */
> > +struct phylink_pcs *ipq_pcs_get(struct device_node *np)
> > +{
> > +	struct platform_device *pdev;
> > +	struct ipq_pcs_mii *qpcs_mii;
> > +	struct ipq_pcs *qpcs;
> > +	u32 index;
> > +
> > +	if (of_property_read_u32(np, "reg", &index))
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (index >= PCS_MAX_MII_NRS)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	/* Get the parent device */
> > +	pdev = of_find_device_by_node(np->parent);
> > +	if (!pdev)
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	qpcs = platform_get_drvdata(pdev);
>
> What if the node referenced belongs to another driver?

Usually this case would not happen, unless the DTS for the ethernet 
ports are incorrectly configured. However I can add the 'compatible 
string' check to catch such cases.

> > +	if (!qpcs) {
> > +		put_device(&pdev->dev);
> > +
> > +		/* If probe is not yet completed, return DEFER to
> > +		 * the dependent driver.
> > +		 */
> > +		return ERR_PTR(-EPROBE_DEFER);
> > +	}
> > +
> > +	qpcs_mii = qpcs->qpcs_mii[index];
> > +	if (!qpcs_mii) {
> > +		put_device(&pdev->dev);
> > +		return ERR_PTR(-ENOENT);
> > +	}
> > +
> > +	return &qpcs_mii->pcs;
> > +}
> > +EXPORT_SYMBOL(ipq_pcs_get);
>
> All the above seems a bit fragile to me, and most of the comments
> Russell King has made on my series implementing a PCS driver for the
> MediaTek SoCs apply here as well, esp.:
>
> "If we are going to have device drivers for PCS, then we need to
> seriously think about how we look up PCS and return the phylink_pcs
> pointer - and also how we handle the PCS device going away. None of that
> should be coded into _any_ PCS driver."
>
> It would hence be better to implement a generic way to get/put
> phylink_pcs instances from a DT node, and take care of what happens
> if the PCS device goes away.
>
> See also
> https://patchwork.kernel.org/comment/25625601/
>
> I've since (unsucessfully) started to work on such infrastructure.
> In order to avoid repeating the same debate and mistakes, you may want
> to take a look at at:
>
> https://patchwork.kernel.org/project/netdevbpf/patch/ba4e359584a6b3bc4b3470822c42186d5b0856f9.1721910728.git.daniel@xxxxxxxxxxxxxx/
>
>
> Cheers
>
>
> Daniel

I reviewed the discussion shared above. As I understand, there were two 
concerns from Russel, that can be potentially resolved using a common 
framework for all PCS drivers:

a.) Safe handling of PCS device removals
b.) Consistency in the PCS instance lookup API

For a), please note that the PCS device in IPQ chipsets such as IPQ9574 
is built into the SoC chip and is not pluggable. Also, the PCS driver 
module is not unloadable until the GMAC driver that depends on it is 
unloaded. Therefore, marking the driver '.suppress_bind_attrs = true' to 
disable user unbind action may be good enough to cover all possible 
scenarios of device going away for IPQ9574 PCS driver.

In relation to b), while a common infrastructure is certainly preferable
for the longer term to have a consistent lookup, the urgency is perhaps
more to resolve the issue of hot-pluggable devices going away, and less
for devices that do not support it.

One another issue we can notice currently is the lack of consistency in
the PCS lookup API signatures across drivers (For ex: xxx_get() vs
xxx_create() vs xxx_select_pcs() etc). A common 
naming-convention/signature can be enforced for the lookup API across 
the newer PCS drivers like these, to bring in some consistency in lookup.

Perhaps Russel could provide his comments as well. Thanks.