From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754091Ab2DBSSX (ORCPT ); Mon, 2 Apr 2012 14:18:23 -0400 Received: from exchange.solarflare.com ([216.237.3.220]:53720 "EHLO ocex02.SolarFlarecom.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1753885Ab2DBSSW (ORCPT ); Mon, 2 Apr 2012 14:18:22 -0400 Message-ID: <1333390698.2623.47.camel@bwh-desktop.uk.solarflarecom.com> Subject: Re: [RFC PATCH 2/2] net: ethtool: Add capability to retrieve plug-in module EEPROM From: Ben Hutchings To: Stuart Hodgson CC: , , Date: Mon, 2 Apr 2012 19:18:18 +0100 In-Reply-To: <4F71FE15.7060708@solarflare.com> References: <4F71FE15.7060708@solarflare.com> Organization: Solarflare Communications Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3 (3.2.3-1.fc16) Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-Originating-IP: [10.17.20.137] X-TM-AS-Product-Ver: SMEX-10.0.0.1412-6.800.1017-18812.005 X-TM-AS-Result: No--12.118200-0.000000-31 X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2012-03-27 at 18:51 +0100, Stuart Hodgson wrote: > Implementation in sfc driver to return the plugin module eeprom > > 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. > > Signed-off-by: Stuart Hodgson > --- > drivers/net/ethernet/sfc/ethtool.c | 36 +++++++++++ > drivers/net/ethernet/sfc/mcdi_phy.c | 105 > +++++++++++++++++++++++++++++++++ > drivers/net/ethernet/sfc/net_driver.h | 5 ++ > 3 files changed, 146 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/ethtool.c > b/drivers/net/ethernet/sfc/ethtool.c > index f22f45f..e77895f 100644 > --- a/drivers/net/ethernet/sfc/ethtool.c > +++ b/drivers/net/ethernet/sfc/ethtool.c > @@ -1108,6 +1108,40 @@ static int efx_ethtool_set_rxfh_indir(struct > net_device *net_dev, > return 0; > } > > +static int efx_ethtool_get_module_eeprom(struct net_device *net_dev, > + struct ethtool_eeprom *ee, > + u8 *data) > +{ > + struct efx_nic *efx = netdev_priv(net_dev); > + int ret; > + > + if (!efx->phy_op || > + !efx->phy_op->get_module_eeprom) No need to break that line. [...] > --- a/drivers/net/ethernet/sfc/mcdi_phy.c > +++ b/drivers/net/ethernet/sfc/mcdi_phy.c > @@ -304,6 +304,17 @@ 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 SFF_8079_LEN; > + case MC_CMD_MEDIA_XFP: Why split out XFP if we're not going to treat it any differently? > + default: > + return 0; > + } > +} > + > static int efx_mcdi_phy_probe(struct efx_nic *efx) > { > struct efx_mcdi_phy_data *phy_data; > @@ -739,6 +750,98 @@ static const char *efx_mcdi_phy_test_name(struct > efx_nic *efx, > return NULL; > } > > +#define SFP_PAGE_SIZE 128 > +#define NUM_PAGES 2 SFP_NUM_PAGES? > +#define OFF_TO_BUFF(x) (x + MC_CMD_GET_PHY_MEDIA_INFO_OUT_DATA_OFST) This is not really worth defining for just the one use. > +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; > + int payload_len; > + int copied = 0; > + int space_remaining = ee->len; > + int page; > + int page_off; > + int to_copy; None of payload_len..to_copy should be signed. > + u8 *user_data = data; > + > + if (!data || !ee) > + return -EINVAL; Neither of these is allowed to be NULL; don't bother checking. > + if (ee->offset > (SFP_PAGE_SIZE * NUM_PAGES)) { > + rc = -EINVAL; > + goto fail; > + } > + > + page_off = (ee->offset % SFP_PAGE_SIZE); > + page = (ee->offset > SFP_PAGE_SIZE) ? 1 : 0; Why not ee->offset / SFP_PAGE_SIZE? > + while (space_remaining && (page < NUM_PAGES)) { > + > + MCDI_SET_DWORD(inbuf, GET_PHY_MEDIA_INFO_IN_PAGE, page); > + > + rc = efx_mcdi_rpc(efx, MC_CMD_GET_PHY_MEDIA_INFO, > + inbuf, sizeof(inbuf), > + outbuf, sizeof(outbuf), > + &outlen); > + > + if (rc) > + goto fail; > + > + /* Copy as much as we can into data */ > + if (outlen < MC_CMD_GET_PHY_MEDIA_INFO_OUT_LENMIN || > + outlen > MC_CMD_GET_PHY_MEDIA_INFO_OUT_LENMAX) { > + rc = -EIO; > + goto fail; > + } > + > + 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; > + memcpy(user_data, > + (outbuf + OFF_TO_BUFF(page_off)), > + to_copy); > + > + space_remaining -= to_copy; > + user_data += to_copy; > + copied += to_copy; > + page_off = 0; > + page++; > + } > + > + ee->len = copied; > + > + return 0; > +fail: > + return rc; Nothing to clean up here, so you might as well return errors directly. > +} > + > +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; > + > + if (!efx || !efx->phy_data) > + return -EOPNOTSUPP; efx certainly can't be NULL here and I don't believe efx->phy_data can either. (None of the other MCDI PHY operations check it.) > + 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. -- 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.