netdev - Re: [PATCH net-next 3/5] net: pcs: qcom-ipq: Add PCS create and phylink operations for IPQ9574

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 for Android: free password hash cracker in your pocket

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

Message-ID: <a0826aa8-703c-448d-8849-47808f847774@lunn.ch>
Date: Wed, 6 Nov 2024 04:43:43 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Lei Wei <quic_leiwei@...cinc.com>
Cc: "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>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...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 3/5] net: pcs: qcom-ipq: Add PCS create and
 phylink operations for IPQ9574

On Wed, Nov 06, 2024 at 11:16:37AM +0800, Lei Wei wrote:
> 
> 
> On 11/1/2024 9:21 PM, Andrew Lunn wrote:
> > > +static int ipq_pcs_config_mode(struct ipq_pcs *qpcs,
> > > +			       phy_interface_t interface)
> > > +{
> > > +	unsigned int val;
> > > +	int ret;
> > > +
> > > +	/* Configure PCS interface mode */
> > > +	switch (interface) {
> > > +	case PHY_INTERFACE_MODE_SGMII:
> > > +		/* Select Qualcomm SGMII AN mode */
> > > +		ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
> > > +					 PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
> > > +					 PCS_MODE_SGMII);
> > 
> > How does Qualcomm SGMII AN mode differ from Cisco SGMII AN mode?
> > 
> 
> Qualcomm SGMII AN mode extends Cisco SGMII spec Revision 1.8 by adding pause
> bit support in the SGMII word format. It re-uses two of the reserved bits
> 1..9 for this purpose. The PCS supports both Qualcomm SGMII AN and Cisco
> SGMII AN modes.

Is Qualcomm SGMII AN actually needed? I assume it only works against a
Qualcomm PHY? What interoperability testing have you do against
non-Qualcomm PHYs?

> > > +struct phylink_pcs *ipq_pcs_create(struct device_node *np)
> > > +{
> > > +	struct platform_device *pdev;
> > > +	struct ipq_pcs_mii *qpcs_mii;
> > > +	struct device_node *pcs_np;
> > > +	struct ipq_pcs *qpcs;
> > > +	int i, ret;
> > > +	u32 index;
> > > +
> > > +	if (!of_device_is_available(np))
> > > +		return ERR_PTR(-ENODEV);
> > > +
> > > +	if (of_property_read_u32(np, "reg", &index))
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	if (index >= PCS_MAX_MII_NRS)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	pcs_np = of_get_parent(np);
> > > +	if (!pcs_np)
> > > +		return ERR_PTR(-ENODEV);
> > > +
> > > +	if (!of_device_is_available(pcs_np)) {
> > > +		of_node_put(pcs_np);
> > > +		return ERR_PTR(-ENODEV);
> > > +	}
> > 
> > How have you got this far if the parent is not available?
> > 
> 
> This check can fail only if the parent node is disabled in the board DTS. I
> think this error situation may not be caught earlier than this point.
> However I agree, the above check is redundant, since this check is
> immediately followed by a validity check on the 'pdev' of the parent node,
> which should be able cover any such errors as well.

This was also because the driver does not work as i expected. I was
expecting the PCS driver to walk its own DT and instantiate the PCS
devices listed. If the parent is disabled, it is clearly not going to
start its own children.  But it is in fact some other device which
walks the PCS DT blob, and as a result the child/parent relationship
is broken, a child could exist without its parent.

> > > +	for (i = 0; i < PCS_MII_CLK_MAX; i++) {
> > > +		qpcs_mii->clk[i] = of_clk_get_by_name(np, pcs_mii_clk_name[i]);
> > > +		if (IS_ERR(qpcs_mii->clk[i])) {
> > > +			dev_err(qpcs->dev,
> > > +				"Failed to get MII %d interface clock %s\n",
> > > +				index, pcs_mii_clk_name[i]);
> > > +			goto err_clk_get;
> > > +		}
> > > +
> > > +		ret = clk_prepare_enable(qpcs_mii->clk[i]);
> > > +		if (ret) {
> > > +			dev_err(qpcs->dev,
> > > +				"Failed to enable MII %d interface clock %s\n",
> > > +				index, pcs_mii_clk_name[i]);
> > > +			goto err_clk_en;
> > > +		}
> > > +	}
> > 
> > Maybe devm_clk_bulk_get() etc will help you here? I've never actually
> > used them, so i don't know for sure.
> > 
> 
> We don't have a 'device' associated with the 'np', so we could not consider
> using the "devm_clk_bulk_get()" API.

Another artefact of not have a child-parent relationship. I wounder if
it makes sense to change the architecture. Have the PCS driver
instantiate the PCS devices as its children. They then have a device
structure for calls like clk_bulk_get(), and a more normal
consumer/provider setup.

	Andrew