Messages in this thread | | | Subject | Re: [PATCH net-next 3/3] Add capability to retrieve plug-in module EEPROM | From | Ben Hutchings <> | Date | Tue, 24 Apr 2012 18:43:12 +0100 |
| |
On Thu, 2012-04-19 at 16:39 +0100, Stuart Hodgson wrote: > Currently allows for SFP+ eeprom to be returned using the ethtool API. > This can be extended in future to handle different eeprom formats > and sizes [...] > --- a/drivers/net/ethernet/sfc/mcdi_phy.c > +++ b/drivers/net/ethernet/sfc/mcdi_phy.c > @@ -304,6 +304,26 @@ static u32 mcdi_to_ethtool_media(u32 media) > } > } > > +static u32 mcdi_to_module_eeprom_len(u32 media) > +{ > + switch (media) { > + case MC_CMD_MEDIA_SFP_PLUS: > + return ETH_MODULE_SFF_8079_LEN; > + default: > + return 0; > + } > +} > + > +static u32 mcdi_to_module_eeprom_type(u32 media) > +{ > + switch (media) { > + case MC_CMD_MEDIA_SFP_PLUS: > + return ETH_MODULE_SFF_8079; > + default: > + return 0; > + } > +}
These functions ended up unused, so don't add them. You should have got a compiler warning about this...
> static int efx_mcdi_phy_probe(struct efx_nic *efx) > { > struct efx_mcdi_phy_data *phy_data; > @@ -739,6 +759,95 @@ static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, > return NULL; > } > > +#define SFP_PAGE_SIZE 128 > +#define SFP_NUM_PAGES 2 > +static int efx_mcdi_phy_get_module_eeprom(struct efx_nic *efx, > + struct ethtool_eeprom *ee, u8 *data) > +{ > + u8 outbuf[MC_CMD_GET_PHY_MEDIA_INFO_OUT_LENMAX]; > + u8 inbuf[MC_CMD_GET_PHY_MEDIA_INFO_IN_LEN]; > + size_t outlen; > + int rc; > + unsigned int payload_len; > + unsigned int copied = 0; > + unsigned int space_remaining = ee->len; > + unsigned int page; > + unsigned int page_off; > + unsigned int to_copy; > + u8 *user_data = data; > + > + if (ee->offset > (SFP_PAGE_SIZE * SFP_NUM_PAGES)) > + return -EINVAL;
Please also add this sanity-check:
BUILD_BUG_ON(SFP_PAGE_SIZE * SFP_NUM_PAGES != ETH_MODULE_SFF_8079_LEN);
> + page_off = (ee->offset % SFP_PAGE_SIZE); > + page = (ee->offset > SFP_PAGE_SIZE) ? 1 : 0;
Off-by-one error; the comparison should be >=. Why not use the straightforward calculation:
page = ee->offset / SFP_PAGE_SIZE;
[...] > +static int efx_mcdi_phy_get_module_info(struct efx_nic *efx, > + struct ethtool_modinfo *modinfo) > +{ > + /* This will return a length of the eeprom > + * type of the module that was detected during the probe, > + * if not modules inserted then phy_data will be NULL */ > + 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:
The default case should return -EOPNOTSUPP, just as if we didn't implement this operation at all. And there's no need to set modinfo->{type,eeprom_len} to 0 in that case.
Ben.
> + break; > + } > + > + return 0; > +} [...]
-- Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer; that's the marketing department's job. They asked us to note that Solarflare product names are trademarked.
| |