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

[Andrew Lunn]

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 <opensou...@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