| Message ID | 20110720091850.32210.2059.stgit@localhost6.localdomain6 (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, 2011-07-20 at 12:18 +0300, Kalle Valo wrote: > cfg80211 exports zero length register size as it currently only uses > struct ethtool_regs.version to export struct wiphy.hw_version. [...] The ethtool_regs::version field represents the version of the register dump format. This may or may not relate to a hardware version. If you don't actually provide a register dump then don't implement this operation. Ben.
On 07/20/2011 02:38 PM, Ben Hutchings wrote: > On Wed, 2011-07-20 at 12:18 +0300, Kalle Valo wrote: >> cfg80211 exports zero length register size as it currently only uses >> struct ethtool_regs.version to export struct wiphy.hw_version. > [...] > > The ethtool_regs::version field represents the version of the register > dump format. This may or may not relate to a hardware version. > > If you don't actually provide a register dump then don't implement this > operation. Then we have a problem as cfg80211 exports the hw version without any register dumps: static int cfg80211_get_regs_len(struct net_device *dev) { /* For now, return 0... */ return 0; } static void cfg80211_get_regs(struct net_device *dev, struct ethtool_regs *regs, void *data) { struct wireless_dev *wdev = dev->ieee80211_ptr; regs->version = wdev->wiphy->hw_version; regs->len = 0; } And this has been there a long time already. How cfg80211 should export hw version if this is not a proper way? Kalle -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jul 20, 2011 at 02:58:20PM +0300, Kalle Valo wrote: > On 07/20/2011 02:38 PM, Ben Hutchings wrote: > > On Wed, 2011-07-20 at 12:18 +0300, Kalle Valo wrote: > >> cfg80211 exports zero length register size as it currently only uses > >> struct ethtool_regs.version to export struct wiphy.hw_version. > > [...] > > > > The ethtool_regs::version field represents the version of the register > > dump format. This may or may not relate to a hardware version. This seems like a strange claim to make...? struct ethtool_regs { __u32 cmd; __u32 version; /* driver-specific, indicates different chips/revs */ __u32 len; /* bytes */ __u8 data[0]; }; That "indicates different chips/revs" comment has been there at least as long as the kernel has been in git (back to the 2.6.12 era). > > If you don't actually provide a register dump then don't implement this > > operation. > > Then we have a problem as cfg80211 exports the hw version without any > register dumps: > > static int cfg80211_get_regs_len(struct net_device *dev) > { > /* For now, return 0... */ > return 0; > } > > static void cfg80211_get_regs(struct net_device *dev, struct > ethtool_regs *regs, > void *data) > { > struct wireless_dev *wdev = dev->ieee80211_ptr; > > regs->version = wdev->wiphy->hw_version; > regs->len = 0; > } > > And this has been there a long time already. How cfg80211 should export > hw version if this is not a proper way? The ethool binary already has support for the at76c50x_usb driver, which uses this very mechanism in exactly this way. I know this worked previously, although I don't know what might have changed to break it...? John
On Wed, 2011-07-20 at 10:36 -0400, John W. Linville wrote: > On Wed, Jul 20, 2011 at 02:58:20PM +0300, Kalle Valo wrote: > > On 07/20/2011 02:38 PM, Ben Hutchings wrote: > > > On Wed, 2011-07-20 at 12:18 +0300, Kalle Valo wrote: > > >> cfg80211 exports zero length register size as it currently only uses > > >> struct ethtool_regs.version to export struct wiphy.hw_version. > > > [...] > > > > > > The ethtool_regs::version field represents the version of the register > > > dump format. This may or may not relate to a hardware version. > > This seems like a strange claim to make...? > > struct ethtool_regs { > __u32 cmd; > __u32 version; /* driver-specific, indicates different chips/revs */ > __u32 len; /* bytes */ > __u8 data[0]; > }; > > That "indicates different chips/revs" comment has been there at least > as long as the kernel has been in git (back to the 2.6.12 era). Well, it is most importantly *driver-specific*. > > > If you don't actually provide a register dump then don't implement this > > > operation. > > > > Then we have a problem as cfg80211 exports the hw version without any > > register dumps: > > > > static int cfg80211_get_regs_len(struct net_device *dev) > > { > > /* For now, return 0... */ > > return 0; > > } > > > > static void cfg80211_get_regs(struct net_device *dev, struct > > ethtool_regs *regs, > > void *data) > > { > > struct wireless_dev *wdev = dev->ieee80211_ptr; > > > > regs->version = wdev->wiphy->hw_version; > > regs->len = 0; > > } > > > > And this has been there a long time already. How cfg80211 should export > > hw version if this is not a proper way? > > The ethool binary already has support for the at76c50x_usb driver, > which uses this very mechanism in exactly this way. I know this > worked previously, although I don't know what might have changed to > break it...? This is due to: commit a77f5db361ed9953b5b749353ea2c7fed2bf8d93 Author: Ben Hutchings <bhutchings@solarflare.com> Date: Mon Sep 20 08:42:17 2010 +0000 ethtool: Allocate register dump buffer with vmalloc() kmalloc() returns a non-null pointer for size=0 but vmalloc() doesn't. I was unaware that some drivers would (ab)use this operation to export only hardware revision. Given that they do, I suppose this must be made to work again - either using Kalle's fix or the one following this. Ben.
diff --git a/net/core/ethtool.c b/net/core/ethtool.c index fd14116..6f073f4 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -1213,7 +1213,7 @@ static int ethtool_get_regs(struct net_device *dev, char __user *useraddr) { struct ethtool_regs regs; const struct ethtool_ops *ops = dev->ethtool_ops; - void *regbuf; + void *regbuf = NULL; int reglen, ret; if (!ops->get_regs || !ops->get_regs_len) @@ -1226,18 +1226,24 @@ static int ethtool_get_regs(struct net_device *dev, char __user *useraddr) if (regs.len > reglen) regs.len = reglen; - regbuf = vzalloc(reglen); - if (!regbuf) - return -ENOMEM; + if (reglen > 0) { + regbuf = vzalloc(reglen); + if (!regbuf) + return -ENOMEM; + } ops->get_regs(dev, ®s, regbuf); ret = -EFAULT; if (copy_to_user(useraddr, ®s, sizeof(regs))) goto out; - useraddr += offsetof(struct ethtool_regs, data); - if (copy_to_user(useraddr, regbuf, regs.len)) - goto out; + + if (regs.len > 0) { + useraddr += offsetof(struct ethtool_regs, data); + if (copy_to_user(useraddr, regbuf, regs.len)) + goto out; + } + ret = 0; out: