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

[Stuart Hodgson <smhodgson@solarflare.com>]

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