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@xxxxxxxxxxx>
> >
> > ...
> >
> > > +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@xxxxxxxxxxx/
>
> 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.
