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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <44e82323-b40d-41ea-86ee-57c4872a46e8@quicinc.com>
Date: Mon, 13 Jan 2025 16:19:54 +0800
From: Lei Wei <quic_leiwei@...cinc.com>
To: Simon Horman <horms@...nel.org>
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 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@...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.
> 

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.

> 
>