Re: [PATCH net-next v2 1/5] net: phy: mdio-i2c: support I2C MDIO protocol for RollBall SFP modules

[Russell King - ARM Linux admin <linux@armlinux.org.uk>]

On Thu, Oct 29, 2020 at 11:25:05PM +0100, Marek Behún wrote:
> @@ -91,9 +94,210 @@ static int i2c_mii_write(struct mii_bus *bus, int phy_id, int reg, u16 val)
>  	return ret < 0 ? ret : 0;
>  }
>  
> -struct mii_bus *mdio_i2c_alloc(struct device *parent, struct i2c_adapter *i2c)
> +/* RollBall SFPs do not access internal PHY via I2C address 0x56, but
> + * instead via address 0x51, when SFP page is set to 0x03 and password to
> + * 0xffffffff:
> + *
> + * address  size  contents  description
> + * -------  ----  --------  -----------
> + * 0x80     1     CMD       0x01/0x02/0x04 for write/read/done
> + * 0x81     1     DEV       Clause 45 device
> + * 0x82     2     REG       Clause 45 register
> + * 0x84     2     VAL       Register value
> + */
...
> +static int i2c_mii_init_rollball(struct i2c_adapter *i2c)
> +{
> +	u8 page_buf[2], pw_buf[5];
> +	struct i2c_msg msgs[2];
> +	int ret;
> +
> +	page_buf[0] = SFP_PAGE;
> +	page_buf[1] = 3;
> +
> +	msgs[0].addr = ROLLBALL_PHY_I2C_ADDR;
> +	msgs[0].flags = 0;
> +	msgs[0].len = sizeof(page_buf);
> +	msgs[0].buf = page_buf;
> +
> +	pw_buf[0] = ROLLBALL_SFP_PASSWORD_ADDR;
> +	pw_buf[1] = 0xff;
> +	pw_buf[2] = 0xff;
> +	pw_buf[3] = 0xff;
> +	pw_buf[4] = 0xff;
> +
> +	msgs[1].addr = ROLLBALL_PHY_I2C_ADDR;
> +	msgs[1].flags = 0;
> +	msgs[1].len = sizeof(pw_buf);
> +	msgs[1].buf = pw_buf;
> +
> +	ret = i2c_transfer(i2c, msgs, ARRAY_SIZE(msgs));
> +	if (ret < 0)
> +		return ret;
> +	else if (ret != ARRAY_SIZE(msgs))
> +		return -EIO;
> +
> +	return 0;
> +}

One of the points I raised in the previous review was: "Also, shouldn't
we ensure that we are on page 1 before attempting any access?" I
actually meant page 3 which I corrected when commenting on patch 5:
"I think this needs to be done in the MDIO driver - if we have userspace
or otherwise expand what we're doing, relying on page 3 remaining
selected will be very fragile."

I feel that point still stands; if the SFP page is changed after
i2c_mii_init_rollball() and before a subsequent access, the subsequent
access is not going to hit the correct page, and we will lose access
to the PHY. Worse, we will end up writing to that other page.

That said, I don't see anything in SFF8472 that would result in a
clash, so I think I would like to see the comment at the beginning
make explicit the expectation that SFP_PAGE will remain set to 3
throughout the lifetime of the module plugged into the system.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!