Messages in this thread | | | Date | Thu, 12 Apr 2012 10:20:49 +0100 | From | Stuart Hodgson <> | Subject | Re: [RFC PATCH 2/2] net: ethtool: Add capability to retrieve plug-in module EEPROM |
| |
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
| |