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@xxxxxxxxxxxxxx/
Cheers
Daniel
