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

[Andrew Lunn <andrew@lunn.ch>]

> >>+	/* 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().
> 
> Hi Andrew,
> 
> max_read_len is 0 when there is no quirk.
> I can write it a bit differently depending on the outcome of my other email.

Hi René

No, you misunderstood me.

> >>+		ret = sfp->read(sfp, a2, addr, buf, q->max_read_len);

Say max_read_len = 64

The SFP code asks to read 68 bytes. The first call to read() is going
to ask for 64 bytes. The second call is going to also ask for 64
bytes, writing 60 bytes passed the end of buf. Bad things then happen.

> 
> >>+		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.
> 
> Getting less bytes then asked is already an error I think.
> I could check the return size and directly return the number of bytes that I
> have. The callers are checking for size and they can retry if wanted. So that
> should not be an issue.

If that is true, why is rx_bytes += ret, where as all the others are
+= q->max_read_len. Please be consistent. The general pattern of a
read function in POSIX systems is that it returns how many bytes were
actually returned. So i would always use += ret.

> By reading the SSF spec we can write to a user writable EERPOM area of 120
> bytes.
> But the current code has only has 1 sfp_write for a byte value.
> 
> So for now I should say no.

So how about adding a WARN_ON. If the request is bigger than what the
quirk allows, make it very obvious we have an issue.

      Andrew