[<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.
Powered by blists - more mailing lists