* [RFC PATCH 2/2] net: ethtool: Add capability to retrieve plug-in module EEPROM
@ 2012-03-27 17:51 Stuart Hodgson
2012-04-02 18:18 ` Ben Hutchings
0 siblings, 1 reply; 5+ messages in thread
From: Stuart Hodgson @ 2012-03-27 17:51 UTC (permalink / raw)
To: netdev; +Cc: davem, linux-kernel, Ben Hutchings
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)
+ 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,
};
diff --git a/drivers/net/ethernet/sfc/mcdi_phy.c
b/drivers/net/ethernet/sfc/mcdi_phy.c
index 7bcad89..be8e372 100644
--- 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:
+ 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
+#define OFF_TO_BUFF(x) (x + MC_CMD_GET_PHY_MEDIA_INFO_OUT_DATA_OFST)
+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;
+ u8 *user_data = data;
+
+ if (!data || !ee)
+ return -EINVAL;
+
+ 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;
+
+ 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;
+
+ 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;
+}
+
+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;
+
+ phy_cfg = efx->phy_data;
+ modinfo->eeprom_len = mcdi_to_module_eeprom_len(phy_cfg->media);
+ modinfo->type = SFF_8079;
+
+ return 0;
+}
+
const struct efx_phy_operations efx_mcdi_phy_ops = {
.probe = efx_mcdi_phy_probe,
.init = efx_port_dummy_op_int,
@@ -751,4 +854,6 @@ const struct efx_phy_operations efx_mcdi_phy_ops = {
.test_alive = efx_mcdi_phy_test_alive,
.run_tests = efx_mcdi_phy_run_tests,
.test_name = efx_mcdi_phy_test_name,
+ .get_module_eeprom = efx_mcdi_phy_get_module_eeprom,
+ .get_module_info = efx_mcdi_phy_get_module_info,
};
diff --git a/drivers/net/ethernet/sfc/net_driver.h
b/drivers/net/ethernet/sfc/net_driver.h
index 0b95505..245fc42 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -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);
};
/**
--
1.7.7.6
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH 2/2] net: ethtool: Add capability to retrieve plug-in module EEPROM
2012-03-27 17:51 [RFC PATCH 2/2] net: ethtool: Add capability to retrieve plug-in module EEPROM Stuart Hodgson
@ 2012-04-02 18:18 ` Ben Hutchings
2012-04-11 17:41 ` Stuart Hodgson
0 siblings, 1 reply; 5+ messages in thread
From: Ben Hutchings @ 2012-04-02 18:18 UTC (permalink / raw)
To: Stuart Hodgson; +Cc: netdev, davem, linux-kernel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH 2/2] net: ethtool: Add capability to retrieve plug-in module EEPROM
2012-04-02 18:18 ` Ben Hutchings
@ 2012-04-11 17:41 ` Stuart Hodgson
2012-04-11 18:31 ` Ben Hutchings
0 siblings, 1 reply; 5+ messages in thread
From: Stuart Hodgson @ 2012-04-11 17:41 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, davem, linux-kernel
On 02/04/12 19:18, Ben Hutchings wrote:
> 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;
>
I am pretty sure that these two pieces of code are the same
>> + 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.
>
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH 2/2] net: ethtool: Add capability to retrieve plug-in module EEPROM
2012-04-11 17:41 ` Stuart Hodgson
@ 2012-04-11 18:31 ` Ben Hutchings
2012-04-12 9:20 ` Stuart Hodgson
0 siblings, 1 reply; 5+ messages in thread
From: Ben Hutchings @ 2012-04-11 18:31 UTC (permalink / raw)
To: Stuart Hodgson; +Cc: netdev, davem, linux-kernel
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.
[...]
> >> + 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.
--
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH 2/2] net: ethtool: Add capability to retrieve plug-in module EEPROM
2012-04-11 18:31 ` Ben Hutchings
@ 2012-04-12 9:20 ` Stuart Hodgson
0 siblings, 0 replies; 5+ messages in thread
From: Stuart Hodgson @ 2012-04-12 9:20 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, davem, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-04-12 9:20 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-27 17:51 [RFC PATCH 2/2] net: ethtool: Add capability to retrieve plug-in module EEPROM Stuart Hodgson
2012-04-02 18:18 ` Ben Hutchings
2012-04-11 17:41 ` Stuart Hodgson
2012-04-11 18:31 ` Ben Hutchings
2012-04-12 9:20 ` Stuart Hodgson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).