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

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

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