On Thu, Oct 30, 2014 at 03:39:51PM -0700, Guenter Roeck wrote:

On Thu, Oct 30, 2014 at 10:11:31PM +0100, Andrew Lunn wrote:

quoted

quoted

+static int dsa_slave_get_eeprom_len(struct net_device *dev)
+{
+	struct dsa_slave_priv *p = netdev_priv(dev);
+	struct dsa_switch *ds = p->parent;
+
+	if (ds->pd->eeprom_len)
+		return ds->pd->eeprom_len;
+
+	if (ds->drv->get_eeprom_len)
+		return ds->drv->get_eeprom_len(ds);
+
+	return 0;
+}
+

Hi Guenter

I just started doing some testing with this patchset. A bit late since
David just accepted it, but...

root@dir665:~# ethtool -e lan4
Cannot get EEPROM data: Invalid argument
root@dir665:~# ethtool -e eth0
Cannot get EEPROM data: Operation not supported

There is no eeprom for the hardware i'm testing. Operation not
supported seems like a better error code and Invalid argument, and is
what other network drivers i tried returned.

Hi Andrew,

I think the problem is that the infrastructure code (net/core/ethtool.c)
does not accept an error from the get_eeprom_len function, but instead
assumes that reporting eeprom data is supported if a driver provides
the access functions. The get_eeprom_len function returns 0 in your case,
which in ethtool_get_any_eeprom() translates to -EINVAL because user space
either requests no data or more data than available. I wonder why user
space requests anything in the first place; I would have assumed that it
reads the driver information first and is told that the eeprom length is 0,
but I guess that is a different question.

I quickly browsed through a couple of other drivers supporting get_eprom_len,
and they all return 0 if there is no eeprom. Doesn't that mean that they all
end up reporting -EINVAL if an attempt is made to read the eeprom ?

The only solution that comes to my mind would be to have the infrastructure
code check the return value from get_eeprom_len and return -EOPNOTSUPP
if the reported eeprom length is 0. That would be an infrastructure change,
though. Does that sound reasonable, or do you have a better idea ?

In parallel, I'll have a look into the ethtool command to see why it
requests eeprom data even though the reported eeprom length is 0.

As suspected, ethtool will attempt to read a zero-length eeprom.

The following patch should solve the problem. Not sure if it is worth it,
though, since this will change behavior for existing drivers.

Thanks,
Guenter

---
From: Guenter Roeck <linux@roeck-us.net>
Date: Thu, 30 Oct 2014 17:51:34 -0700
Subject: [RFC PATCH] net: ethtool: Return -EOPNOTSUPP if user space tries to read
 EEPROM with lengh 0

If a driver supports reading EEPROM but no EEPROM is installed in the system,
the driver's get_eeprom_len function will return 0. ethtool will subsequently
try to read that zero-length EEPROM anyway. If the driver does not support
EEPROM access at all, this operation will return -EOPNOTSUPP. If the driver
does support EEPROM access but no EEPROM is installed, the operation will
return -EINVAL. Return -EOPNOTSUPP in both cases for consistency.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 net/core/ethtool.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1600aa2..06dfb29 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c

@@ -1036,7 +1036,8 @@ static int ethtool_get_eeprom(struct net_device *dev, void __user *useraddr)
 {
 	const struct ethtool_ops *ops = dev->ethtool_ops;
 
-	if (!ops->get_eeprom || !ops->get_eeprom_len)
+	if (!ops->get_eeprom || !ops->get_eeprom_len ||
+	    !ops->get_eeprom_len(dev))
 		return -EOPNOTSUPP;
 
 	return ethtool_get_any_eeprom(dev, useraddr, ops->get_eeprom,

@@ -1052,7 +1053,8 @@ static int ethtool_set_eeprom(struct net_device *dev, void __user *useraddr)
 	u8 *data;
 	int ret = 0;
 
-	if (!ops->set_eeprom || !ops->get_eeprom_len)
+	if (!ops->set_eeprom || !ops->get_eeprom_len ||
+	    !ops->get_eeprom_len(dev))
 		return -EOPNOTSUPP;
 
 	if (copy_from_user(&eeprom, useraddr, sizeof(eeprom)))

-- 
1.9.1