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

[Simon Horman <horms@kernel.org>]

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@quicinc.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.