Message ID | 20200720172654.1193241-1-olteanv@gmail.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | [net-next] net: phy: fix check in get_phy_c45_ids | expand |
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
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
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.
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