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: <20250110105252.GY7706@kernel.org>
Date: Fri, 10 Jan 2025 10:52:52 +0000
From: Simon Horman <horms@...nel.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

On Thu, Jan 09, 2025 at 09:11:05PM +0800, Lei Wei wrote:
> 
> 
> On 1/8/2025 6:03 PM, Simon Horman wrote:
> > On Wed, Jan 08, 2025 at 10:50:26AM +0800, Lei Wei wrote:
> > > This patch adds the following PCS functionality for the PCS driver
> > > for IPQ9574 SoC:
> > > 
> > > a.) Parses PCS MII DT nodes and instantiate each MII PCS instance.
> > > b.) Exports PCS instance get and put APIs. The network driver calls
> > > the PCS get API to get and associate the PCS instance with the port
> > > MAC.
> > > c.) PCS phylink operations for SGMII/QSGMII interface modes.
> > > 
> > > Signed-off-by: Lei Wei <quic_leiwei@...cinc.com>
> > 
> > ...
> > 
> > > +static int ipq_pcs_enable(struct phylink_pcs *pcs)
> > > +{
> > > +	struct ipq_pcs_mii *qpcs_mii = phylink_pcs_to_qpcs_mii(pcs);
> > > +	struct ipq_pcs *qpcs = qpcs_mii->qpcs;
> > > +	int index = qpcs_mii->index;
> > > +	int ret;
> > > +
> > > +	ret = clk_prepare_enable(qpcs_mii->rx_clk);
> > > +	if (ret) {
> > > +		dev_err(qpcs->dev, "Failed to enable MII %d RX clock\n", index);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = clk_prepare_enable(qpcs_mii->tx_clk);
> > > +	if (ret) {
> > > +		dev_err(qpcs->dev, "Failed to enable MII %d TX clock\n", index);
> > > +		return ret;
> > 
> > Hi Lei Wei,
> > 
> > I think you need something like the following to avoid leaking qpcs_mii->rx_clk.
> > 
> > 		goto err_disable_unprepare_rx_clk;
> > 	}
> > 
> > 	return 0;
> > 
> > err_disable_unprepare_rx_clk:
> > 	clk_disable_unprepare(qpcs_mii->rx_clk);
> > 	return ret;
> > }
> > 
> > Flagged by Smatch.
> > 
> 
> We had a conversation with Russell King in v2 that even if the phylink pcs
> enable sequence encounters an error, it does not unwind the steps it has
> already done. So we removed the call to unprepare in case of error here,
> since an error here is essentially fatal in this path with no unwind
> possibility.
> 
> https://lore.kernel.org/all/38d7191f-e4bf-4457-9898-bb2b186ec3c7@quicinc.com/
> 
> However to satisfy this smatch warning/error, we may need to revert back to
> the adding the unprepare call in case of error. Request Russel to comment as
> well if this is fine.

Thanks, I had missed that.

I don't think there is a need to update the code just to make Smatch happy.
Only if there is a real problem. Which, with the discussion at the link
above in mind, does not seem to be the case here.

> Is it possible to share the log/command-options of the smatch failure so
> that we can reproduce this? Thanks.

Sure, I hope this answers your question.

Smatch can be found here https://github.com/error27/smatch/

And I invoked it like this:
$ PATH=".../smatch/bin:$PATH" .../smatch/smatch_scripts/kchecker drivers/net/pcs/pcs-qcom-ipq9574.o

Which yields the following warning:
drivers/net/pcs/pcs-qcom-ipq9574.c:283 ipq_pcs_enable() warn: 'qpcs_mii->rx_clk' from clk_prepare_enable() not released on lines: 280.