Re: [Patch net-next v3] net: phy: Add driver for Motorcomm Quad 2.5GbE phy

[Kyle Switch]

On 7/17/26 23:11, Andrew Lunn wrote:
>> +/**
>> + * ytphy_read_top_ext() - read a PHY's top extended register for YT8824
>> + * @phydev: a pointer to a &struct phy_device
>> + * @regnum: register number to read
>> + *
>> + * NOTE:The caller must have taken the MDIO bus lock.
> 
> Didn't i request this is done in code, not comments?

Ans: fix done in patch v4 by called lockdep_assert_held to make
sure to hold the lock.

> 
>> +static int ytphy_read_top_ext(struct phy_device *phydev, u16 regnum)
>> +{
>> +	struct yt8824_shared_priv *shared_priv;
>> +	unsigned int offset;
>> +	int ret;
>> +
>> +	shared_priv = phy_package_get_priv(phydev);
>> +	offset = shared_priv->offset;
>> +	ret = __phy_package_write(phydev, offset, YTPHY_PAGE_SELECT, regnum);
> 
> You look to be using the package wrongly.
> 
> https://elixir.bootlin.com/linux/v7.1.3/source/drivers/net/phy/phy_package.c#L192
> 
>  * The base_addr parameter serves as cookie which has to have the same values
>  * for all members of one group and as the base PHY address of the PHY package
>  * for offset calculation to access generic registers of a PHY package.
>  * Usually, one of the PHY addresses of the different PHYs in the package
>  * provides access to these global registers.
>  * The address which is given here, will be used in the __phy_package_read()
>  * and __phy_package_write() convenience functions as base and added to the
>  * passed offset in those functions.
> 
> Although it is called base, it can be above all the others. So when
> you create the package, set the cookie value to your top address. You
> can then use __phy_package_write() directly, offset of 0.

Ans: fix done in patch v4, the base_addr is used to access to the top extend register
for phy8824.

> 
>> -static struct phy_driver motorcomm_phy_drvs[] = {
>> -	{
>> -		PHY_ID_MATCH_EXACT(PHY_ID_YT8511),
>> -		.name		= "YT8511 Gigabit Ethernet",
>> -		.config_init	= yt8511_config_init,
>> -		.suspend	= genphy_suspend,
>> -		.resume		= genphy_resume,
>> -		.read_page	= yt8511_read_page,
>> -		.write_page	= yt8511_write_page,
>> -	},
> 
> I still don't like this. Please stop diff deleting all these and
> putting them back later. It makes me thing something changed, maybe
> white space?

Ans: I tried to refactor this issue by hanging the position of the newly
added code. However, the same issue may still arise in other code segments, 
without modifying the existing code.
> 
>> +static int yt8824_utp_softreset_paged(struct phy_device *phydev,
>> +				      int reg_space)
>> +{
>> +	int old_page;
>> +	int ret = 0;
>> +	int val;
>> +
>> +	old_page = phy_select_page(phydev, reg_space);
>> +	if (old_page < 0)
>> +		goto err_restore_page;
>> +	if (reg_space == YT8824_RSSR_UTP_SPACE) {
>> +		ret = __phy_read(phydev, MII_BMCR);
>> +		if (ret < 0)
>> +			goto err_restore_page;
>> +		ret |= BMCR_RESET;
>> +		ret = __phy_write(phydev, MII_BMCR, ret);
> 
> So here you are using __phy_write() so it does not take the mdio lock.
> 
>> +		if (ret < 0)
>> +			goto err_restore_page;
>> +		/* wait until softreset done. */
>> +		ret = phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET),
>> +					    50000, 600000, true);
> 
> This uses phy_read(), so does take the lock.
> 
> How can this be correct?
> 
> Please take some time to think about locking. Please enable all the
> locking debug options in the kernel and see if you get splats. And
> also really do turn your locking comments into code.
> 

Ans: this issue fixed done in patch v4. The correct procedure might be to acquire the mdio lock 
within this phy_select_page() and release it within phy_restore_page(). During this period, 
all interfaces that involve acquiring the mdio lock are unacceptable and incorrect.

>     Andrew