: Add support for large eeproms

PATCH Applied Mandeep Singh Baines 6 participants 14 messages Apr 3, 2008 – Apr 25, 2008 1 message not in the archive

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): 1207195123.23161.291.camel@localhost

Mandeep Singh Baines

author

2008-04-03 18:00 UTC #1 lore raw

Thanks Joe,

Fixed and updated patch. Please take another look.

Regards,
Mandeep

Quoted 6 lines · Joe Perches

net/core/ethtool.c
Quoted 2 lines · Mandeep Singh Baines
net/core/ethtool.c +0 -0
index 1163eb2..54f1004 100644
please run scripts/checkpatch.pl

Fixed

Quoted 12 lines

net/core/ethtool.c static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr) · line 336
Quoted 9 lines
net/core/ethtool.c +3 -1
@@ -329,8 +330,10 @@ static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_eeprom eeprom;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
+	void __user *userbuf = useraddr + sizeof(eeprom);
+	u32 bytes_remaining;
 	u8 *data;
-	int ret;
+	int ret = 0;
You don't need to initialize this.

Fixed

---
Currently, it is not possible to read/write to an eeprom larger than 128k in size because the buffer used for temporarily storing the eeprom contents is allocated using kmalloc. kmalloc can only allocate a maximum of 128k depending on architecture.

Modified ethtool_get/set_eeprom to only allocate a page of memory and then copy the eeprom a page at a time.

Signed-off-by Mandeep Singh Baines <[email protected]>

---
 net/core/ethtool.c |   57 +++++++++++++++++++++++++++++----------------------
 1 files changed, 32 insertions(+), 25 deletions(-)

net/core/ethtool.c +32 ~~-25~~

index 1163eb2..45d5f35 100644
@@ -284,6 +284,8 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_eeprom eeprom;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
+	void __user *userbuf = useraddr + sizeof(eeprom);
+	u32 bytes_remaining;
 	u8 *data;
 	int ret;

@@ -301,26 +303,26 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
 	if (eeprom.offset + eeprom.len > ops->get_eeprom_len(dev))
 		return -EINVAL;
 
-	data = kmalloc(eeprom.len, GFP_USER);
+	data = kmalloc(PAGE_SIZE, GFP_USER);
 	if (!data)
 		return -ENOMEM;
 
-	ret = -EFAULT;
-	if (copy_from_user(data, useraddr + sizeof(eeprom), eeprom.len))
-		goto out;
-
-	ret = ops->get_eeprom(dev, &eeprom, data);
-	if (ret)
-		goto out;
+	bytes_remaining = eeprom.len;
+	while (bytes_remaining > 0) {
+		eeprom.len = min(bytes_remaining, (u32)PAGE_SIZE);
 
-	ret = -EFAULT;
-	if (copy_to_user(useraddr, &eeprom, sizeof(eeprom)))
-		goto out;
-	if (copy_to_user(useraddr + sizeof(eeprom), data, eeprom.len))
-		goto out;
-	ret = 0;
+		ret = ops->get_eeprom(dev, &eeprom, data)
+		if (ret)
+			break;
+		ret = -EFAULT;
+		if (copy_to_user(userbuf, data, eeprom.len))
+			break;
+		ret = 0;
+		userbuf += eeprom.len;
+		eeprom.offset += eeprom.len;
+		bytes_remaining -= eeprom.len;
+	}
 
- out:
 	kfree(data);
 	return ret;
 }

@@ -329,6 +331,8 @@ static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_eeprom eeprom;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
+	void __user *userbuf = useraddr + sizeof(eeprom);
+	u32 bytes_remaining;
 	u8 *data;
 	int ret;

@@ -346,22 +350,25 @@ static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
 	if (eeprom.offset + eeprom.len > ops->get_eeprom_len(dev))
 		return -EINVAL;
 
-	data = kmalloc(eeprom.len, GFP_USER);
+	data = kmalloc(PAGE_SIZE, GFP_USER);
 	if (!data)
 		return -ENOMEM;
 
-	ret = -EFAULT;
-	if (copy_from_user(data, useraddr + sizeof(eeprom), eeprom.len))
-		goto out;
-
-	ret = ops->set_eeprom(dev, &eeprom, data);
-	if (ret)
-		goto out;
+	bytes_remaining = eeprom.len;
+	while (bytes_remaining > 0) {
+		eeprom.len = min(bytes_remaining, (u32)PAGE_SIZE);
 
-	if (copy_to_user(useraddr + sizeof(eeprom), data, eeprom.len))
 		ret = -EFAULT;
+		if (copy_from_user(data, userbuf, eeprom.len))
+			break;
+		ret = ops->set_eeprom(dev, &eeprom, data);
+		if (ret)
+			break;
+		userbuf += eeprom.len;
+		eeprom.offset += eeprom.len;
+		bytes_remaining -= eeprom.len;
+	}
 
- out:
 	kfree(data);
 	return ret;
 }

Signature · 1 line

1.5.2.5

Joe Perches

2008-04-04 00:04 UTC #2 lore raw

Quoted 1 line · Mandeep Singh Baines

Fixed and updated patch. Please take another look.

Hi Mandeep,

This looks better.
There's a typo at line 315 though, it needs a ";".

I was also wrong that int ret _does_ need to be
initialized to 0 in case (eeprom.len == 0)

I think this should be written as below:

cheers, Joe

 net/core/ethtool.c |   68 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 38 insertions(+), 30 deletions(-)

net/core/ethtool.c +36 ~~-28~~

index 1163eb2..a29b43d 100644
@@ -284,8 +284,10 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_eeprom eeprom;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
+	void __user *userbuf = useraddr + sizeof(eeprom);
+	u32 bytes_remaining;
 	u8 *data;
-	int ret;
+	int ret = 0;
 
 	if (!ops->get_eeprom || !ops->get_eeprom_len)
 		return -EOPNOTSUPP;

@@ -301,26 +303,26 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
 	if (eeprom.offset + eeprom.len > ops->get_eeprom_len(dev))
 		return -EINVAL;
 
-	data = kmalloc(eeprom.len, GFP_USER);
+	data = kmalloc(PAGE_SIZE, GFP_USER);
 	if (!data)
 		return -ENOMEM;
 
-	ret = -EFAULT;
-	if (copy_from_user(data, useraddr + sizeof(eeprom), eeprom.len))
-		goto out;
-
-	ret = ops->get_eeprom(dev, &eeprom, data);
-	if (ret)
-		goto out;
+	bytes_remaining = eeprom.len;
+	while (bytes_remaining > 0) {
+		eeprom.len = min(bytes_remaining, (u32)PAGE_SIZE);
 
-	ret = -EFAULT;
-	if (copy_to_user(useraddr, &eeprom, sizeof(eeprom)))
-		goto out;
-	if (copy_to_user(useraddr + sizeof(eeprom), data, eeprom.len))
-		goto out;
-	ret = 0;
+		ret = ops->get_eeprom(dev, &eeprom, data);
+		if (ret)
+			break;
+		if (copy_to_user(userbuf, data, eeprom.len)) {
+			ret = -EFAULT;
+			break;
+		}
+		userbuf += eeprom.len;
+		eeprom.offset += eeprom.len;
+		bytes_remaining -= eeprom.len;
+	}
 
- out:
 	kfree(data);
 	return ret;
 }

@@ -329,8 +331,10 @@ static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_eeprom eeprom;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
+	void __user *userbuf = useraddr + sizeof(eeprom);
+	u32 bytes_remaining;
 	u8 *data;
-	int ret;
+	int ret = 0;
 
 	if (!ops->set_eeprom || !ops->get_eeprom_len)
 		return -EOPNOTSUPP;

@@ -346,22 +350,26 @@ static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
 	if (eeprom.offset + eeprom.len > ops->get_eeprom_len(dev))
 		return -EINVAL;
 
-	data = kmalloc(eeprom.len, GFP_USER);
+	data = kmalloc(PAGE_SIZE, GFP_USER);
 	if (!data)
 		return -ENOMEM;
 
-	ret = -EFAULT;
-	if (copy_from_user(data, useraddr + sizeof(eeprom), eeprom.len))
-		goto out;
-
-	ret = ops->set_eeprom(dev, &eeprom, data);
-	if (ret)
-		goto out;
+	bytes_remaining = eeprom.len;
+	while (bytes_remaining > 0) {
+		eeprom.len = min(bytes_remaining, (u32)PAGE_SIZE);
 
-	if (copy_to_user(useraddr + sizeof(eeprom), data, eeprom.len))
-		ret = -EFAULT;
+		if (copy_from_user(data, userbuf, eeprom.len)) {
+			ret = -EFAULT;
+			break;
+		}
+		ret = ops->set_eeprom(dev, &eeprom, data);
+		if (ret)
+			break;
+		userbuf += eeprom.len;
+		eeprom.offset += eeprom.len;
+		bytes_remaining -= eeprom.len;
+	}
 
- out:
 	kfree(data);
 	return ret;
 }

Mandeep Singh Baines

author

2008-04-04 01:42 UTC #3 lore raw

Quoted 3 lines · Joe Perches

This looks better.
There's a typo at line 315 though, it needs a ";".

Oops. In my haste to get out the revised patch I didn't bother to test. Bad Mandeep.

Quoted 3 lines

I was also wrong that int ret _does_ need to be
initialized to 0 in case (eeprom.len == 0)

I knew I had that there for a reason. But I forgot to initialize ret in ethtool_get_eeprom. You've fixed this in your update to the patch:)

Quoted 2 lines

I think this should be written as below:

Agreed. You've fixed my bugs and I think setting ret inside the if makes the code easier to reason about.

I've tested your updated version of the patch and it looks good.

Quoted 115 lines

cheers, Joe

 net/core/ethtool.c |   68 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 38 insertions(+), 30 deletions(-)

net/core/ethtool.c +36 ~~-28~~

index 1163eb2..a29b43d 100644
@@ -284,8 +284,10 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_eeprom eeprom;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
+	void __user *userbuf = useraddr + sizeof(eeprom);
+	u32 bytes_remaining;
 	u8 *data;
-	int ret;
+	int ret = 0;
 
 	if (!ops->get_eeprom || !ops->get_eeprom_len)
 		return -EOPNOTSUPP;

@@ -301,26 +303,26 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
 	if (eeprom.offset + eeprom.len > ops->get_eeprom_len(dev))
 		return -EINVAL;
 
-	data = kmalloc(eeprom.len, GFP_USER);
+	data = kmalloc(PAGE_SIZE, GFP_USER);
 	if (!data)
 		return -ENOMEM;
 
-	ret = -EFAULT;
-	if (copy_from_user(data, useraddr + sizeof(eeprom), eeprom.len))
-		goto out;
-
-	ret = ops->get_eeprom(dev, &eeprom, data);
-	if (ret)
-		goto out;
+	bytes_remaining = eeprom.len;
+	while (bytes_remaining > 0) {
+		eeprom.len = min(bytes_remaining, (u32)PAGE_SIZE);
 
-	ret = -EFAULT;
-	if (copy_to_user(useraddr, &eeprom, sizeof(eeprom)))
-		goto out;
-	if (copy_to_user(useraddr + sizeof(eeprom), data, eeprom.len))
-		goto out;
-	ret = 0;
+		ret = ops->get_eeprom(dev, &eeprom, data);
+		if (ret)
+			break;
+		if (copy_to_user(userbuf, data, eeprom.len)) {
+			ret = -EFAULT;
+			break;
+		}
+		userbuf += eeprom.len;
+		eeprom.offset += eeprom.len;
+		bytes_remaining -= eeprom.len;
+	}
 
- out:
 	kfree(data);
 	return ret;
 }

@@ -329,8 +331,10 @@ static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_eeprom eeprom;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
+	void __user *userbuf = useraddr + sizeof(eeprom);
+	u32 bytes_remaining;
 	u8 *data;
-	int ret;
+	int ret = 0;
 
 	if (!ops->set_eeprom || !ops->get_eeprom_len)
 		return -EOPNOTSUPP;

@@ -346,22 +350,26 @@ static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
 	if (eeprom.offset + eeprom.len > ops->get_eeprom_len(dev))
 		return -EINVAL;
 
-	data = kmalloc(eeprom.len, GFP_USER);
+	data = kmalloc(PAGE_SIZE, GFP_USER);
 	if (!data)
 		return -ENOMEM;
 
-	ret = -EFAULT;
-	if (copy_from_user(data, useraddr + sizeof(eeprom), eeprom.len))
-		goto out;
-
-	ret = ops->set_eeprom(dev, &eeprom, data);
-	if (ret)
-		goto out;
+	bytes_remaining = eeprom.len;
+	while (bytes_remaining > 0) {
+		eeprom.len = min(bytes_remaining, (u32)PAGE_SIZE);
 
-	if (copy_to_user(useraddr + sizeof(eeprom), data, eeprom.len))
-		ret = -EFAULT;
+		if (copy_from_user(data, userbuf, eeprom.len)) {
+			ret = -EFAULT;
+			break;
+		}
+		ret = ops->set_eeprom(dev, &eeprom, data);
+		if (ret)
+			break;
+		userbuf += eeprom.len;
+		eeprom.offset += eeprom.len;
+		bytes_remaining -= eeprom.len;
+	}
 
- out:
 	kfree(data);
 	return ret;
 }

Jeff Garzik

2008-04-12 09:11 UTC #4 lore raw

Quoted 15 lines · Mandeep Singh Baines

Quoted 3 lines · Joe Perches
This looks better.
There's a typo at line 315 though, it needs a ";".
Oops. In my haste to get out the revised patch I didn't bother to test. Bad Mandeep.
Quoted 3 lines
I was also wrong that int ret _does_ need to be
initialized to 0 in case (eeprom.len == 0)
I knew I had that there for a reason. But I forgot to initialize ret in ethtool_get_eeprom. You've fixed this in your update to the patch:)

Quoted 12 lines

Quoted 2 lines
I think this should be written as below:
Agreed. You've fixed my bugs and I think setting ret inside the if makes the code easier to reason about.
I've tested your updated version of the patch and it looks good.
Quoted 4 lines
cheers, Joe
 net/core/ethtool.c |   68 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 38 insertions(+), 30 deletions(-)

someone wanna resend with proper sign-offs?

Mandeep Singh Baines

author

2008-04-14 18:04 UTC #5 lore raw

PATCH 1/1 : Add support for large eeproms

Currently, it is not possible to read/write to an eeprom larger than 128k in size because the buffer used for temporarily storing the eeprom contents is allocated using kmalloc. kmalloc can only allocate a maximum of 128k depending on architecture.

Modified ethtool_get/set_eeprom to only allocate a page of memory and then copy the eeprom a page at a time.

Updated original patch as per suggestions from Joe Perches.

Signed-off-by Mandeep Singh Baines <[email protected]>

---
 net/core/ethtool.c |   64 +++++++++++++++++++++++++++++----------------------
 1 files changed, 36 insertions(+), 28 deletions(-)

net/core/ethtool.c +36 ~~-28~~

index 1163eb2..a29b43d 100644
@@ -284,8 +284,10 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_eeprom eeprom;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
+	void __user *userbuf = useraddr + sizeof(eeprom);
+	u32 bytes_remaining;
 	u8 *data;
-	int ret;
+	int ret = 0;
 
 	if (!ops->get_eeprom || !ops->get_eeprom_len)
 		return -EOPNOTSUPP;

@@ -301,26 +303,26 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
 	if (eeprom.offset + eeprom.len > ops->get_eeprom_len(dev))
 		return -EINVAL;
 
-	data = kmalloc(eeprom.len, GFP_USER);
+	data = kmalloc(PAGE_SIZE, GFP_USER);
 	if (!data)
 		return -ENOMEM;
 
-	ret = -EFAULT;
-	if (copy_from_user(data, useraddr + sizeof(eeprom), eeprom.len))
-		goto out;
-
-	ret = ops->get_eeprom(dev, &eeprom, data);
-	if (ret)
-		goto out;
+	bytes_remaining = eeprom.len;
+	while (bytes_remaining > 0) {
+		eeprom.len = min(bytes_remaining, (u32)PAGE_SIZE);
 
-	ret = -EFAULT;
-	if (copy_to_user(useraddr, &eeprom, sizeof(eeprom)))
-		goto out;
-	if (copy_to_user(useraddr + sizeof(eeprom), data, eeprom.len))
-		goto out;
-	ret = 0;
+		ret = ops->get_eeprom(dev, &eeprom, data);
+		if (ret)
+			break;
+		if (copy_to_user(userbuf, data, eeprom.len)) {
+			ret = -EFAULT;
+			break;
+		}
+		userbuf += eeprom.len;
+		eeprom.offset += eeprom.len;
+		bytes_remaining -= eeprom.len;
+	}
 
- out:
 	kfree(data);
 	return ret;
 }

@@ -329,8 +331,10 @@ static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
 {
 	struct ethtool_eeprom eeprom;
 	const struct ethtool_ops *ops = dev->ethtool_ops;
+	void __user *userbuf = useraddr + sizeof(eeprom);
+	u32 bytes_remaining;
 	u8 *data;
-	int ret;
+	int ret = 0;
 
 	if (!ops->set_eeprom || !ops->get_eeprom_len)
 		return -EOPNOTSUPP;

@@ -346,22 +350,26 @@ static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
 	if (eeprom.offset + eeprom.len > ops->get_eeprom_len(dev))
 		return -EINVAL;
 
-	data = kmalloc(eeprom.len, GFP_USER);
+	data = kmalloc(PAGE_SIZE, GFP_USER);
 	if (!data)
 		return -ENOMEM;
 
-	ret = -EFAULT;
-	if (copy_from_user(data, useraddr + sizeof(eeprom), eeprom.len))
-		goto out;
-
-	ret = ops->set_eeprom(dev, &eeprom, data);
-	if (ret)
-		goto out;
+	bytes_remaining = eeprom.len;
+	while (bytes_remaining > 0) {
+		eeprom.len = min(bytes_remaining, (u32)PAGE_SIZE);
 
-	if (copy_to_user(useraddr + sizeof(eeprom), data, eeprom.len))
-		ret = -EFAULT;
+		if (copy_from_user(data, userbuf, eeprom.len)) {
+			ret = -EFAULT;
+			break;
+		}
+		ret = ops->set_eeprom(dev, &eeprom, data);
+		if (ret)
+			break;
+		userbuf += eeprom.len;
+		eeprom.offset += eeprom.len;
+		bytes_remaining -= eeprom.len;
+	}
 
- out:
 	kfree(data);
 	return ret;
 }

Signature · 1 line

1.5.2.5

David Miller

maintainer

2008-04-15 07:31 UTC #6 lore raw

From: Mandeep Singh Baines <[email protected]>
Date: Mon, 14 Apr 2008 11:03:38 -0700

Quoted 11 lines

Currently, it is not possible to read/write to an eeprom larger than 128k in size because the buffer used for temporarily storing the eeprom contents is allocated using kmalloc. kmalloc can only allocate a maximum of 128k depending on architecture.
Modified ethtool_get/set_eeprom to only allocate a page of memory and then copy the eeprom a page at a time.
Updated original patch as per suggestions from Joe Perches.
Signed-off-by Mandeep Singh Baines <[email protected]>

This looks fine on the surface.

But I wonder what we can do if we have some EEPROM implementation that can only write the whole time at once, and thus will fail if you try to do it in pieces? Do we have such a case?

If so this new code could cause regressions.

Tim Hockin

2008-04-15 17:09 UTC #7 lore raw

Hey Dave, long time!

Quoted 8 lines · David Miller

This looks fine on the surface.
But I wonder what we can do if we have some EEPROM implementation that can only write the whole time at once, and thus will fail if you try to do it in pieces? Do we have such a case?
If so this new code could cause regressions.

No device that I know of behaves that way. It's certainly possible, but I've never seen any sort of PROM device that has such a requirement.

Tim

David Miller

maintainer

2008-04-16 02:23 UTC #8 lore raw

From: "Tim Hockin" <[email protected]>
Date: Tue, 15 Apr 2008 10:07:21 -0700

Quoted 8 lines · David Miller

Quoted 7 lines
This looks fine on the surface.
But I wonder what we can do if we have some EEPROM implementation that can only write the whole time at once, and thus will fail if you try to do it in pieces? Do we have such a case?

Quoted 5 lines

Quoted 1 line
If so this new code could cause regressions.
No device that I know of behaves that way. It's certainly possible, but I've never seen any sort of PROM device that has such a requirement.

Fair enough, let's apply the patch and see if anything explode :)

Breno Leitao

2008-04-24 18:17 UTC #9 lore raw

Quoted 11 lines · David Miller

From: "Tim Hockin" <[email protected]>
Date: Tue, 15 Apr 2008 10:07:21 -0700
Quoted 4 lines
No device that I know of behaves that way. It's certainly possible, but I've never seen any sort of PROM device that has such a requirement.
Fair enough, let's apply the patch and see if anything explode :)

I've just applied this patch, and I got something really strange now. I got the following message "Magic number 0x00000000 does not match 0x669955aa " when I try to dump the eeprom from my tg3 card (BCM5780S rev. 03). I suppose that somehow it isn't getting the correct NIC magic number.

Well, without the patch, I just got a "Cannnot allocate enough memory" message when I rung the ethtool command, but if I pass the length argument < 128k, then everything went ok. Now, with the patch applied, I can't even dump even using the length parameter, since I hit the same Magic number error.

Note that I didn't run the kernel from your tree, I just backport this patch to my current kernel 2.6.16, and tried it over a ppc machine. I'll run the kernel from your tree later and then post the result.

--
Breno Leitão
[email protected]

Mandeep Singh Baines

author

2008-04-24 19:02 UTC #10 lore raw

Hi Breno,

Comments are inline.

Quoted 5 lines · Breno Leitao

I've just applied this patch, and I got something really strange now. I got the following message "Magic number 0x00000000 does not match 0x669955aa " when I try to dump the eeprom from my tg3 card (BCM5780S rev. 03). I suppose that somehow it isn't getting the correct NIC magic number.

You should not be getting this error when trying to dump an EEPROM.

The magic number is only checked when writing to an eeprom. The magic number prevents accidental writes. The check is implemented inside the driver's set_eeprom function. In this case, the function would be tg3_set_eeprom().

Quoted 10 lines

Well, without the patch, I just got a "Cannnot allocate enough memory" message when I rung the ethtool command, but if I pass the length argument < 128k, then everything went ok.
Now, with the patch applied, I can't even dump even using the length parameter, since I hit the same Magic number error.
Note that I didn't run the kernel from your tree, I just backport this patch to my current kernel 2.6.16, and tried it over a ppc machine. I'll run the kernel from your tree later and then post the result.

I suspect you may have mis-applied the patch. The patch changes both set_eeprom and get_eeprom. I think you may have modified get_eeprom to call ops->set_eeprom instead of ops->get_eeprom. If not, please send me ethtool.c and tg3.c and I can take a look.

Regards,
Mandeep

Breno Leitao

2008-04-24 20:21 UTC #11 lore raw

Mandeep,

Quoted 4 lines · Mandeep Singh Baines

I suspect you may have mis-applied the patch. The patch changes both set_eeprom and get_eeprom. I think you may have modified get_eeprom to call ops->set_eeprom instead of ops->get_eeprom. If not, please send me ethtool.c and tg3.c and I can take a look.

Well, IMHO the patch is correctly applied as I could see. I just got the David's tree and run a diff between the ethtool.c files (from David's tree and mine), and I get the following differences. Note that I just add a printk() there to "debug" what was going on.

static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)

{
       struct ethtool_eeprom eeprom;
-       const struct ethtool_ops *ops = dev->ethtool_ops;
+       struct ethtool_ops *ops = dev->ethtool_ops;
       void __user *userbuf = useraddr + sizeof(eeprom);
       u32 bytes_remaining;
       u8 *data;
       int ret = 0;

+       printk("<1> ethtool_get_eeprom()\n");
       if (!ops->get_eeprom || !ops->get_eeprom_len)
               return -EOPNOTSUPP;

@@ -330,12 +330,13 @@ static int ethtool_get_eeprom(struct net static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)

{
       struct ethtool_eeprom eeprom;
-       const struct ethtool_ops *ops = dev->ethtool_ops;
+       struct ethtool_ops *ops = dev->ethtool_ops;
       void __user *userbuf = useraddr + sizeof(eeprom);
       u32 bytes_remaining;
       u8 *data;
       int ret = 0;

+       printk("<1> ethtool_set_eeprom()\n");
       if (!ops->set_eeprom || !ops->get_eeprom_len)
               return -EOPNOTSUPP;

The patched ethtool.c is at http://rafb.net/p/fWXNwk28.html and tg3.c is at http://rapidshare.com/files/110139941/tg3.c.html

--
Breno Leitão
[email protected]

Mandeep Singh Baines

author

2008-04-25 01:16 UTC #12 lore raw

Hi Breno,

My bad. You are correct. There is a bug. In the ethtool user-space app, tg3 and natsemi over-ride the default implementation of dump_eeprom(). In both tg3_dump_eeprom() and natsemi_dump_eeprom(), there is a magic number check which is not present in the default implementation.

When fixing the ethtool interface to read large EEPROMs, I snipped the code which copied the ethtool_eeprom structure back to user-space. I thought it was read-only. Oops. Looks like tg3 and natsemi are over-writing the magic number field and then checking it in user-space.

The fix is simple. Add the ethtool_eeprom copy back. I will test and send out a patch shortly.

Thanks much for finding this:)

Regards,
Mandeep

Quoted 51 lines · Breno Leitao

Mandeep,
Quoted 5 lines · Mandeep Singh Baines
I suspect you may have mis-applied the patch. The patch changes both set_eeprom and get_eeprom. I think you may have modified get_eeprom to call ops->set_eeprom instead of ops->get_eeprom. If not, please send me ethtool.c
and tg3.c and I can take a look.
Well, IMHO the patch is correctly applied as I could see. I just got the David's tree and run a diff between the ethtool.c files (from David's tree and mine), and I get the following differences. Note that I just add a printk() there to "debug" what was going on.
static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
{
       struct ethtool_eeprom eeprom;
(quoted diff) +2 -1
-       const struct ethtool_ops *ops = dev->ethtool_ops;
+       struct ethtool_ops *ops = dev->ethtool_ops;
       void __user *userbuf = useraddr + sizeof(eeprom);
       u32 bytes_remaining;
       u8 *data;
       int ret = 0;
 
+       printk("<1> ethtool_get_eeprom()\n");
       if (!ops->get_eeprom || !ops->get_eeprom_len)
               return -EOPNOTSUPP;
@@ -330,12 +330,13 @@ static int ethtool_get_eeprom(struct net
static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
{
       struct ethtool_eeprom eeprom;
(quoted diff) +2 -1
-       const struct ethtool_ops *ops = dev->ethtool_ops;
+       struct ethtool_ops *ops = dev->ethtool_ops;
       void __user *userbuf = useraddr + sizeof(eeprom);
       u32 bytes_remaining;
       u8 *data;
       int ret = 0;
 
+       printk("<1> ethtool_set_eeprom()\n");
       if (!ops->set_eeprom || !ops->get_eeprom_len)
               return -EOPNOTSUPP;
The patched ethtool.c is at http://rafb.net/p/fWXNwk28.html and tg3.c is at http://rapidshare.com/files/110139941/tg3.c.html
--
Breno Leitão
[email protected]

Mandeep Singh Baines

author

2008-04-15 17:40 UTC #13 lore raw

In reply to David Miller

Quoted 13 lines · David Miller

From: Mandeep Singh Baines <[email protected]>
Date: Mon, 14 Apr 2008 11:03:38 -0700
Quoted 10 lines
Currently, it is not possible to read/write to an eeprom larger than 128k in size because the buffer used for temporarily storing the eeprom contents is allocated using kmalloc. kmalloc can only allocate a maximum of 128k depending on architecture.
Modified ethtool_get/set_eeprom to only allocate a page of memory and then copy the eeprom a page at a time.
Updated original patch as per suggestions from Joe Perches.

Quoted 8 lines

Quoted 1 line
Signed-off-by Mandeep Singh Baines <[email protected]>
This looks fine on the surface.
But I wonder what we can do if we have some EEPROM implementation that can only write the whole time at once, and thus will fail if you try to do it in pieces? Do we have such a case?

I suspect such a case does not exist but can't say for certain. Currently, the only user-space application I'm aware of which makes use of the set_eeprom ioctl interface is ethtool. It currently only supports single byte writes. Hence, any device which supports set_eeprom will likely also support single byte writing.

To test my changes, I had to modify the ethtool application to support writing an arbitrary number of bytes to eeprom. This makes it possible to update an eeprom from a binary file. To do so, I use ethtool as follows:

# ethtool -E eth1 < eeprom.bin

I plan on sending my ethtool changes to the maintainers once this patch is accepted.

Quoted 1 line

If so this new code could cause regressions.

An alternative would be to vmalloc the entire size to avoid the potential regression.

David Miller

maintainer Applied

2008-04-16 02:24 UTC #14 lore raw

In reply to Mandeep Singh Baines

From: Mandeep Singh Baines <[email protected]>
Date: Mon, 14 Apr 2008 11:03:38 -0700

Quoted 11 lines

Currently, it is not possible to read/write to an eeprom larger than 128k in size because the buffer used for temporarily storing the eeprom contents is allocated using kmalloc. kmalloc can only allocate a maximum of 128k depending on architecture.
Modified ethtool_get/set_eeprom to only allocate a page of memory and then copy the eeprom a page at a time.
Updated original patch as per suggestions from Joe Perches.
Signed-off-by Mandeep Singh Baines <[email protected]>

Applied, thanks.