[PATCH 3/3] dm9601: add support ethtool style utility

[PATCH 3/3] dm9601: add support ethtool style utility

[Joseph CHANG]

Add function dm9601_set_eeprom which tested good with ethtool
utility, include the eeprom words dump and the eeprom byte write.

Signed-off-by: Joseph CHANG <josright123@gmail.com>
---
 drivers/net/usb/dm9601.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
index 50095df..a6904f4 100644
--- a/drivers/net/usb/dm9601.c
+++ b/drivers/net/usb/dm9601.c
@@ -61,6 +61,7 @@
 #define DM_RX_OVERHEAD	7	/* 3 byte header + 4 byte crc tail */
 #define DM_TIMEOUT	1000
 #define	DM_EP3I_VAL	0x07
+#define	MD96XX_EEPROM_MAGIC	0x9620
 
 static int dm_read(struct usbnet *dev, u8 reg, u16 length, void *data)
 {
@@ -289,6 +290,43 @@ static int dm9601_get_eeprom(struct net_device *net,
 	return 0;
 }
 
+static int dm9601_set_eeprom(struct net_device *net,
+			     struct ethtool_eeprom *eeprom, u8 *data)
+{
+	struct usbnet *dev = netdev_priv(net);
+	int offset = eeprom->offset;
+	int len = eeprom->len;
+	int done;
+
+	if (eeprom->magic != MD96XX_EEPROM_MAGIC) {
+		netdev_dbg(dev->net, "EEPROM: magic value mismatch, magic = 0x%x",
+			   eeprom->magic);
+		return -EINVAL;
+	}
+
+	while (len > 0) {
+		if (len & 1 || offset & 1) {
+			int which = offset & 1;
+			u8 tmp[2];
+
+			dm_read_eeprom_word(dev, offset / 2, tmp);
+			tmp[which] = *data;
+			dm_write_eeprom_word(dev, offset / 2,
+					     tmp[0] | tmp[1] << 8);
+			mdelay(10);
+			done = 1;
+		} else {
+			dm_write_eeprom_word(dev, offset / 2,
+					     data[0] | data[1] << 8);
+			done = 2;
+		}
+		data += done;
+		offset += done;
+		len -= done;
+	}
+	return 0;
+}
+
 static int dm9601_mdio_read(struct net_device *netdev, int phy_id, int loc)
 {
 	struct usbnet *dev = netdev_priv(netdev);
@@ -354,6 +392,7 @@ static const struct ethtool_ops dm9601_ethtool_ops = {
 	.set_msglevel	= usbnet_set_msglevel,
 	.get_eeprom_len	= dm9601_get_eeprom_len,
 	.get_eeprom	= dm9601_get_eeprom,
+	.set_eeprom	= dm9601_set_eeprom,
 	.get_settings	= usbnet_get_settings,
 	.set_settings	= usbnet_set_settings,
 	.nway_reset	= usbnet_nway_reset,
-- 
2.1.4

Re: [PATCH 3/3] dm9601: add support ethtool style utility

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2571 bytes --]

The subject line on this is very vague; it should say which ethtool
operation you're implementing.

On Thu, 2016-03-10 at 19:24 +0800, Joseph CHANG wrote:

> Add function dm9601_set_eeprom which tested good with ethtool
> utility, include the eeprom words dump and the eeprom byte write.
> 
> Signed-off-by: Joseph CHANG <josright123@gmail.com>
> ---
>  drivers/net/usb/dm9601.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
> index 50095df..a6904f4 100644
> --- a/drivers/net/usb/dm9601.c
> +++ b/drivers/net/usb/dm9601.c
> @@ -61,6 +61,7 @@
>  #define DM_RX_OVERHEAD	7	/* 3 byte header + 4 byte crc tail */
>  #define DM_TIMEOUT	1000
>  #define	DM_EP3I_VAL	0x07
> +#define	MD96XX_EEPROM_MAGIC	0x9620

The get_eeprom operation needs to be changed, to set eeprom->magic to
this value.

>  static int dm_read(struct usbnet *dev, u8 reg, u16 length, void *data)
>  {
> @@ -289,6 +290,43 @@ static int dm9601_get_eeprom(struct net_device *net,
>  	return 0;
>  }
>  
> +static int dm9601_set_eeprom(struct net_device *net,
> +			     struct ethtool_eeprom *eeprom, u8 *data)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	int offset = eeprom->offset;
> +	int len = eeprom->len;
> +	int done;
> +
> +	if (eeprom->magic != MD96XX_EEPROM_MAGIC) {
> +		netdev_dbg(dev->net, "EEPROM: magic value mismatch, magic = 0x%x",
> +			   eeprom->magic);
> +		return -EINVAL;
> +	}
> +
> +	while (len > 0) {
> +		if (len & 1 || offset & 1) {

Given that the get_eeprom operation only handles word-aligned reads, is
it really important to support misaligned writes in set_eeprom?

Also, this test should be 'if (len == 1 || offset & 1)'.  Consider a
write with offset = 2, len = 3.  You want to write a word on the first
iteration, then a byte on the second iteration.

> +			int which = offset & 1;
> +			u8 tmp[2];
> +
> +			dm_read_eeprom_word(dev, offset / 2, tmp);
> +			tmp[which] = *data;
> +			dm_write_eeprom_word(dev, offset / 2,
> +					     tmp[0] | tmp[1] << 8);
> +			mdelay(10);

Why is a delay required here, but not in the other case?

> +			done = 1;
> +		} else {
> +			dm_write_eeprom_word(dev, offset / 2,
> +					     data[0] | data[1] << 8);
> +			done = 2;
> +		}
> +		data += done;
> +		offset += done;
> +		len -= done;
> +	}
> +	return 0;
> +}
[...]

Ben.

-- 
Ben Hutchings
To err is human; to really foul things up requires a computer.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] dm9601: add support ethtool style utility

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2601 bytes --]

The subject line on this is very vague; it should say which ethtool
operation you're implementing.

On Thu, 2016-03-10 at 19:24 +0800, Joseph CHANG wrote:

> Add function dm9601_set_eeprom which tested good with ethtool
> utility, include the eeprom words dump and the eeprom byte write.
> 
> Signed-off-by: Joseph CHANG <josright123-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/net/usb/dm9601.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
> index 50095df..a6904f4 100644
> --- a/drivers/net/usb/dm9601.c
> +++ b/drivers/net/usb/dm9601.c
> @@ -61,6 +61,7 @@
>  #define DM_RX_OVERHEAD	7	/* 3 byte header + 4 byte crc tail */
>  #define DM_TIMEOUT	1000
>  #define	DM_EP3I_VAL	0x07
> +#define	MD96XX_EEPROM_MAGIC	0x9620

The get_eeprom operation needs to be changed, to set eeprom->magic to
this value.

>  static int dm_read(struct usbnet *dev, u8 reg, u16 length, void *data)
>  {
> @@ -289,6 +290,43 @@ static int dm9601_get_eeprom(struct net_device *net,
>  	return 0;
>  }
>  
> +static int dm9601_set_eeprom(struct net_device *net,
> +			     struct ethtool_eeprom *eeprom, u8 *data)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	int offset = eeprom->offset;
> +	int len = eeprom->len;
> +	int done;
> +
> +	if (eeprom->magic != MD96XX_EEPROM_MAGIC) {
> +		netdev_dbg(dev->net, "EEPROM: magic value mismatch, magic = 0x%x",
> +			   eeprom->magic);
> +		return -EINVAL;
> +	}
> +
> +	while (len > 0) {
> +		if (len & 1 || offset & 1) {

Given that the get_eeprom operation only handles word-aligned reads, is
it really important to support misaligned writes in set_eeprom?

Also, this test should be 'if (len == 1 || offset & 1)'.  Consider a
write with offset = 2, len = 3.  You want to write a word on the first
iteration, then a byte on the second iteration.

> +			int which = offset & 1;
> +			u8 tmp[2];
> +
> +			dm_read_eeprom_word(dev, offset / 2, tmp);
> +			tmp[which] = *data;
> +			dm_write_eeprom_word(dev, offset / 2,
> +					     tmp[0] | tmp[1] << 8);
> +			mdelay(10);

Why is a delay required here, but not in the other case?

> +			done = 1;
> +		} else {
> +			dm_write_eeprom_word(dev, offset / 2,
> +					     data[0] | data[1] << 8);
> +			done = 2;
> +		}
> +		data += done;
> +		offset += done;
> +		len -= done;
> +	}
> +	return 0;
> +}
[...]

Ben.

-- 
Ben Hutchings
To err is human; to really foul things up requires a computer.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH 3/3] dm9601: add support ethtool style utility

[Joseph Chang]

[-- Attachment #1: Type: text/plain, Size: 3829 bytes --]

I did verify to dump EEPROM data and also write EEPROM by per byte.

1.Plug dm9601/dm9621 adapter and has get dm9601.ko be 'insmod' to have 'eht0',
2.Run ethtool v3.7 (as attached executable file and it's help display.)
3. Commands:
   ./ethtool -e eth0  (dump EEPROM data for all the .get_eeprom_len )
   ./ethtool -E eth0 magic 0x9620 offset 0 value 0xf1  (write 0xf1 to eeprom byte0)
   ./ethtool -E eth0 magic 0x9620 offset 1 value 0xf2  (write 0xf2 to eeprom byte1)
   ./ethtool -E eth0 magic 0x9620 offset 2 value 0xf3  (write 0xf3 to eeprom byte2)

*I am not sure if it can cover all the huge purpose functions of ethtool, or
 some leakage in such implementation. Thanks and need further advice~

Best Regards,
Joseph CHANG
System Application Engineering Division
Davicom Semiconductor, Inc.
No. 6 Li-Hsin 6th Rd., Science-Based Park,
Hsin-Chu, Taiwan.
Tel: 886-3-5798797 Ex 8534
Fax: 886-3-5646929
Web: http://www.davicom.com.tw


-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk] 
Sent: Thursday, March 10, 2016 7:49 PM
To: Joseph CHANG; Peter Korsgaard; netdev@vger.kernel.org; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
Cc: Joseph Chang
Subject: Re: [PATCH 3/3] dm9601: add support ethtool style utility

The subject line on this is very vague; it should say which ethtool
operation you're implementing.

On Thu, 2016-03-10 at 19:24 +0800, Joseph CHANG wrote:

> Add function dm9601_set_eeprom which tested good with ethtool
> utility, include the eeprom words dump and the eeprom byte write.
> 
> Signed-off-by: Joseph CHANG <josright123@gmail.com>
> ---
>  drivers/net/usb/dm9601.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
> index 50095df..a6904f4 100644
> --- a/drivers/net/usb/dm9601.c
> +++ b/drivers/net/usb/dm9601.c
> @@ -61,6 +61,7 @@
>  #define DM_RX_OVERHEAD	7	/* 3 byte header + 4 byte crc tail */
>  #define DM_TIMEOUT	1000
>  #define	DM_EP3I_VAL	0x07
> +#define	MD96XX_EEPROM_MAGIC	0x9620

The get_eeprom operation needs to be changed, to set eeprom->magic to
this value.

>  static int dm_read(struct usbnet *dev, u8 reg, u16 length, void *data)
>  {
> @@ -289,6 +290,43 @@ static int dm9601_get_eeprom(struct net_device *net,
>  	return 0;
>  }
>  
> +static int dm9601_set_eeprom(struct net_device *net,
> +			     struct ethtool_eeprom *eeprom, u8 *data)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	int offset = eeprom->offset;
> +	int len = eeprom->len;
> +	int done;
> +
> +	if (eeprom->magic != MD96XX_EEPROM_MAGIC) {
> +		netdev_dbg(dev->net, "EEPROM: magic value mismatch, magic = 0x%x",
> +			   eeprom->magic);
> +		return -EINVAL;
> +	}
> +
> +	while (len > 0) {
> +		if (len & 1 || offset & 1) {

Given that the get_eeprom operation only handles word-aligned reads, is
it really important to support misaligned writes in set_eeprom?

Also, this test should be 'if (len == 1 || offset & 1)'.  Consider a
write with offset = 2, len = 3.  You want to write a word on the first
iteration, then a byte on the second iteration.

> +			int which = offset & 1;
> +			u8 tmp[2];
> +
> +			dm_read_eeprom_word(dev, offset / 2, tmp);
> +			tmp[which] = *data;
> +			dm_write_eeprom_word(dev, offset / 2,
> +					     tmp[0] | tmp[1] << 8);
> +			mdelay(10);

Why is a delay required here, but not in the other case?

> +			done = 1;
> +		} else {
> +			dm_write_eeprom_word(dev, offset / 2,
> +					     data[0] | data[1] << 8);
> +			done = 2;
> +		}
> +		data += done;
> +		offset += done;
> +		len -= done;
> +	}
> +	return 0;
> +}
[...]

Ben.

-- 
Ben Hutchings
To err is human; to really foul things up requires a computer.

[-- Attachment #2: ethtool --]
[-- Type: application/octet-stream, Size: 1034422 bytes --]

[-- Attachment #3: ethtool.help.txt --]
[-- Type: text/plain, Size: 4790 bytes --]

ethtool version 3.7
Usage:
        ethtool DEVNAME	Display standard information about device
        ethtool -s|--change DEVNAME	Change generic options
		[ speed %d ]
		[ duplex half|full ]
		[ port tp|aui|bnc|mii|fibre ]
		[ mdix auto|on|off ]
		[ autoneg on|off ]
		[ advertise %x ]
		[ phyad %d ]
		[ xcvr internal|external ]
		[ wol p|u|m|b|a|g|s|d... ]
		[ sopass %x:%x:%x:%x:%x:%x ]
		[ msglvl %d | msglvl type on|off ... ]
        ethtool -a|--show-pause DEVNAME	Show pause options
        ethtool -A|--pause DEVNAME	Set pause options
		[ autoneg on|off ]
		[ rx on|off ]
		[ tx on|off ]
        ethtool -c|--show-coalesce DEVNAME	Show coalesce options
        ethtool -C|--coalesce DEVNAME	Set coalesce options
		[adaptive-rx on|off]
		[adaptive-tx on|off]
		[rx-usecs N]
		[rx-frames N]
		[rx-usecs-irq N]
		[rx-frames-irq N]
		[tx-usecs N]
		[tx-frames N]
		[tx-usecs-irq N]
		[tx-frames-irq N]
		[stats-block-usecs N]
		[pkt-rate-low N]
		[rx-usecs-low N]
		[rx-frames-low N]
		[tx-usecs-low N]
		[tx-frames-low N]
		[pkt-rate-high N]
		[rx-usecs-high N]
		[rx-frames-high N]
		[tx-usecs-high N]
		[tx-frames-high N]
		[sample-interval N]
        ethtool -g|--show-ring DEVNAME	Query RX/TX ring parameters
        ethtool -G|--set-ring DEVNAME	Set RX/TX ring parameters
		[ rx N ]
		[ rx-mini N ]
		[ rx-jumbo N ]
		[ tx N ]
        ethtool -k|--show-features|--show-offload DEVNAME	Get state of protocol offload and other features
        ethtool -K|--features|--offload DEVNAME	Set protocol offload and other features
		FEATURE on|off ...
        ethtool -i|--driver DEVNAME	Show driver information
        ethtool -d|--register-dump DEVNAME	Do a register dump
		[ raw on|off ]
		[ file FILENAME ]
        ethtool -e|--eeprom-dump DEVNAME	Do a EEPROM dump
		[ raw on|off ]
		[ offset N ]
		[ length N ]
        ethtool -E|--change-eeprom DEVNAME	Change bytes in device EEPROM
		[ magic N ]
		[ offset N ]
		[ length N ]
		[ value N ]
        ethtool -r|--negotiate DEVNAME	Restart N-WAY negotiation
        ethtool -p|--identify DEVNAME	Show visible port identification (e.g. blinking)
               [ TIME-IN-SECONDS ]
        ethtool -t|--test DEVNAME	Execute adapter self test
               [ online | offline | external_lb ]
        ethtool -S|--statistics DEVNAME	Show adapter statistics
        ethtool -n|-u|--show-nfc|--show-ntuple DEVNAME	Show Rx network flow classification options or rules
		[ rx-flow-hash tcp4|udp4|ah4|esp4|sctp4|tcp6|udp6|ah6|esp6|sctp6 |
		  rule %d ]
        ethtool -N|-U|--config-nfc|--config-ntuple DEVNAME	Configure Rx network flow classification options or rules
		rx-flow-hash tcp4|udp4|ah4|esp4|sctp4|tcp6|udp6|ah6|esp6|sctp6 m|v|t|s|d|f|n|r... |
		flow-type ether|ip4|tcp4|udp4|sctp4|ah4|esp4
			[ src %x:%x:%x:%x:%x:%x [m %x:%x:%x:%x:%x:%x] ]
			[ dst %x:%x:%x:%x:%x:%x [m %x:%x:%x:%x:%x:%x] ]
			[ proto %d [m %x] ]
			[ src-ip %d.%d.%d.%d [m %d.%d.%d.%d] ]
			[ dst-ip %d.%d.%d.%d [m %d.%d.%d.%d] ]
			[ tos %d [m %x] ]
			[ l4proto %d [m %x] ]
			[ src-port %d [m %x] ]
			[ dst-port %d [m %x] ]
			[ spi %d [m %x] ]
			[ vlan-etype %x [m %x] ]
			[ vlan %x [m %x] ]
			[ user-def %x [m %x] ]
			[ action %d ]
			[ loc %d]] |
		delete %d
        ethtool -T|--show-time-stamping DEVNAME	Show time stamping capabilities
        ethtool -x|--show-rxfh-indir DEVNAME	Show Rx flow hash indirection
        ethtool -X|--set-rxfh-indir DEVNAME	Set Rx flow hash indirection
		equal N | weight W0 W1 ...
        ethtool -f|--flash DEVNAME	Flash firmware image from the specified file to a region on the device
               FILENAME [ REGION-NUMBER-TO-FLASH ]
        ethtool -P|--show-permaddr DEVNAME	Show permanent hardware address
        ethtool -w|--get-dump DEVNAME	Get dump flag, data
		[ data FILENAME ]
        ethtool -W|--set-dump DEVNAME	Set dump flag of the device
		N
        ethtool -l|--show-channels DEVNAME	Query Channels
        ethtool -L|--set-channels DEVNAME	Set Channels
               [ rx N ]
               [ tx N ]
               [ other N ]
               [ combined N ]
        ethtool --show-priv-flags DEVNAME	Query private flags
        ethtool --set-priv-flags DEVNAME	Set private flags
		FLAG on|off ...
        ethtool -m|--dump-module-eeprom|--module-info DEVNAME	Query/Decode Module EEPROM information and optical diagnostics if available
		[ raw on|off ]
		[ hex on|off ]
		[ offset N ]
		[ length N ]
        ethtool --show-eee DEVNAME	Show EEE settings
        ethtool --set-eee DEVNAME	Set EEE settings
		[ eee on|off ]
		[ advertise %x ]
		[ tx-lpi on|off ]
		[ tx-timer %d ]
        ethtool -h|--help 		Show this help
        ethtool --version 		Show version number

Re: [PATCH 3/3] dm9601: add support ethtool style utility

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On Thu, 2016-03-10 at 20:51 +0800, Joseph Chang wrote:
> I did verify to dump EEPROM data and also write EEPROM by per byte.
> 
> 1.Plug dm9601/dm9621 adapter and has get dm9601.ko be 'insmod' to have 'eht0',
> 2.Run ethtool v3.7 (as attached executable file and it's help display.)
> 3. Commands:
>    ./ethtool -e eth0  (dump EEPROM data for all the .get_eeprom_len )
>    ./ethtool -E eth0 magic 0x9620 offset 0 value 0xf1  (write 0xf1 to eeprom byte0)
>    ./ethtool -E eth0 magic 0x9620 offset 1 value 0xf2  (write 0xf2 to eeprom byte1)
>    ./ethtool -E eth0 magic 0x9620 offset 2 value 0xf3  (write 0xf3 to eeprom byte2)
[...]

So you only tested writing 1 byte at a time.  Try again with 3 bytes
and you'll see how it goes wrong.

Ben.

-- 
Ben Hutchings
To err is human; to really foul things up requires a computer.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] dm9601: add support ethtool style utility

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On Thu, 2016-03-10 at 20:51 +0800, Joseph Chang wrote:
> I did verify to dump EEPROM data and also write EEPROM by per byte.
> 
> 1.Plug dm9601/dm9621 adapter and has get dm9601.ko be 'insmod' to have 'eht0',
> 2.Run ethtool v3.7 (as attached executable file and it's help display.)
> 3. Commands:
>    ./ethtool -e eth0  (dump EEPROM data for all the .get_eeprom_len )
>    ./ethtool -E eth0 magic 0x9620 offset 0 value 0xf1  (write 0xf1 to eeprom byte0)
>    ./ethtool -E eth0 magic 0x9620 offset 1 value 0xf2  (write 0xf2 to eeprom byte1)
>    ./ethtool -E eth0 magic 0x9620 offset 2 value 0xf3  (write 0xf3 to eeprom byte2)
[...]

So you only tested writing 1 byte at a time.  Try again with 3 bytes
and you'll see how it goes wrong.

Ben.

-- 
Ben Hutchings
To err is human; to really foul things up requires a computer.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH 3/3] dm9601: add support ethtool style utility

[Joseph Chang]

I tested by
 ./ethtool -E eth0 magic 0x9620 offset 0 length 3 value 0xf1 value 0xf2 value 0xf3

I think ethtool need [ value N ] for each byte (so need three "value xx" in above command line), 
am I right?

Oh, I can see it goes wrong~ Thanks~

* I will go a vacation from now on, this issue will be study later.
 Any helpful reference implementation data is appreciated. 
 
Best Regards,
Joseph CHANG
System Application Engineering Division
Davicom Semiconductor, Inc.
No. 6 Li-Hsin 6th Rd., Science-Based Park,
Hsin-Chu, Taiwan.
Tel: 886-3-5798797 Ex 8534
Fax: 886-3-5646929
Web: http://www.davicom.com.tw


-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk] 
Sent: Thursday, March 10, 2016 9:18 PM
To: Joseph Chang; 'Joseph CHANG'; 'Peter Korsgaard'; netdev@vger.kernel.org; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] dm9601: add support ethtool style utility

On Thu, 2016-03-10 at 20:51 +0800, Joseph Chang wrote:
> I did verify to dump EEPROM data and also write EEPROM by per byte.
> 
> 1.Plug dm9601/dm9621 adapter and has get dm9601.ko be 'insmod' to have 'eht0',
> 2.Run ethtool v3.7 (as attached executable file and it's help display.)
> 3. Commands:
>    ./ethtool -e eth0  (dump EEPROM data for all the .get_eeprom_len )
>    ./ethtool -E eth0 magic 0x9620 offset 0 value 0xf1  (write 0xf1 to eeprom byte0)
>    ./ethtool -E eth0 magic 0x9620 offset 1 value 0xf2  (write 0xf2 to eeprom byte1)
>    ./ethtool -E eth0 magic 0x9620 offset 2 value 0xf3  (write 0xf3 to eeprom byte2)
[...]

So you only tested writing 1 byte at a time.  Try again with 3 bytes
and you'll see how it goes wrong.

Ben.

-- 
Ben Hutchings
To err is human; to really foul things up requires a computer.

Re: [PATCH 3/3] dm9601: add support ethtool style utility

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

On Fri, 2016-03-11 at 19:08 +0800, Joseph Chang wrote:
> I tested by
>  ./ethtool -E eth0 magic 0x9620 offset 0 length 3 value 0xf1 value 0xf2 value 0xf3
> 
> I think ethtool need [ value N ] for each byte (so need three "value xx" in above command line), 
> am I right?
> 
> Oh, I can see it goes wrong~ Thanks~
[...]

You can only pass one byte on the command line and that forces the
length to be 1.  To set multiple bytes, you need to provide them on
stdin instead.

Ben.

-- 
Ben Hutchings
73.46% of all statistics are made up.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] dm9601: add support ethtool style utility

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

On Fri, 2016-03-11 at 11:33 +0000, Ben Hutchings wrote:
> On Fri, 2016-03-11 at 19:08 +0800, Joseph Chang wrote:
> > 
> > I tested by
> >  ./ethtool -E eth0 magic 0x9620 offset 0 length 3 value 0xf1 value 0xf2 value 0xf3
> > 
> > I think ethtool need [ value N ] for each byte (so need three "value xx" in above command line), 
> > am I right?
> > 
> > Oh, I can see it goes wrong~ Thanks~
> [...]
> 
> You can only pass one byte on the command line and that forces the
> length to be 1.  To set multiple bytes, you need to provide them on
> stdin instead.

...as raw binary data, not hexadecimal strings.

Ben.

-- 
Ben Hutchings
73.46% of all statistics are made up.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] dm9601: add support ethtool style utility

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 693 bytes --]

On Fri, 2016-03-11 at 11:33 +0000, Ben Hutchings wrote:
> On Fri, 2016-03-11 at 19:08 +0800, Joseph Chang wrote:
> > 
> > I tested by
> >  ./ethtool -E eth0 magic 0x9620 offset 0 length 3 value 0xf1 value 0xf2 value 0xf3
> > 
> > I think ethtool need [ value N ] for each byte (so need three "value xx" in above command line), 
> > am I right?
> > 
> > Oh, I can see it goes wrong~ Thanks~
> [...]
> 
> You can only pass one byte on the command line and that forces the
> length to be 1.  To set multiple bytes, you need to provide them on
> stdin instead.

...as raw binary data, not hexadecimal strings.

Ben.

-- 
Ben Hutchings
73.46% of all statistics are made up.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]