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

-----Original Message-----
From: Francois Romieu [mailto:romieu@fr.zoreil.com]
Sent: Saturday, October 01, 2011 2:31 AM
To: Kirsher, Jeffrey T
Cc: davem@davemloft.net; Tantilov, Emil S; <redacted>;
gospo@redhat.com
Subject: Re: [net-next 11/11 v2] ixgbe: allow eeprom writes via ethtool

Jeff Kirsher [off-list ref] :

quoted

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c

b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c

quoted

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,
[...]

quoted

+	ptr = (void *)eeprom_buff;

(nit) useless cast to void *.

quoted

+
+	if (eeprom->offset & 1) {

[...]

quoted

+		ret_val = hw->eeprom.ops.read(hw, first_word, &eeprom_buff[0]);
+		ptr++;
+	}
+	if (((eeprom->offset + eeprom->len) & 1) && (ret_val == 0)) {

[...]

quoted

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

quoted

+
+	/* 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.

quoted

+	/* 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