linux-kernel - Re: [PATCH] net: dsa: mv88e6xxx: Single chip mode detection for MV88E6*41

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] [day] [month] [year] [list]

Message-ID: <YmicmQEGbAvITaNm@lunn.ch>
Date:   Wed, 27 Apr 2022 03:30:01 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Nathan Rossi <nathan@...hanrossi.com>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH] net: dsa: mv88e6xxx: Single chip mode detection for
 MV88E6*41

On Sun, Apr 24, 2022 at 12:54:51PM +0000, Nathan Rossi wrote:
> The mv88e6xxx driver expects switches that are configured in single chip
> addressing mode to have the MDIO address configured as 0. This is due to
> the switch ADDR pins representing the single chip addressing mode as 0.
> However depending on the device (e.g. MV88E6*41) the switch does not
> respond on address 0 or any other address below 16 (the first port
> address) in single chip addressing mode. This allows for other devices
> to be on the same shared MDIO bus despite the switch being in single
> chip addressing mode.
> 
> When using a switch that works this way it is not possible to configure
> switch driver as single chip addressing via device tree, along with
> another MDIO device on the same bus with address 0, as both devices
> would have the same address of 0 resulting in mdiobus_register_device
> -EBUSY errors for one of the devices with address 0.
> 
> In order to support this configuration the switch node can have its MDIO
> address configured as 16 (the first address that the device responds
> to). During initialization the driver will treat this address similar to
> how address 0 is, however because this address is also a valid
> multi-chip address (in certain switch models, but not all) the driver
> will configure the SMI in single chip addressing mode and attempt to
> detect the switch model. If the device is configured in single chip
> addressing mode this will succeed and the initialization process can
> continue. If it fails to detect a valid model this is because the switch
> model register is not a valid register when in multi-chip mode, it will
> then fall back to the existing SMI initialization process using the MDIO
> address as the multi-chip mode address.
> 
> This detection method is safe if the device is in either mode because
> the single chip addressing mode read is a direct SMI/MDIO read operation
> and has no side effects compared to the SMI writes required for the
> multi-chip addressing mode.

Thanks for rewording the commit message. This makes it a lot clearer
what is going on and how it is fixed.

> @@ -6971,9 +6993,18 @@ static int mv88e6xxx_probe(struct mdio_device *mdiodev)
>  	if (chip->reset)
>  		usleep_range(1000, 2000);
>  
> -	err = mv88e6xxx_detect(chip);
> -	if (err)
> -		goto out;
> +	/* Detect if the device is configured in single chip addressing mode,
> +	 * otherwise continue with address specific smi init/detection.
> +	 */
> +	if (mv88e6xxx_single_chip_detect(chip, mdiodev)) {
> +		err = mv88e6xxx_smi_init(chip, mdiodev->bus, mdiodev->addr);
> +		if (err)
> +			goto out;
> +

This is confusing. Then name mv88e6xxx_single_chip_detect() suggests
it will return true if it detects a single chip device. When it fact
is return 0 == False if it does find such a device.

So i think this would be better coded as

	err = mv88e6xxx_single_chip_detect(chip, mdiodev);
	if (err) {
		err = mv88e6xxx_smi_init(chip, mdiodev->bus, mdiodev->addr);
		if (err)
			goto out;

I did however test this code on my 370rd, and it does work. So once we
get this sorted out, it is good to go.

	Andrew