[PATCH net-next] net: phy: fix check in get_phy_c45_ids

[PATCH net-next] net: phy: fix check in get_phy_c45_ids

[Vladimir Oltean]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

After the patch below, the iteration through the available MMDs is
completely short-circuited, and devs_in_pkg remains set to the initial
value of zero.

Due to devs_in_pkg being zero, the rest of get_phy_c45_ids() is
short-circuited too: the following loop never reaches below this point
either (it executes "continue" for every device in package, failing to
retrieve PHY ID for any of them):

	/* Now probe Device Identifiers for each device present. */
	for (i = 1; i < num_ids; i++) {
		if (!(devs_in_pkg & (1 << i)))
			continue;

So c45_ids->device_ids remains populated with zeroes. This causes an
Aquantia AQR412 PHY (same as any C45 PHY would, in fact) to be probed by
the Generic PHY driver.

The issue seems to be a case of submitting partially committed work (and
therefore testing something other than was submitted).

The intention of the patch was to delay exiting the loop until one more
condition is reached (the devs_in_pkg read from hardware is either 0, OR
mostly f's). So fix the patch to reflect that.

Tested with traffic on a LS1028A-QDS, the PHY is now probed correctly
using the Aquantia driver. The devs_in_pkg bit field is set to
0xe000009a, and the MMDs that are present have the following IDs:

[    5.600772] libphy: get_phy_c45_ids: device_ids[1]=0x3a1b662
[    5.618781] libphy: get_phy_c45_ids: device_ids[3]=0x3a1b662
[    5.630797] libphy: get_phy_c45_ids: device_ids[4]=0x3a1b662
[    5.654535] libphy: get_phy_c45_ids: device_ids[7]=0x3a1b662
[    5.791723] libphy: get_phy_c45_ids: device_ids[29]=0x3a1b662
[    5.804050] libphy: get_phy_c45_ids: device_ids[30]=0x3a1b662
[    5.816375] libphy: get_phy_c45_ids: device_ids[31]=0x0

[    7.690237] mscc_felix 0000:00:00.5: PHY [0.5:00] driver [Aquantia AQR412] (irq=POLL)
[    7.704739] mscc_felix 0000:00:00.5: PHY [0.5:01] driver [Aquantia AQR412] (irq=POLL)
[    7.718918] mscc_felix 0000:00:00.5: PHY [0.5:02] driver [Aquantia AQR412] (irq=POLL)
[    7.733044] mscc_felix 0000:00:00.5: PHY [0.5:03] driver [Aquantia AQR412] (irq=POLL)

Fixes: bba238ed037c ("net: phy: continue searching for C45 MMDs even if first returned ffff:ffff")
Reported-by: Colin King <colin.king@canonical.com>
Reported-by: Ioana Ciornei <ioana.ciornei@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/phy_device.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 49e98a092b96..1b9523595839 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -734,8 +734,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
 	/* Find first non-zero Devices In package. Device zero is reserved
 	 * for 802.3 c45 complied PHYs, so don't probe it at first.
 	 */
-	for (i = 1; i < MDIO_MMD_NUM && devs_in_pkg == 0 &&
-	     (devs_in_pkg & 0x1fffffff) == 0x1fffffff; i++) {
+	for (i = 1; i < MDIO_MMD_NUM && (devs_in_pkg == 0 ||
+	     (devs_in_pkg & 0x1fffffff) == 0x1fffffff); i++) {
 		if (i == MDIO_MMD_VEND1 || i == MDIO_MMD_VEND2) {
 			/* Check that there is a device present at this
 			 * address before reading the devices-in-package
-- 
2.25.1

Re: [PATCH net-next] net: phy: fix check in get_phy_c45_ids

[Vladimir Oltean]

On Mon, Jul 20, 2020 at 08:26:54PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> After the patch below, the iteration through the available MMDs is
> completely short-circuited, and devs_in_pkg remains set to the initial
> value of zero.
> 
> Due to devs_in_pkg being zero, the rest of get_phy_c45_ids() is
> short-circuited too: the following loop never reaches below this point
> either (it executes "continue" for every device in package, failing to
> retrieve PHY ID for any of them):
> 
> 	/* Now probe Device Identifiers for each device present. */
> 	for (i = 1; i < num_ids; i++) {
> 		if (!(devs_in_pkg & (1 << i)))
> 			continue;
> 
> So c45_ids->device_ids remains populated with zeroes. This causes an
> Aquantia AQR412 PHY (same as any C45 PHY would, in fact) to be probed by
> the Generic PHY driver.
> 
> The issue seems to be a case of submitting partially committed work (and
> therefore testing something other than was submitted).
> 
> The intention of the patch was to delay exiting the loop until one more
> condition is reached (the devs_in_pkg read from hardware is either 0, OR
> mostly f's). So fix the patch to reflect that.
> 
> Tested with traffic on a LS1028A-QDS, the PHY is now probed correctly
> using the Aquantia driver. The devs_in_pkg bit field is set to
> 0xe000009a, and the MMDs that are present have the following IDs:
> 
> [    5.600772] libphy: get_phy_c45_ids: device_ids[1]=0x3a1b662
> [    5.618781] libphy: get_phy_c45_ids: device_ids[3]=0x3a1b662
> [    5.630797] libphy: get_phy_c45_ids: device_ids[4]=0x3a1b662
> [    5.654535] libphy: get_phy_c45_ids: device_ids[7]=0x3a1b662
> [    5.791723] libphy: get_phy_c45_ids: device_ids[29]=0x3a1b662
> [    5.804050] libphy: get_phy_c45_ids: device_ids[30]=0x3a1b662
> [    5.816375] libphy: get_phy_c45_ids: device_ids[31]=0x0
> 
> [    7.690237] mscc_felix 0000:00:00.5: PHY [0.5:00] driver [Aquantia AQR412] (irq=POLL)
> [    7.704739] mscc_felix 0000:00:00.5: PHY [0.5:01] driver [Aquantia AQR412] (irq=POLL)
> [    7.718918] mscc_felix 0000:00:00.5: PHY [0.5:02] driver [Aquantia AQR412] (irq=POLL)
> [    7.733044] mscc_felix 0000:00:00.5: PHY [0.5:03] driver [Aquantia AQR412] (irq=POLL)
> 
> Fixes: bba238ed037c ("net: phy: continue searching for C45 MMDs even if first returned ffff:ffff")
> Reported-by: Colin King <colin.king@canonical.com>
> Reported-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---

This patch is repairing some pretty significant breakage. Could we
please get some review before there start appearing user reports?

[ sorry for the breakage ]

>  drivers/net/phy/phy_device.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 49e98a092b96..1b9523595839 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -734,8 +734,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr,
>  	/* Find first non-zero Devices In package. Device zero is reserved
>  	 * for 802.3 c45 complied PHYs, so don't probe it at first.
>  	 */
> -	for (i = 1; i < MDIO_MMD_NUM && devs_in_pkg == 0 &&
> -	     (devs_in_pkg & 0x1fffffff) == 0x1fffffff; i++) {
> +	for (i = 1; i < MDIO_MMD_NUM && (devs_in_pkg == 0 ||
> +	     (devs_in_pkg & 0x1fffffff) == 0x1fffffff); i++) {
>  		if (i == MDIO_MMD_VEND1 || i == MDIO_MMD_VEND2) {
>  			/* Check that there is a device present at this
>  			 * address before reading the devices-in-package
> -- 
> 2.25.1
> 

Thanks,
-Vladimir

Re: [PATCH net-next] net: phy: fix check in get_phy_c45_ids

[Andrew Lunn]

On Wed, Jul 22, 2020 at 02:52:09PM +0300, Vladimir Oltean wrote:
> On Mon, Jul 20, 2020 at 08:26:54PM +0300, Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > 
> > After the patch below, the iteration through the available MMDs is
> > completely short-circuited, and devs_in_pkg remains set to the initial
> > value of zero.
> > 
> > Due to devs_in_pkg being zero, the rest of get_phy_c45_ids() is
> > short-circuited too: the following loop never reaches below this point
> > either (it executes "continue" for every device in package, failing to
> > retrieve PHY ID for any of them):
> > 
> > 	/* Now probe Device Identifiers for each device present. */
> > 	for (i = 1; i < num_ids; i++) {
> > 		if (!(devs_in_pkg & (1 << i)))
> > 			continue;
> > 
> > So c45_ids->device_ids remains populated with zeroes. This causes an
> > Aquantia AQR412 PHY (same as any C45 PHY would, in fact) to be probed by
> > the Generic PHY driver.
> > 
> > The issue seems to be a case of submitting partially committed work (and
> > therefore testing something other than was submitted).
> > 
> > The intention of the patch was to delay exiting the loop until one more
> > condition is reached (the devs_in_pkg read from hardware is either 0, OR
> > mostly f's). So fix the patch to reflect that.
> > 
> > Tested with traffic on a LS1028A-QDS, the PHY is now probed correctly
> > using the Aquantia driver. The devs_in_pkg bit field is set to
> > 0xe000009a, and the MMDs that are present have the following IDs:
> > 
> > [    5.600772] libphy: get_phy_c45_ids: device_ids[1]=0x3a1b662
> > [    5.618781] libphy: get_phy_c45_ids: device_ids[3]=0x3a1b662
> > [    5.630797] libphy: get_phy_c45_ids: device_ids[4]=0x3a1b662
> > [    5.654535] libphy: get_phy_c45_ids: device_ids[7]=0x3a1b662
> > [    5.791723] libphy: get_phy_c45_ids: device_ids[29]=0x3a1b662
> > [    5.804050] libphy: get_phy_c45_ids: device_ids[30]=0x3a1b662
> > [    5.816375] libphy: get_phy_c45_ids: device_ids[31]=0x0
> > 
> > [    7.690237] mscc_felix 0000:00:00.5: PHY [0.5:00] driver [Aquantia AQR412] (irq=POLL)
> > [    7.704739] mscc_felix 0000:00:00.5: PHY [0.5:01] driver [Aquantia AQR412] (irq=POLL)
> > [    7.718918] mscc_felix 0000:00:00.5: PHY [0.5:02] driver [Aquantia AQR412] (irq=POLL)
> > [    7.733044] mscc_felix 0000:00:00.5: PHY [0.5:03] driver [Aquantia AQR412] (irq=POLL)
> > 
> > Fixes: bba238ed037c ("net: phy: continue searching for C45 MMDs even if first returned ffff:ffff")
> > Reported-by: Colin King <colin.king@canonical.com>
> > Reported-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> 
> This patch is repairing some pretty significant breakage. Could we
> please get some review before there start appearing user reports?
> 
> [ sorry for the breakage ]

I'm surprised it has not been merged, since the fix seems quite
obvious.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next] net: phy: fix check in get_phy_c45_ids

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Mon, 20 Jul 2020 20:26:54 +0300

> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> After the patch below, the iteration through the available MMDs is
> completely short-circuited, and devs_in_pkg remains set to the initial
> value of zero.
> 
> Due to devs_in_pkg being zero, the rest of get_phy_c45_ids() is
> short-circuited too: the following loop never reaches below this point
> either (it executes "continue" for every device in package, failing to
> retrieve PHY ID for any of them):
> 
> 	/* Now probe Device Identifiers for each device present. */
> 	for (i = 1; i < num_ids; i++) {
> 		if (!(devs_in_pkg & (1 << i)))
> 			continue;
> 
> So c45_ids->device_ids remains populated with zeroes. This causes an
> Aquantia AQR412 PHY (same as any C45 PHY would, in fact) to be probed by
> the Generic PHY driver.
> 
> The issue seems to be a case of submitting partially committed work (and
> therefore testing something other than was submitted).
> 
> The intention of the patch was to delay exiting the loop until one more
> condition is reached (the devs_in_pkg read from hardware is either 0, OR
> mostly f's). So fix the patch to reflect that.
> 
> Tested with traffic on a LS1028A-QDS, the PHY is now probed correctly
> using the Aquantia driver. The devs_in_pkg bit field is set to
> 0xe000009a, and the MMDs that are present have the following IDs:
 ...
> Fixes: bba238ed037c ("net: phy: continue searching for C45 MMDs even if first returned ffff:ffff")
> Reported-by: Colin King <colin.king@canonical.com>
> Reported-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Applied, thanks.

I waited on this because I wanted to get a review from someone, and I
try to always give Andrew/Florian/Heiner/etc. a day or two on core PHY
stuff so that they can have a chance to do a review.