netdev - RE: [net-next 11/11 v2] ixgbe: allow eeprom writes via ethtool

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] [day] [month] [year] [list]

Message-ID: <EA929A9653AAE14F841771FB1DE5A136605A8FCB18@rrsmsx501.amr.corp.intel.com>
Date:	Mon, 3 Oct 2011 11:00:10 -0600
From:	"Tantilov, Emil S" <emil.s.tantilov@...el.com>
To:	Francois Romieu <romieu@...zoreil.com>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
CC:	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>
Subject: RE: [net-next 11/11 v2] ixgbe: allow eeprom writes via ethtool

>-----Original Message-----
>From: Francois Romieu [mailto:romieu@...zoreil.com]
>Sent: Saturday, October 01, 2011 2:31 AM
>To: Kirsher, Jeffrey T
>Cc: davem@...emloft.net; Tantilov, Emil S; netdev@...r.kernel.org;
>gospo@...hat.com
>Subject: Re: [net-next 11/11 v2] ixgbe: allow eeprom writes via ethtool
>
>Jeff Kirsher <jeffrey.t.kirsher@...el.com> :
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> index 10ea29f..fb47abb 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
>> @@ -812,6 +812,66 @@ static int ixgbe_get_eeprom(struct net_device
>*netdev,
>[...]
>> +	ptr = (void *)eeprom_buff;
>
>(nit) useless cast to void *.
>
>> +
>> +	if (eeprom->offset & 1) {
>[...]
>> +		ret_val = hw->eeprom.ops.read(hw, first_word, &eeprom_buff[0]);
>> +		ptr++;
>> +	}
>> +	if (((eeprom->offset + eeprom->len) & 1) && (ret_val == 0)) {
>[...]
>> +		ret_val = hw->eeprom.ops.read(hw, last_word,
>> +					  &eeprom_buff[last_word - first_word]);
>> +	}
>
>I guess that the code tries to do its best to write something even when the
>reads or writes partially fail but this one should imho be :

Actually we shouldn't be writing if any of the eeprom read operations fail. I will revisit the error checking logic.

>
>	if ((eeprom->offset + eeprom->len) & 1) {
>[...]
>		ret_val |= hw->eeprom.ops.read(hw, last_word,
>
>> +
>> +	/* Device's eeprom is always little-endian, word addressable */
>> +	for (i = 0; i < last_word - first_word + 1; i++)
>> +		le16_to_cpus(&eeprom_buff[i]);
>
>Shouldn't there be (drivers/net/ethernet/intel/ixgbe/ixgbe_type.h):
>
>struct ixgbe_eeprom_operations {
>[...]
>        s32 (*read)(struct ixgbe_hw *, u16, __le16 *);
>
>instead of :
>
>struct ixgbe_eeprom_operations {
>[...]
>        s32 (*read)(struct ixgbe_hw *, u16, u16 *);
>
>[...]

That's a good idea.

>> +	/* Update the checksum */
>> +	hw->eeprom.ops.update_checksum(hw);
>
>The returned status code is ignored.

Thanks for the review. I already asked Jeff to drop this patch, we'll address the issues pointed out and submit a new version.

Thanks,
Emil

>
>--
>Ueimor
--
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