Re: [PATCH] sfp: sfp_read: split-up request when hw rx buffer is too small.

[Andrew Lunn <andrew@lunn.ch>]

On Wed, Jan 23, 2019 at 10:20:46PM +0100, René van Dorst wrote:
> Without this patch sfp code retries to read the full struct sfp_eeprom_id
> id out of the SFP eeprom. Sizeof(id) is 96 bytes.
> My i2c hardware, Mediatek mt7621, has a rx buffer of 64 bytes.
> So sfp_read gets -NOSUPPORTED back on his turn return -EAGAIN.
> Same issue is with the SFP_EXT_STATUS data which is 92 bytes.
> 
> By split-up the request in multiple smaller requests with a max size of i2c
> max_read_len, we can readout the SFP module successfully.
> 
> Tested with MT7621 and two Fiberstore modules SFP-GB-GE-T and SFP-GE-BX.
> 
> Signed-off-by: René van Dorst <opensource@vdorst.com>
> ---
>  drivers/net/phy/sfp.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index fd8bb998ae52..1352a19571cd 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -367,7 +367,28 @@ static void sfp_set_state(struct sfp *sfp, unsigned int state)
>  
>  static int sfp_read(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)
>  {
> -	return sfp->read(sfp, a2, addr, buf, len);
> +	const struct i2c_adapter_quirks *q = sfp->i2c->quirks;
> +	int ret;
> +	size_t rx_bytes = 0;
> +
> +	/* Many i2c hw have limited rx buffers, split-up request when needed. */
> +	while ((q->max_read_len) && (len > q->max_read_len)) {
> +		ret = sfp->read(sfp, a2, addr, buf, q->max_read_len);

Hi René

I think you want to pass MIN(len, q->max_read_len) to read().

> +		if (ret < 0)
> +			return ret;
> +		rx_bytes += ret;
> +		addr += q->max_read_len;
> +		buf += q->max_read_len;
> +		len -= q->max_read_len;

I would prefer you add ret, not q->max_read_len. There is a danger it
returned less bytes than you asked for.

> +	}
> +
> +	ret = sfp->read(sfp, a2, addr, buf, len);
> +	if (ret < 0)
> +		return ret;
> +
> +	rx_bytes += ret;
> +
> +	return rx_bytes;
>  }
>  
>  static int sfp_write(struct sfp *sfp, bool a2, u8 addr, void *buf, size_t len)

Doesn't write need the same handling?

Thanks
	Andrew