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.
