lkml.org 
[lkml]   [2012]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH 2/2] net: ethtool: Add capability to retrieve plug-in module EEPROM
    From
    Date
    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 <smhodgson@solarflare.com>
    > ---
    > 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.



    \
     
     \ /
      Last update: 2012-04-02 20:21    [W:0.033 / U:1.116 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site