[PATCH] [ETHTOOL]: Add support for large eeproms

[PATCH] [ETHTOOL]: Add support for large eeproms

[Mandeep Singh Baines]

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 <msb@google.com>
---
 net/core/ethtool.c |   57 ++++++++++++++++++++++++++++-----------------------
 1 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1163eb2..54f1004 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -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,25 @@ 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;
+		if ((ret = ops->get_eeprom(dev, &eeprom, data)))
+			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,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;
 
 	if (!ops->set_eeprom || !ops->get_eeprom_len)
 		return -EOPNOTSUPP;
@@ -346,22 +349,24 @@ 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;
+	bytes_remaining = eeprom.len;
+	while (bytes_remaining > 0) {
+		eeprom.len = min(bytes_remaining, (u32)PAGE_SIZE);
 
-	ret = ops->set_eeprom(dev, &eeprom, data);
-	if (ret)
-		goto out;
-
-	if (copy_to_user(useraddr + sizeof(eeprom), data, eeprom.len))
 		ret = -EFAULT;
+		if (copy_from_user(data, userbuf, eeprom.len))
+			break;
+		if ((ret = ops->set_eeprom(dev, &eeprom, data)))
+			break;
+		userbuf += eeprom.len;
+		eeprom.offset += eeprom.len;
+		bytes_remaining -= eeprom.len;
+	}
 
- out:
 	kfree(data);
 	return ret;
 }
-- 
1.5.2.5

Re: [PATCH] [ETHTOOL]: Add support for large eeproms

[Mandeep Singh Baines]

Thanks Joe,

Fixed and updated patch. Please take another look.

Regards,
Mandeep

Joe Perches (joe@perches.com) wrote:
> On Wed, 2008-04-02 at 19:12 -0700, Mandeep Singh Baines wrote:
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index 1163eb2..54f1004 100644
> 
> please run scripts/checkpatch.pl
> 

Fixed

> > @@ -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 <msb@google.com>
---
 net/core/ethtool.c |   57 +++++++++++++++++++++++++++++----------------------
 1 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1163eb2..45d5f35 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -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;
 }
-- 
1.5.2.5

Re: [PATCH] [ETHTOOL]: Add support for large eeproms

[Joe Perches]

On Thu, 2008-04-03 at 11:00 -0700, Mandeep Singh Baines wrote:
> 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(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1163eb2..a29b43d 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -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;
 }

Re: [PATCH] [ETHTOOL]: Add support for large eeproms

[Mandeep Singh Baines]

Joe Perches (joe@perches.com) wrote:
> 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.

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

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

> cheers, Joe
> 
>  net/core/ethtool.c |   68 +++++++++++++++++++++++++++++-----------------------
>  1 files changed, 38 insertions(+), 30 deletions(-)
> 
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 1163eb2..a29b43d 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -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;
>  }
> 
> 

Re: [PATCH] [ETHTOOL]: Add support for large eeproms

[Jeff Garzik]

Mandeep Singh Baines wrote:
> Joe Perches (joe@perches.com) wrote:
>> 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.
> 
>> 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:)
> 
>> 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.
> 
>> cheers, Joe
>>
>>  net/core/ethtool.c |   68 +++++++++++++++++++++++++++++-----------------------
>>  1 files changed, 38 insertions(+), 30 deletions(-)

someone wanna resend with proper sign-offs?

[PATCH] [ETHTOOL]: Add support for large eeproms

[Mandeep Singh Baines]

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 <msb@google.com>
---
 net/core/ethtool.c |   64 +++++++++++++++++++++++++++++----------------------
 1 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1163eb2..a29b43d 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -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;
 }
-- 
1.5.2.5

Re: [PATCH] [ETHTOOL]: Add support for large eeproms

[David Miller]

From: Mandeep Singh Baines <msb@google.com>
Date: Mon, 14 Apr 2008 11:03:38 -0700

> 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 <msb@google.com>

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.

Re: [PATCH] [ETHTOOL]: Add support for large eeproms

[Tim Hockin]

Hey Dave, long time!

On Tue, Apr 15, 2008 at 12:31 AM, David Miller <davem@davemloft.net> wrote:
>
>  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

Re: [PATCH] [ETHTOOL]: Add support for large eeproms

[David Miller]

From: "Tim Hockin" <thockin@google.com>
Date: Tue, 15 Apr 2008 10:07:21 -0700

> On Tue, Apr 15, 2008 at 12:31 AM, David Miller <davem@davemloft.net> wrote:
> >
> >  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.

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

Re: [PATCH] [ETHTOOL]: Add support for large eeproms

[Breno Leitao]

David Miller wrote:
> From: "Tim Hockin" <thockin@google.com>
> Date: Tue, 15 Apr 2008 10:07:21 -0700
>
>   
>> 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
leitao@linux.vnet.ibm.com

Re: [PATCH] [ETHTOOL]: Add support for large eeproms

[Mandeep Singh Baines]

Hi Breno,

Comments are inline.

Breno Leitao (leitao@linux.vnet.ibm.com) wrote:
> 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().

> 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

Re: [PATCH] [ETHTOOL]: Add support for large eeproms

[Breno Leitao]

Mandeep,

Mandeep Singh Baines wrote:
> 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
leitao@linux.vnet.ibm.com

Re: [PATCH] [ETHTOOL]: Add support for large eeproms

[Mandeep Singh Baines]

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

Breno Leitao (leitao@linux.vnet.ibm.com) wrote:
> Mandeep,
> 
> Mandeep Singh Baines wrote:
> >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
> leitao@linux.vnet.ibm.com

Re: [PATCH] [ETHTOOL]: Add support for large eeproms

[Mandeep Singh Baines]

David Miller (davem@davemloft.net) wrote:
> From: Mandeep Singh Baines <msb@google.com>
> Date: Mon, 14 Apr 2008 11:03:38 -0700
> 
> > 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 <msb@google.com>
> 
> 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.

> If so this new code could cause regressions.

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

Re: [PATCH] [ETHTOOL]: Add support for large eeproms

[David Miller]

From: Mandeep Singh Baines <msb@google.com>
Date: Mon, 14 Apr 2008 11:03:38 -0700

> 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 <msb@google.com>

Applied, thanks.