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: <20190125215843.Horde.8SasDoteCejPU5IfJ0ibweV@www.vdorst.com>
Date:   Fri, 25 Jan 2019 21:58:43 +0000
From:   René van Dorst <opensource@...rst.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        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 René van Dorst <opensource@...rst.com>:

> Quoting Florian Fainelli <f.fainelli@...il.com>:
>
>> On 1/23/19 1:20 PM, 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);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +		rx_bytes += ret;
>>> +		addr += q->max_read_len;
>>> +		buf += q->max_read_len;
>>> +		len -= q->max_read_len;
>>> +	}
>>
>> sfp->read defaults to sfp_i2c_read() but it is possible to override that
>> by registering a custom sfp_bus instance (nothing does it today, but
>> that could obviously change), so there is no guarantee that
>> sfp->i2c->quirks is applicable unless sfp_i2c_read() is used.
>>
>> sfp_i2c_read() is presumably where the max_read_len splitting should
>> occur, or better yet, should not i2c_transfer() take care of that on its
>> own? That way there would be no layering violation of having to fetch
>> the quirk bitmask for the i2c adapter being used, that is something that
>> should belong in the core i2c framework.
>
> Yes it is better to put it in sfp_i2c_read().
>
> I think it is best to handle the split-up within the driver.
> The driver knows how to talk to the device and may apply device quirks.
>
> Also tda1004x [0] and TPM [1] driver also handles it within the  
> driver itself.
>
> TPM driver just try to send the want size and split-up request to
> I2C_SMBUS_BLOCK_MAX when a -EOPNOTSUPP returns, just retries it a number of
> times.
>
> I can do the same but I have to pick a minimum size.
> Looking in SSF-8472rev12.2.1 they don't limit the way you access the device.
> So use I2C_SMBUS_BLOCK_MAX of 32 bytes is sufficient or lookup the i2c
> quirk max_read_len is also an option.

Hi Florian,

I did a test in sfp_i2c_read() with reducing the read size when -EOPNOTSUPP
returns like TPM driver. But this always produces noise in kernel log about
the "msg too long" if the device is init at boot or when plugged-in.
Devices with SFP_EXT_STATUS generates also an 2nd error.
So I prefer to use the quirk max_read_len.

dmesg output reduce read size when -EOPNOTSUPP returns.
[  134.220000] i2c i2c-0: adapter quirk: msg too long (addr 0x0050,  
size 96, read)
[  135.250000] sfp sfp-lan5: module FiberStore       SFP-GB-GE-T       
rev B    sn <snip>      dc <snip>
[  141.150000] sfp sfp-lan5: module removed
[  150.950000] i2c i2c-0: adapter quirk: msg too long (addr 0x0050,  
size 96, read)
[  151.980000] sfp sfp-lan5: module FiberStore       SFP-GE-BX         
rev A0   sn <snip>      dc <snip>
[  151.990000] i2c i2c-0: adapter quirk: msg too long (addr 0x0051,  
size 92, read)

dmesg output with read size always reduced to quiek max_read_len.
[   27.350000] sfp sfp-lan5: module FiberStore       SFP-GE-BX         
rev A0   sn <snip>      dc <snip>
[   34.540000] sfp sfp-lan5: module removed
[   39.360000] sfp sfp-lan5: module FiberStore       SFP-GB-GE-T       
rev B    sn <snip>      dc <snip>

So shall I spin v2 with quirk max_read_len as max read size?

Greats,

René

> Grepping thru the i2c busses I see only 2 devices which has less  
> then 32 bytes
> of buffer. i2c-nvidia-gpu (Nvidia GPU) and i2c-pmcmsp  
> (microcontroller MSP71xx).
> Both are unlikely to be used for these kind of applications.
>
> I think I2C_SMBUS_BLOCK_MAX is safe to use.
>
> Greats,
>
> René
>
> [0]  
> https://elixir.bootlin.com/linux/v5.0-rc3/source/drivers/media/dvb-frontends/tda1004x.c#L319
> [1]  
> https://elixir.bootlin.com/linux/v5.0-rc3/source/drivers/char/tpm/tpm_i2c_infineon.c#L158