netdev - 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: <8ac3167c-c8aa-4ddb-948f-758714df7495@quicinc.com>
Date: Thu, 9 Jan 2025 21:11:05 +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/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.

Is it possible to share the log/command-options of the smatch failure so 
that we can reproduce this? Thanks.


>> +	}
>> +
>> +	return 0;
>> +}
> 
> ...
>