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

[Lei Wei]

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.