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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Z4B6DqpZG55aGVh9@makrotopia.org>
Date: Fri, 10 Jan 2025 01:38:22 +0000
From: Daniel Golle <daniel@...rotopia.org>
To: Lei Wei <quic_leiwei@...cinc.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Andrew Lunn <andrew@...n.ch>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, quic_kkumarcs@...cinc.com,
	quic_suruchia@...cinc.com, quic_pavir@...cinc.com,
	quic_linchen@...cinc.com, quic_luoj@...cinc.com,
	srinivas.kandagatla@...aro.org, bartosz.golaszewski@...aro.org,
	vsmuthu@....qualcomm.com, john@...ozen.org
Subject: Re: [PATCH net-next v4 3/5] net: pcs: qcom-ipq9574: Add PCS
 instantiation and phylink operations

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