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

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

Message-ID: <20190124140323.Horde.-BXNAw3tfYO0-oHPatZeOOi@www.vdorst.com>
Date:   Thu, 24 Jan 2019 14:03:23 +0000
From:   René van Dorst <opensource@...rst.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] sfp: sfp_read: split-up request when hw rx buffer is
 too small.

Quoting Andrew Lunn <andrew@...n.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@...rst.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().

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.

>> +		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.

>> +	}
>> +
>> +	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?

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.

Greats,

René