On 1/10/2025 6:52 PM, Simon Horman wrote:
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.
OK.
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.
Thanks for sharing this information.
