netdev - Re: [PATCH net v2] net: dsa: lan9303: ensure chip reset and wait for READY status

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241003211524.ugrkjjc7legax2ak@skbuf>
Date: Fri, 4 Oct 2024 00:15:24 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "A. Sverdlin" <alexander.sverdlin@...mens.com>
Cc: netdev@...r.kernel.org, Anatolij Gustschin <agust@...x.de>,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net v2] net: dsa: lan9303: ensure chip reset and wait for
 READY status

On Wed, Oct 02, 2024 at 07:12:28PM +0200, A. Sverdlin wrote:
> @@ -866,6 +869,29 @@ static int lan9303_check_device(struct lan9303 *chip)
>  	int ret;
>  	u32 reg;
>  
> +	/*
> +	 * In I2C-managed configurations this polling loop will clash with

netdev coding style is with comments like this: /* In I2C managed configurations...

> +	 * switch's reading of EEPROM right after reset and this behaviour is
> +	 * not configurable. While lan9303_read() already has quite long retry
> +	 * timeout, seems not all cases are being detected as arbitration error.

These arbitration errors happen only after reset? So in theory, after
this patch, we could remove the for() loop from lan9303_read()?

> +	 *
> +	 * According to datasheet, EEPROM loader has 30ms timeout (in case of
> +	 * missing EEPROM).
> +	 *
> +	 * Loading of the largest supported EEPROM is expected to take at least
> +	 * 5.9s.
> +	 */
> +	if (read_poll_timeout(lan9303_read, ret, reg & LAN9303_HW_CFG_READY,

Isn't "reg" uninitialized if "ret" is non-zero? So shouldn't be "ret"
also part of the break condition somehow?

> +			      20000, 6000000, false,
> +			      chip->regmap, LAN9303_HW_CFG, &reg)) {
> +		dev_err(chip->dev, "HW_CFG not ready: 0x%08x\n", reg);
> +		return -ENODEV;

What point is there to mangle the return code from read_poll_timeout()
(-ETIMEDOUT) into -ENODEV, instead of just propagating that?

> +	}
> +	if (ret) {
> +		dev_err(chip->dev, "failed to read HW_CFG reg: %d\n", ret);

%pe, ERR_PTR(ret) is nicer for the average, non-expert in errno.h user.
I see this driver isn't using it, so maybe there's an argument about
consistency, but there's a beginning for everything..

> +		return ret;
> +	}
> +
>  	ret = lan9303_read(chip->regmap, LAN9303_CHIP_REV, &reg);
>  	if (ret) {
>  		dev_err(chip->dev, "failed to read chip revision register: %d\n",
> -- 
> 2.46.2
>