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

[Daniel Golle <daniel@makrotopia.org>]

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?

> +	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@makrotopia.org/


Cheers


Daniel