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

[Lei Wei <quic_leiwei@quicinc.com>]

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

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.

> 
>