net: ethtool: Add capability to retrieve plug-in module EEPROM

RFC PATCH In review Stuart Hodgson 2 participants 5 messages Mar 27, 2012 – Apr 12, 2012

Subsystems (3)

Networking Network drivers Sfc network driver

lore

Share & tools

lore

Stuart Hodgson

author

2012-03-27 17:51 UTC #0 lore raw

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

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 <[email protected]>

---
 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(-)

drivers/net/ethernet/sfc/ethtool.c +36 -0

index f22f45f..e77895f 100644
@@ -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)
+        return -EOPNOTSUPP;
+
+    mutex_lock(&efx->mac_lock);
+    ret = efx->phy_op->get_module_eeprom(efx, ee, data);
+    mutex_unlock(&efx->mac_lock);
+
+    return ret;
+}
+
+static int efx_ethtool_get_module_info(struct net_device *net_dev,
+                       struct ethtool_modinfo *modinfo)
+{
+    struct efx_nic *efx = netdev_priv(net_dev);
+    int ret;
+
+    if (!efx->phy_op || !efx->phy_op->get_module_info)
+        return 0;
+
+    mutex_lock(&efx->mac_lock);
+    ret = efx->phy_op->get_module_info(efx, modinfo);
+    mutex_unlock(&efx->mac_lock);
+
+    return ret;
+}
+
 const struct ethtool_ops efx_ethtool_ops = {
     .get_settings        = efx_ethtool_get_settings,
     .set_settings        = efx_ethtool_set_settings,

@@ -1137,4 +1171,6 @@ const struct ethtool_ops efx_ethtool_ops = {
     .get_rxfh_indir_size    = efx_ethtool_get_rxfh_indir_size,
     .get_rxfh_indir        = efx_ethtool_get_rxfh_indir,
     .set_rxfh_indir        = efx_ethtool_set_rxfh_indir,
+    .get_module_info    = efx_ethtool_get_module_info,
+    .get_module_eeprom    = efx_ethtool_get_module_eeprom,
 };

drivers/net/ethernet/sfc/mcdi_phy.c +105 -0

121 lines not on this page · raw message

drivers/net/ethernet/sfc/net_driver.h +5 -0

index 0b95505..245fc42 100644
@@ -524,6 +524,11 @@ struct efx_phy_operations {
     int (*test_alive) (struct efx_nic *efx);
     const char *(*test_name) (struct efx_nic *efx, unsigned int index);
     int (*run_tests) (struct efx_nic *efx, int *results, unsigned flags);
+    int (*get_module_eeprom) (struct efx_nic *efx,
+                   struct ethtool_eeprom *ee,
+                   u8 *data);
+    int (*get_module_info) (struct efx_nic *efx,
+                struct ethtool_modinfo *modinfo);
 };
 
 /**

Signature · 1 line

1.7.7.6

Ben Hutchings

2012-04-02 18:18 UTC #1 lore raw

Quoted 23 lines · Stuart Hodgson

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 <[email protected]>
---
  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(-)
drivers/net/ethernet/sfc/ethtool.c +0 -0
index f22f45f..e77895f 100644
@@ -1108,6 +1108,40 @@ static int efx_ethtool_set_rxfh_indir(struct 
 net_device *net_dev,
      return 0;
  }

drivers/net/ethernet/sfc/ethtool.c static int efx_ethtool_set_rxfh_indir(struct · line 1120

Quoted 10 lines

drivers/net/ethernet/sfc/ethtool.c +9 -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.

[...]

drivers/net/ethernet/sfc/mcdi_phy.c static u32 mcdi_to_ethtool_media(u32 media) · line 312

Quoted 12 lines

drivers/net/ethernet/sfc/mcdi_phy.c +6 -0

@@ -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?

Quoted 9 lines

drivers/net/ethernet/sfc/mcdi_phy.c +5 -0

+    default:
+        return 0;
+    }
+}
+
  static int efx_mcdi_phy_probe(struct efx_nic *efx)
  {
      struct efx_mcdi_phy_data *phy_data;

drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct · line 755

Quoted 6 lines

drivers/net/ethernet/sfc/mcdi_phy.c +2 -0

@@ -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?

drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 755

Quoted 1 line

drivers/net/ethernet/sfc/mcdi_phy.c +1 -0
+#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.

drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 768

Quoted 13 lines

drivers/net/ethernet/sfc/mcdi_phy.c +13 -0

+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.

drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 772

Quoted 4 lines

drivers/net/ethernet/sfc/mcdi_phy.c +4 -0

+    u8 *user_data = data;
+
+    if (!data || !ee)
+        return -EINVAL;

Neither of these is allowed to be NULL; don't bother checking.

drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 780

Quoted 7 lines

drivers/net/ethernet/sfc/mcdi_phy.c +7 -0

+    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?

Quoted 16 lines

drivers/net/ethernet/sfc/mcdi_phy.c +16 -0

+    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;

drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 807

Quoted 10 lines

drivers/net/ethernet/sfc/mcdi_phy.c +10 -0

+            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;

Quoted 6 lines

drivers/net/ethernet/sfc/mcdi_phy.c +6 -0

+        memcpy(user_data,
+               (outbuf + OFF_TO_BUFF(page_off)),
+               to_copy);
+
+        space_remaining -= to_copy;
+        user_data += to_copy;

drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 824

Quoted 10 lines

drivers/net/ethernet/sfc/mcdi_phy.c +10 -0

+        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.

drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 836

Quoted 12 lines

drivers/net/ethernet/sfc/mcdi_phy.c +12 -0

+}
+
+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.)

drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 840

Quoted 3 lines

drivers/net/ethernet/sfc/mcdi_phy.c +3 -0

+    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.

Signature · 3 lines

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.

Stuart Hodgson

author

2012-04-11 17:41 UTC #2 lore raw

Quoted 139 lines · Ben Hutchings

Quoted 23 lines · Stuart Hodgson
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<[email protected]>
---
   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(-)
drivers/net/ethernet/sfc/ethtool.c +0 -0
index f22f45f..e77895f 100644
@@ -1108,6 +1108,40 @@ static int efx_ethtool_set_rxfh_indir(struct
 net_device *net_dev,
       return 0;
   }
drivers/net/ethernet/sfc/ethtool.c static int efx_ethtool_set_rxfh_indir(struct · line 1120
Quoted 10 lines
drivers/net/ethernet/sfc/ethtool.c +9 -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.
[...]
drivers/net/ethernet/sfc/mcdi_phy.c static u32 mcdi_to_ethtool_media(u32 media) · line 312
Quoted 12 lines
drivers/net/ethernet/sfc/mcdi_phy.c +6 -0
@@ -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?
Quoted 9 lines
drivers/net/ethernet/sfc/mcdi_phy.c +5 -0
+    default:
+        return 0;
+    }
+}
+
   static int efx_mcdi_phy_probe(struct efx_nic *efx)
   {
       struct efx_mcdi_phy_data *phy_data;
drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct · line 755
Quoted 6 lines
drivers/net/ethernet/sfc/mcdi_phy.c +2 -0
@@ -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?
drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 755
Quoted 1 line
drivers/net/ethernet/sfc/mcdi_phy.c +1 -0
+#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.
drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 768
Quoted 13 lines
drivers/net/ethernet/sfc/mcdi_phy.c +13 -0
+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.
drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 772
Quoted 4 lines
drivers/net/ethernet/sfc/mcdi_phy.c +4 -0
+    u8 *user_data = data;
+
+    if (!data || !ee)
+        return -EINVAL;
Neither of these is allowed to be NULL; don't bother checking.
drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx,
Quoted 7 lines
drivers/net/ethernet/sfc/mcdi_phy.c +7 -0
+    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?
Quoted 16 lines
drivers/net/ethernet/sfc/mcdi_phy.c +16 -0
+    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;
drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 807
Quoted 10 lines
drivers/net/ethernet/sfc/mcdi_phy.c +10 -0
+            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:

Quoted 4 lines

	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

Quoted 38 lines

Quoted 6 lines
drivers/net/ethernet/sfc/mcdi_phy.c +6 -0
+        memcpy(user_data,
+               (outbuf + OFF_TO_BUFF(page_off)),
+               to_copy);
+
+        space_remaining -= to_copy;
+        user_data += to_copy;
drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 824
Quoted 10 lines
drivers/net/ethernet/sfc/mcdi_phy.c +10 -0
+        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.
drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 836
Quoted 12 lines
drivers/net/ethernet/sfc/mcdi_phy.c +12 -0
+}
+
+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.)
drivers/net/ethernet/sfc/mcdi_phy.c static const char *efx_mcdi_phy_test_name(struct efx_nic *efx, · line 840
Quoted 3 lines
drivers/net/ethernet/sfc/mcdi_phy.c +3 -0
+    phy_cfg = efx->phy_data;
+    modinfo->eeprom_len = mcdi_to_module_eeprom_len(phy_cfg->media);
+    modinfo->type = SFF_8079;

Quoted 7 lines

[...]
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?

Stu

Ben Hutchings

2012-04-11 18:31 UTC #3 lore raw

Quoted 2 lines · Stuart Hodgson

Quoted 1 line · Ben Hutchings
On Tue, 2012-03-27 at 18:51 +0100, Stuart Hodgson wrote:

[...]

Quoted 2 lines

Quoted 2 lines
Quoted 2 lines
drivers/net/ethernet/sfc/ethtool.c +0 -0

[...]

Quoted 12 lines

Quoted 12 lines
drivers/net/ethernet/sfc/ethtool.c
Quoted 7 lines
drivers/net/ethernet/sfc/ethtool.c +7 -0
+        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:

Quoted 6 lines

Quoted 4 lines
	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.

[...]

Quoted 5 lines

Quoted 5 lines
Quoted 3 lines
drivers/net/ethernet/sfc/ethtool.c +3 -0
+    phy_cfg = efx->phy_data;
+    modinfo->eeprom_len = mcdi_to_module_eeprom_len(phy_cfg->media);
+    modinfo->type = SFF_8079;
[...]

Quoted 8 lines

Quoted 5 lines
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.

Signature · 3 lines

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.

Stuart Hodgson

author

2012-04-12 09:20 UTC #4 lore raw

Quoted 26 lines · Ben Hutchings

Quoted 2 lines · Stuart Hodgson
Quoted 1 line · Ben Hutchings
On Tue, 2012-03-27 at 18:51 +0100, Stuart Hodgson wrote:
[...]
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/net/ethernet/sfc/ethtool.c +0 -0
[...]
Quoted 12 lines
Quoted 12 lines
drivers/net/ethernet/sfc/ethtool.c
Quoted 7 lines
drivers/net/ethernet/sfc/ethtool.c +7 -0
+        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:
Quoted 6 lines
Quoted 4 lines
	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

Quoted 7 lines

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.

Quoted 14 lines

Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
drivers/net/ethernet/sfc/ethtool.c +3 -0
+    phy_cfg = efx->phy_data;
+    modinfo->eeprom_len = mcdi_to_module_eeprom_len(phy_cfg->media);
+    modinfo->type = SFF_8079;
[...]
Quoted 8 lines
Quoted 5 lines
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?

Quoted 7 lines

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