netdev - Re: [PATCH net-next v3 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] [day] [month] [year] [list]

Message-ID: <7cac0b93-c1eb-4269-b397-794f43a8507a@quicinc.com>
Date: Fri, 3 Jan 2025 18:14:36 +0800
From: Lei Wei <quic_leiwei@...cinc.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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>, <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 v3 3/5] net: pcs: qcom-ipq9574: Add PCS
 instantiation and phylink operations



On 1/2/2025 6:54 PM, Russell King (Oracle) wrote:
> Hi,
> 
> On Mon, Dec 16, 2024 at 09:40:25PM +0800, Lei Wei wrote:
>> +static int ipq_pcs_config_sgmii(struct ipq_pcs *qpcs,
>> +				int index,
>> +				unsigned int neg_mode,
>> +				phy_interface_t interface)
>> +{
>> +	int ret;
>> +
>> +	/* Access to PCS registers such as PCS_MODE_CTRL which are
>> +	 * common to all MIIs, is lock protected and configured
>> +	 * only once.
>> +	 */
>> +	mutex_lock(&qpcs->config_lock);
>> +
>> +	if (qpcs->interface != interface) {
>> +		ret = ipq_pcs_config_mode(qpcs, interface);
>> +		if (ret) {
>> +			mutex_unlock(&qpcs->config_lock);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	mutex_unlock(&qpcs->config_lock);
> 
> Phylink won't make two concurrent calls to this function (it's protected
> by phylink's state_lock). Since this looks to me like "qpcs" is per PCS,
> the lock does nothing that phylink doesn't already do.
> 

The per phylink pcs instance is "qpcs_mii" and not "qpcs". The 
"config_lock" is to protect from concurrent configurations for each of 
MII ports in case of QSGMII mode where there is common register access.

However after taking a re-look in the case of QSGMII, I think it may be 
OK to remove this lock from the driver. This is because the phylink pcs 
config called by phylink_mac_initial_config() during phylink_start() is 
protected by the rtnl_mutex, which ensures that each netdev starts/opens 
sequentially. After that, for the QSGMII case, the interface mode will 
never change when the phy's link is resolved again. So, I think this 
lock can be removed.

>> +static const struct phylink_pcs_ops ipq_pcs_phylink_ops = {
>> +	.pcs_validate = ipq_pcs_validate,
> 
> I would also like to see the recently added .pcs_inband_caps() method
> implemented too, so that phylink gets to know whether inband can be
> supported by the PCS.
> 

Sure, I will add this method in next update. I will rebase this update 
on top of the latest net-next which has the .pcs_inband_caps() patch 
included. Hope this is fine.