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: > > > +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 <li...@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 <li...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/