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.
