On Mon, Jul 20, 2020 at 08:26:54PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <[email protected]>
>
> 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 <[email protected]>
> Reported-by: Ioana Ciornei <[email protected]>
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
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
