| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fd4ed3cf-381e-4895-8f43-04bcafa3eb6d@gmx.net>
Date: Sat, 14 Sep 2024 15:16:09 +0200
From: Hans-Frieder Vogt <hfdevel@....net>
To: Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>, Russell King
<linux@...linux.org.uk>, Bartosz Golaszewski
<bartosz.golaszewski@...aro.org>, Christian Marangi <ansuelsmth@...il.com>,
Clark Wang <xiaoning.wang@....com>, Jon Hunter <jonathanh@...dia.com>
Subject: Re: [PATCH net] net: phy: aquantia: fix -ETIMEDOUT PHY probe failure
when firmware not present
On 13.09.2024 14.12, Vladimir Oltean wrote:
> The author of the blamed commit apparently did not notice something
> about aqr_wait_reset_complete(): it polls the exact same register -
> MDIO_MMD_VEND1:VEND1_GLOBAL_FW_ID - as aqr_firmware_load().
>
> Thus, the entire logic after the introduction of aqr_wait_reset_complete() is
> now completely side-stepped, because if aqr_wait_reset_complete()
> succeeds, MDIO_MMD_VEND1:VEND1_GLOBAL_FW_ID could have only been a
> non-zero value. The handling of the case where the register reads as 0
> is dead code, due to the previous -ETIMEDOUT having stopped execution
> and returning a fatal error to the caller. We never attempt to load
> new firmware if no firmware is present.
>
> Based on static code analysis, I guess we should simply introduce a
> switch/case statement based on the return code from aqr_wait_reset_complete(),
> to determine whether to load firmware or not. I am not intending to
> change the procedure through which the driver determines whether to load
> firmware or not, as I am unaware of alternative possibilities.
>
> At the same time, Russell King suggests that if aqr_wait_reset_complete()
> is expected to return -ETIMEDOUT as part of normal operation and not
> just catastrophic failure, the use of phy_read_mmd_poll_timeout() is
> improper, since that has an embedded print inside. Just open-code a
> call to read_poll_timeout() to avoid printing -ETIMEDOUT, but continue
> printing actual read errors from the MDIO bus.
>
> Fixes: ad649a1fac37 ("net: phy: aquantia: wait for FW reset before checking the vendor ID")
> Reported-by: Clark Wang <xiaoning.wang@....com>
> Reported-by: Jon Hunter <jonathanh@...dia.com>
> Closes: https://lore.kernel.org/netdev/8ac00a45-ac61-41b4-9f74-d18157b8b6bf@nvidia.com/
> Reported-by: Hans-Frieder Vogt <hfdevel@....net>
> Closes: https://lore.kernel.org/netdev/c7c1a3ae-be97-4929-8d89-04c8aa870209@gmx.net/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> Only compile-tested. However, my timeout timer expired waiting for
> reactions on the thread with Bartosz' original patch, and Hans-Frieder
> Vogt wrote a message in his cover letter implying that the patch fixes
> the issue for him. Any Tested-by: tags are welcome.
>
> drivers/net/phy/aquantia/aquantia_firmware.c | 42 +++++++++++---------
> drivers/net/phy/aquantia/aquantia_main.c | 19 +++++++--
> 2 files changed, 39 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/net/phy/aquantia/aquantia_firmware.c b/drivers/net/phy/aquantia/aquantia_firmware.c
> index 524627a36c6f..dac6464b5fe2 100644
> --- a/drivers/net/phy/aquantia/aquantia_firmware.c
> +++ b/drivers/net/phy/aquantia/aquantia_firmware.c
> @@ -353,26 +353,32 @@ int aqr_firmware_load(struct phy_device *phydev)
> {
> int ret;
>
> - ret = aqr_wait_reset_complete(phydev);
> - if (ret)
> - return ret;
> -
> - /* Check if the firmware is not already loaded by pooling
> - * the current version returned by the PHY. If 0 is returned,
> - * no firmware is loaded.
> + /* Check if the firmware is not already loaded by polling
> + * the current version returned by the PHY.
> */
> - ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_FW_ID);
> - if (ret > 0)
> - goto exit;
> -
> - ret = aqr_firmware_load_nvmem(phydev);
> - if (!ret)
> - goto exit;
> -
> - ret = aqr_firmware_load_fs(phydev);
> - if (ret)
> + ret = aqr_wait_reset_complete(phydev);
> + switch (ret) {
> + case 0:
> + /* Some firmware is loaded => do nothing */
> + return 0;
> + case -ETIMEDOUT:
> + /* VEND1_GLOBAL_FW_ID still reads 0 after 2 seconds of polling.
> + * We don't have full confidence that no firmware is loaded (in
> + * theory it might just not have loaded yet), but we will
> + * assume that, and load a new image.
> + */
> + ret = aqr_firmware_load_nvmem(phydev);
> + if (!ret)
> + return ret;
> +
> + ret = aqr_firmware_load_fs(phydev);
> + if (ret)
> + return ret;
> + break;
> + default:
> + /* PHY read error, propagate it to the caller */
> return ret;
> + }
>
> -exit:
> return 0;
> }
> diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
> index e982e9ce44a5..57b8b8f400fd 100644
> --- a/drivers/net/phy/aquantia/aquantia_main.c
> +++ b/drivers/net/phy/aquantia/aquantia_main.c
> @@ -435,6 +435,9 @@ static int aqr107_set_tunable(struct phy_device *phydev,
> }
> }
>
> +#define AQR_FW_WAIT_SLEEP_US 20000
> +#define AQR_FW_WAIT_TIMEOUT_US 2000000
> +
> /* If we configure settings whilst firmware is still initializing the chip,
> * then these settings may be overwritten. Therefore make sure chip
> * initialization has completed. Use presence of the firmware ID as
> @@ -444,11 +447,19 @@ static int aqr107_set_tunable(struct phy_device *phydev,
> */
> int aqr_wait_reset_complete(struct phy_device *phydev)
> {
> - int val;
> + int ret, val;
> +
> + ret = read_poll_timeout(phy_read_mmd, val, val != 0,
> + AQR_FW_WAIT_SLEEP_US, AQR_FW_WAIT_TIMEOUT_US,
> + false, phydev, MDIO_MMD_VEND1,
> + VEND1_GLOBAL_FW_ID);
> + if (val < 0) {
> + phydev_err(phydev, "Failed to read VEND1_GLOBAL_FW_ID: %pe\n",
> + ERR_PTR(val));
> + return val;
> + }
>
> - return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1,
> - VEND1_GLOBAL_FW_ID, val, val != 0,
> - 20000, 2000000, false);
> + return ret;
> }
>
> static void aqr107_chip_info(struct phy_device *phydev)
Tested-by: Hans-Frieder Vogt <hfdevel@....net>
Hans
Powered by blists - more mailing lists