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

Message-ID: <20111001093105.GA9885@electric-eye.fr.zoreil.com>
Date:	Sat, 1 Oct 2011 11:31:05 +0200
From:	Francois Romieu <romieu@...zoreil.com>
To:	Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:	davem@...emloft.net, Emil Tantilov <emil.s.tantilov@...el.com>,
	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 :

	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 *);

[...]
> +	/* Update the checksum */
> +	hw->eeprom.ops.update_checksum(hw);

The returned status code is ignored.

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