Re: [RFC PATCH 2/2] net: ethtool: Add capability to retrieve plug-in module EEPROM

Stuart Hodgson
2012-04-12T09:20:53Z

On 11/04/12 19:31, Ben Hutchings wrote:
> On Wed, 2012-04-11 at 18:41 +0100, Stuart Hodgson wrote:
>> On 02/04/12 19:18, Ben Hutchings wrote:
>>> On Tue, 2012-03-27 at 18:51 +0100, Stuart Hodgson wrote:
> [...]
>>>> --- a/drivers/net/ethernet/sfc/ethtool.c
>>>> +++ b/drivers/net/ethernet/sfc/ethtool.c
> [...]
>>>> + payload_len = MCDI_DWORD(outbuf,
>>>> + GET_PHY_MEDIA_INFO_OUT_DATALEN);
>>>> +
>>>> + to_copy = (space_remaining< payload_len) ?
>>>> + space_remaining : payload_len;
>>>> +
>>>> + to_copy -= page_off;
>>>
>>> page_off is the number of bytes we need to discard from payload_len, but
>>> we don't want do discard that from space_remaining. I think the last
>>> two statements should be changed to:
>>>
>>> payload_len -= page_off;
>>> to_copy = (space_remaining< payload_len) ?
>>> space_remaining : payload_len;
>>>
>>
>> I am pretty sure that these two pieces of code are the same
>
> Suppose we start with ee->offset = 64, ee->len = 32. After the MCDI
> request returns and we assign payload_len from that, I believe we will
> have page_off = 64, payload_len = 128, space_remaining = 32.
>
> Work through the calculations from there and you'll see the problem.
>
> [...]

Yep, I go negative if len is shorter than offset.

>>>> + phy_cfg = efx->phy_data;
>>>> + modinfo->eeprom_len = mcdi_to_module_eeprom_len(phy_cfg->media);
>>>> + modinfo->type = SFF_8079;
>>> [...]
>>>
>>> I don't think this makes sense. If we're fixing the type as SFF_8079
>>> then why are we calling a function to get the length?
>>>
>>> Ben.
>>>
>>
>> What about adding an mcdi_to_module_eeprom_type in the same manner
>> as mcdi_to_module_eeprom_len and the other mapping functions?
>
> Could do, but I don't see the point of breaking this out into separate
> functions that are only used once. It's actually going to increase code
> duplication because you'll have to put the same PHY type checks in both
> of them.
>
> Ben.
>

I simply followed the precedent of mcdi_to_ethtool_media. I could
combine these functions into the module_info function such as

...
struct efx_mcdi_phy_data *phy_cfg;

phy_cfg = efx->phy_data;
modinfo->eeprom_len = 0;
modinfo->type = 0;

switch (phy_cfg->media) {
case MC_CMD_MEDIA_SFP_PLUS:
modinfo->type = ETH_MODULE_SFF_8079;
modinfo->eeprom_len = ETH_MODULE_SFF_8079_LEN;
break;
default:
break;
}
...

Stu