Re: [RFC PATCH 2/2] net: ethtool: Add capability to retrieve plug-inmodule EEPROM

[Stuart Hodgson]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/