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

[Andrew Lunn <andrew@lunn.ch>]

> +/**
> + * 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?

> +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.

> -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?

> +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.

    Andrew