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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 11 Apr 2012 19:31:59 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Stuart Hodgson <smhodgson@...arflare.com>
CC:	<netdev@...r.kernel.org>, <davem@...emloft.net>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 2/2] net: ethtool: Add capability to retrieve
 plug-in module EEPROM

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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html