[PATCH 0/2] cfg80211: firmware and hardware version

[PATCH 0/2] cfg80211: firmware and hardware version

[Kalle Valo]

Here's my suggestion how to provide firmware and hardware version to
user space. First I was thinking adding a new nl80211 command and
it looked so ugly that I decided include the versions in struct wiphy
instead.

Please comment.

---

Kalle Valo (2):
      at76c50x-usb: set firmware and hardware version in wiphy
      cfg80211: add firmware and hardware version to wiphy


 drivers/net/wireless/at76c50x-usb.c |   15 +++++++++++++++
 include/linux/nl80211.h             |    3 +++
 include/net/cfg80211.h              |    5 +++++
 net/wireless/nl80211.c              |   11 +++++++++++
 4 files changed, 34 insertions(+), 0 deletions(-)

[PATCH 1/2] cfg80211: add firmware and hardware version to wiphy

[Kalle Valo]

From: Kalle Valo <kalle.valo@nokia.com>

It's useful to provide firmware and hardware version to user space and have a
generic interface to retrieve them. Users can provide the version information
in bug reports etc.

Add fields for firmware and hardware version to struct wiphy and return
them to user space in NL80211_CMD_GET_WIPHY reply.

Signed-off-by: Kalle Valo <kalle.valo@nokia.com>
---

 include/linux/nl80211.h |    3 +++
 include/net/cfg80211.h  |    5 +++++
 net/wireless/nl80211.c  |   11 +++++++++++
 3 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/include/linux/nl80211.h b/include/linux/nl80211.h
index a8d71ed..6d6651f 100644
--- a/include/linux/nl80211.h
+++ b/include/linux/nl80211.h
@@ -714,6 +714,9 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_PID,
 
+	NL80211_ATTR_FW_VERSION,
+	NL80211_ATTR_HW_VERSION,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 3d874c6..de3da19 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1070,6 +1070,8 @@ struct cfg80211_ops {
  * and registration/helper functions
  */
 
+#define CFG80211_VERSION_LEN 32
+
 /**
  * struct wiphy - wireless hardware description
  * @idx: the wiphy index assigned to this item
@@ -1142,6 +1144,9 @@ struct wiphy {
 	u32 frag_threshold;
 	u32 rts_threshold;
 
+	char fw_version[CFG80211_VERSION_LEN];
+	char hw_version[CFG80211_VERSION_LEN];
+
 	/* If multiple wiphys are registered and you're handed e.g.
 	 * a regular netdev with assigned ieee80211_ptr, you won't
 	 * know whether it points to a wiphy your driver has registered
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index eddab09..9e2214e 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -138,6 +138,14 @@ static struct nla_policy nl80211_policy[NL80211_ATTR_MAX+1] __read_mostly = {
 	[NL80211_ATTR_CIPHER_SUITE_GROUP] = { .type = NLA_U32 },
 	[NL80211_ATTR_WPA_VERSIONS] = { .type = NLA_U32 },
 	[NL80211_ATTR_PID] = { .type = NLA_U32 },
+	[NL80211_ATTR_FW_VERSION] = {
+		.type = NLA_NUL_STRING,
+		.len = CFG80211_VERSION_LEN,
+	},
+	[NL80211_ATTR_HW_VERSION] = {
+		.type = NLA_NUL_STRING,
+		.len = CFG80211_VERSION_LEN,
+	},
 };
 
 /* policy for the attributes */
@@ -420,6 +428,9 @@ static int nl80211_send_wiphy(struct sk_buff *msg, u32 pid, u32 seq, int flags,
 	NLA_PUT_U32(msg, NL80211_ATTR_WIPHY_RTS_THRESHOLD,
 		    dev->wiphy.rts_threshold);
 
+	NLA_PUT_STRING(msg, NL80211_ATTR_FW_VERSION, dev->wiphy.fw_version);
+	NLA_PUT_STRING(msg, NL80211_ATTR_HW_VERSION, dev->wiphy.hw_version);
+
 	NLA_PUT_U8(msg, NL80211_ATTR_MAX_NUM_SCAN_SSIDS,
 		   dev->wiphy.max_scan_ssids);
 	NLA_PUT_U16(msg, NL80211_ATTR_MAX_SCAN_IE_LEN,

[PATCH 2/2] at76c50x-usb: set firmware and hardware version in wiphy

[Kalle Valo]

Set firmware and hardware version in wiphy so that user space can access
it.

Signed-off-by: Kalle Valo <kalle.valo@iki.fi>
---

 drivers/net/wireless/at76c50x-usb.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index 8e1a55d..b6de657 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -2217,6 +2217,8 @@ static struct ieee80211_supported_band at76_supported_band = {
 static int at76_init_new_device(struct at76_priv *priv,
 				struct usb_interface *interface)
 {
+	struct wiphy *wiphy;
+	size_t len;
 	int ret;
 
 	/* set up the endpoint information */
@@ -2254,6 +2256,7 @@ static int at76_init_new_device(struct at76_priv *priv,
 	priv->device_unplugged = 0;
 
 	/* mac80211 initialisation */
+	wiphy = priv->hw->wiphy;
 	priv->hw->wiphy->max_scan_ssids = 1;
 	priv->hw->wiphy->max_scan_ie_len = 0;
 	priv->hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION);
@@ -2265,6 +2268,18 @@ static int at76_init_new_device(struct at76_priv *priv,
 	SET_IEEE80211_DEV(priv->hw, &interface->dev);
 	SET_IEEE80211_PERM_ADDR(priv->hw, priv->mac_addr);
 
+	len = sizeof(wiphy->fw_version);
+	snprintf(wiphy->fw_version, len, "%d.%d.%d-%d",
+		 priv->fw_version.major, priv->fw_version.minor,
+		 priv->fw_version.patch, priv->fw_version.build);
+
+	len = sizeof(wiphy->hw_version);
+	snprintf(wiphy->hw_version, len, "%d", priv->board_type);
+
+	/* null terminate the strings in case they were truncated */
+	wiphy->fw_version[len - 1] = '\0';
+	wiphy->hw_version[len - 1] = '\0';
+
 	ret = ieee80211_register_hw(priv->hw);
 	if (ret) {
 		printk(KERN_ERR "cannot register mac80211 hw (status %d)!\n",

[PATCH 1/2] iw: update nl80211.h from wireless-testing

[Kalle Valo]

---

 nl80211.h |   26 +++++++++++++++++++++-----
 1 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/nl80211.h b/nl80211.h
index 962e223..a8d71ed 100644
--- a/nl80211.h
+++ b/nl80211.h
@@ -262,6 +262,9 @@
  *	reasons, for this the %NL80211_ATTR_DISCONNECTED_BY_AP and
  *	%NL80211_ATTR_REASON_CODE attributes are used.
  *
+ * @NL80211_CMD_SET_WIPHY_NETNS: Set a wiphy's netns. Note that all devices
+ *	associated with this wiphy must be down and will follow.
+ *
  * @NL80211_CMD_MAX: highest used command number
  * @__NL80211_CMD_AFTER_LAST: internal use
  */
@@ -336,6 +339,8 @@ enum nl80211_commands {
 	NL80211_CMD_ROAM,
 	NL80211_CMD_DISCONNECT,
 
+	NL80211_CMD_SET_WIPHY_NETNS,
+
 	/* add new commands above here */
 
 	/* used to define NL80211_CMD_MAX below */
@@ -475,10 +480,6 @@ enum nl80211_commands {
  * @NL80211_ATTR_SCAN_FREQUENCIES: nested attribute with frequencies (in MHz)
  * @NL80211_ATTR_SCAN_SSIDS: nested attribute with SSIDs, leave out for passive
  *	scanning and include a zero-length SSID (wildcard) for wildcard scan
- * @NL80211_ATTR_SCAN_GENERATION: the scan generation increases whenever the
- *	scan result list changes (BSS expired or added) so that applications
- *	can verify that they got a single, consistent snapshot (when all dump
- *	messages carried the same generation number)
  * @NL80211_ATTR_BSS: scan result BSS
  *
  * @NL80211_ATTR_REG_INITIATOR: indicates who requested the regulatory domain
@@ -573,6 +574,16 @@ enum nl80211_commands {
  *	and join_ibss(), key information is in a nested attribute each
  *	with %NL80211_KEY_* sub-attributes
  *
+ * @NL80211_ATTR_PID: Process ID of a network namespace.
+ *
+ * @NL80211_ATTR_GENERATION: Used to indicate consistent snapshots for
+ *	dumps. This number increases whenever the object list being
+ *	dumped changes, and as such userspace can verify that it has
+ *	obtained a complete and consistent snapshot by verifying that
+ *	all dump messages contain the same generation number. If it
+ *	changed then the list changed and the dump should be repeated
+ *	completely from scratch.
+ *
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
  */
@@ -644,7 +655,7 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_SCAN_FREQUENCIES,
 	NL80211_ATTR_SCAN_SSIDS,
-	NL80211_ATTR_SCAN_GENERATION,
+	NL80211_ATTR_GENERATION, /* replaces old SCAN_GENERATION */
 	NL80211_ATTR_BSS,
 
 	NL80211_ATTR_REG_INITIATOR,
@@ -701,12 +712,17 @@ enum nl80211_attrs {
 	NL80211_ATTR_KEY,
 	NL80211_ATTR_KEYS,
 
+	NL80211_ATTR_PID,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
 	NL80211_ATTR_MAX = __NL80211_ATTR_AFTER_LAST - 1
 };
 
+/* source-level API compatibility */
+#define NL80211_ATTR_SCAN_GENERATION NL80211_ATTR_GENERATION
+
 /*
  * Allow user space programs to use #ifdef on new attributes by defining them
  * here

[PATCH 2/2] iw: print firmware and hardware version

[Kalle Valo]

struct wiphy now contains firmware and hardware version, print that
information to the user.
---

 info.c    |   15 +++++++++++++++
 nl80211.h |    3 +++
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/info.c b/info.c
index 7bca69d..ae3ed54 100644
--- a/info.c
+++ b/info.c
@@ -66,6 +66,7 @@ static int print_phy_handler(struct nl_msg *msg, void *arg)
 	struct nlattr *nl_freq;
 	struct nlattr *nl_rate;
 	struct nlattr *nl_mode;
+	const char *str;
 	int bandidx = 1;
 	int rem_band, rem_freq, rem_rate, rem_mode;
 	int open;
@@ -263,6 +264,20 @@ static int print_phy_handler(struct nl_msg *msg, void *arg)
 			printf("\tRTS threshold: %d\n", rts);
 	}
 
+	if (tb_msg[NL80211_ATTR_FW_VERSION]) {
+		str = nla_get_string(tb_msg[NL80211_ATTR_FW_VERSION]);
+		if (strlen(str) == 0)
+			str = "<unknown>";
+		printf("\tFirmware version: %s\n", str);
+	}
+
+	if (tb_msg[NL80211_ATTR_HW_VERSION]) {
+		str = nla_get_string(tb_msg[NL80211_ATTR_HW_VERSION]);
+		if (strlen(str) == 0)
+			str = "<unknown>";
+		printf("\tHardware version: %s\n", str);
+	}
+
 	if (!tb_msg[NL80211_ATTR_SUPPORTED_IFTYPES])
 		return NL_SKIP;
 
diff --git a/nl80211.h b/nl80211.h
index a8d71ed..6d6651f 100644
--- a/nl80211.h
+++ b/nl80211.h
@@ -714,6 +714,9 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_PID,
 
+	NL80211_ATTR_FW_VERSION,
+	NL80211_ATTR_HW_VERSION,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,

Re: [PATCH 1/2] cfg80211: add firmware and hardware version to wiphy

[Luis R. Rodriguez]

On Thu, Sep 24, 2009 at 11:02 AM, Kalle Valo <kalle.valo@iki.fi> wrote:
> From: Kalle Valo <kalle.valo@nokia.com>
>
> It's useful to provide firmware and hardware version to user space and have a
> generic interface to retrieve them. Users can provide the version information
> in bug reports etc.
>
> Add fields for firmware and hardware version to struct wiphy and return
> them to user space in NL80211_CMD_GET_WIPHY reply.

Wow that was quick :)

> Signed-off-by: Kalle Valo <kalle.valo@nokia.com>
> ---
>
>  include/linux/nl80211.h |    3 +++
>  include/net/cfg80211.h  |    5 +++++
>  net/wireless/nl80211.c  |   11 +++++++++++
>  3 files changed, 19 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/nl80211.h b/include/linux/nl80211.h
> index a8d71ed..6d6651f 100644
> --- a/include/linux/nl80211.h
> +++ b/include/linux/nl80211.h
> @@ -714,6 +714,9 @@ enum nl80211_attrs {
>
>        NL80211_ATTR_PID,
>
> +       NL80211_ATTR_FW_VERSION,
> +       NL80211_ATTR_HW_VERSION,
> +

Some kdoc on this would be nice.

>        /* add attributes here, update the policy in nl80211.c */
>
>        __NL80211_ATTR_AFTER_LAST,
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 3d874c6..de3da19 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -1070,6 +1070,8 @@ struct cfg80211_ops {
>  * and registration/helper functions
>  */
>
> +#define CFG80211_VERSION_LEN 32

Probably best to just remove this or at least make this not just
CFG80211_VERSION_LEN, seems like this is related to cfg80211's version
somehow.

  Luis

Re: [PATCH 2/2] at76c50x-usb: set firmware and hardware version in wiphy

[Luis R. Rodriguez]

On Thu, Sep 24, 2009 at 11:02 AM, Kalle Valo <kalle.valo@iki.fi> wrote:
> Set firmware and hardware version in wiphy so that user space can access
> it.
>
> Signed-off-by: Kalle Valo <kalle.valo@iki.fi>
> ---
>
>  drivers/net/wireless/at76c50x-usb.c |   15 +++++++++++++++
>  1 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
> index 8e1a55d..b6de657 100644
> --- a/drivers/net/wireless/at76c50x-usb.c
> +++ b/drivers/net/wireless/at76c50x-usb.c
> @@ -2217,6 +2217,8 @@ static struct ieee80211_supported_band at76_supported_band = {
>  static int at76_init_new_device(struct at76_priv *priv,
>                                struct usb_interface *interface)
>  {
> +       struct wiphy *wiphy;
> +       size_t len;
>        int ret;
>
>        /* set up the endpoint information */
> @@ -2254,6 +2256,7 @@ static int at76_init_new_device(struct at76_priv *priv,
>        priv->device_unplugged = 0;
>
>        /* mac80211 initialisation */
> +       wiphy = priv->hw->wiphy;
>        priv->hw->wiphy->max_scan_ssids = 1;
>        priv->hw->wiphy->max_scan_ie_len = 0;
>        priv->hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION);
> @@ -2265,6 +2268,18 @@ static int at76_init_new_device(struct at76_priv *priv,
>        SET_IEEE80211_DEV(priv->hw, &interface->dev);
>        SET_IEEE80211_PERM_ADDR(priv->hw, priv->mac_addr);
>
> +       len = sizeof(wiphy->fw_version);
> +       snprintf(wiphy->fw_version, len, "%d.%d.%d-%d",
> +                priv->fw_version.major, priv->fw_version.minor,
> +                priv->fw_version.patch, priv->fw_version.build);
> +
> +       len = sizeof(wiphy->hw_version);
> +       snprintf(wiphy->hw_version, len, "%d", priv->board_type);

So ath9k and ath5k keep their own strings for such things, to name the
MAC/Baseband, and then the radio revision and subrevisions... What I'd
like to see documented on the kdoc for hw_version is what exactly is
expected to be put there.

The hw_version and fw_version seem to be helpful in providing more
information to userspace which you would not typically see -- things
you would only tend to see on a dmesg output so at least for that
purpose I think its nice. For example lspci won't really tell you the
exact hardware type on atheros chipsets, so this seems nice.

Anyway, getting some more clarification on the docs would be nice.

  Luis

Re: [PATCH 2/2] at76c50x-usb: set firmware and hardware version in wiphy

[Kalle Valo]

On Thu, Sep 24, 2009 at 11:35 AM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> So ath9k and ath5k keep their own strings for such things, to name the
> MAC/Baseband, and then the radio revision and subrevisions... What I'd
> like to see documented on the kdoc for hw_version is what exactly is
> expected to be put there.
>
> The hw_version and fw_version seem to be helpful in providing more
> information to userspace which you would not typically see -- things
> you would only tend to see on a dmesg output so at least for that
> purpose I think its nice. For example lspci won't really tell you the
> exact hardware type on atheros chipsets, so this seems nice.

Exactly, that's my idea here. And with this the information should be
easy to show in UI.

> Anyway, getting some more clarification on the docs would be nice.

I will definitely add documentation. I meant to send these patches RFC
for the implementation idea, but forgot to do it.

BTW, is there an easy way to get the module name for the interface?
That's also helpful information for the user.

Kalle

Re: [PATCH 1/2] cfg80211: add firmware and hardware version to wiphy

[Kalle Valo]

On Thu, Sep 24, 2009 at 11:32 AM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On Thu, Sep 24, 2009 at 11:02 AM, Kalle Valo <kalle.valo@iki.fi> wrote:
>> From: Kalle Valo <kalle.valo@nokia.com>
>>
>> It's useful to provide firmware and hardware version to user space and have a
>> generic interface to retrieve them. Users can provide the version information
>> in bug reports etc.
>>
>> Add fields for firmware and hardware version to struct wiphy and return
>> them to user space in NL80211_CMD_GET_WIPHY reply.
>
> Wow that was quick :)

Yeah, it's easy to hack here at Plumbers :D

>>        NL80211_ATTR_PID,
>>
>> +       NL80211_ATTR_FW_VERSION,
>> +       NL80211_ATTR_HW_VERSION,
>> +
>
> Some kdoc on this would be nice.

Definitely. I'll add it in v2 if the implementation is ok otherwise.

>>        /* add attributes here, update the policy in nl80211.c */
>>
>>        __NL80211_ATTR_AFTER_LAST,
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index 3d874c6..de3da19 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -1070,6 +1070,8 @@ struct cfg80211_ops {
>>  * and registration/helper functions
>>  */
>>
>> +#define CFG80211_VERSION_LEN 32
>
> Probably best to just remove this or at least make this not just
> CFG80211_VERSION_LEN, seems like this is related to cfg80211's version
> somehow.

The length is used four time so I would not want to remove it. Maybe
rename to CFG80211_FWHW_VERSION_LEN?

Kalle

Re: [PATCH 1/2] cfg80211: add firmware and hardware version to wiphy

[Luis R. Rodriguez]

On Thu, Sep 24, 2009 at 12:14 PM, Kalle Valo <kalle.valo@iki.fi> wrote:

> The length is used four time so I would not want to remove it. Maybe
> rename to CFG80211_FWHW_VERSION_LEN?

Sure.

  Luis

Re: [PATCH 2/2] at76c50x-usb: set firmware and hardware version in wiphy

[Luis R. Rodriguez]

On Thu, Sep 24, 2009 at 12:10 PM, Kalle Valo <kalle.valo@iki.fi> wrote:

> BTW, is there an easy way to get the module name for the interface?
> That's also helpful information for the user.

If you can map the interface to PCI ID then I think its possible,
lspci -k seems to do it. It would be a nice addition to iw output as
well IMHO.

  Luis

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[Luis R. Rodriguez]

On Thu, Sep 24, 2009 at 11:02 AM, Kalle Valo <kalle.valo@iki.fi> wrote:
> Here's my suggestion how to provide firmware and hardware version to
> user space. First I was thinking adding a new nl80211 command and
> it looked so ugly that I decided include the versions in struct wiphy
> instead.
>
> Please comment.

What was the conclusion on ethtool stuff again? I forgot.

  Luis

Re: [PATCH 2/2] at76c50x-usb: set firmware and hardware version in wiphy

[Joerg Albert]

On 09/24/2009 08:02 PM, Kalle Valo wrote:

> +	len = sizeof(wiphy->fw_version);
> +	snprintf(wiphy->fw_version, len, "%d.%d.%d-%d",
> +		 priv->fw_version.major, priv->fw_version.minor,
> +		 priv->fw_version.patch, priv->fw_version.build);
> +
> +	len = sizeof(wiphy->hw_version);
> +	snprintf(wiphy->hw_version, len, "%d", priv->board_type);
> +
> +	/* null terminate the strings in case they were truncated */
> +	wiphy->fw_version[len - 1] = '\0';
> +	wiphy->hw_version[len - 1] = '\0';

This only works as long as sizeof(wiphy->fw_version) == sizeof(wiphy->hw_version) - which is currently the case.
For sizeof(wiphy->fw_version) < sizeof(wiphy_hw_version) it overwrites memory behind wiphy->fw_version.

IMHO this is more robust against changes in the lengths of the char arrays:

+	wiphy->fw_version[sizeof(wiphy->fw_version) - 1] = '\0';
+	wiphy->hw_version[sizeof(wiphy->hw_version) - 1] = '\0';


Regards,
Jörg.

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[John W. Linville]

On Thu, Sep 24, 2009 at 01:20:35PM -0700, Luis R. Rodriguez wrote:
> On Thu, Sep 24, 2009 at 11:02 AM, Kalle Valo <kalle.valo@iki.fi> wrote:
> > Here's my suggestion how to provide firmware and hardware version to
> > user space. First I was thinking adding a new nl80211 command and
> > it looked so ugly that I decided include the versions in struct wiphy
> > instead.
> >
> > Please comment.
> 
> What was the conclusion on ethtool stuff again? I forgot.

IIRC, I suggested that the cfg80211 driver API (or just the wiphy
data structure) could be extended for appropriate bits like this,
then cfg80211 could catch ethtool operations in a way similar to how
it catches wireless extensions now.

I was thinking to look at this after I get home, maybe next week.
Others are welcome to beat me to it, of course. :-)

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[Kalle Valo]

On Thu, Sep 24, 2009 at 9:42 PM, John W. Linville
<linville@tuxdriver.com> wrote:
> On Thu, Sep 24, 2009 at 01:20:35PM -0700, Luis R. Rodriguez wrote:
>> On Thu, Sep 24, 2009 at 11:02 AM, Kalle Valo <kalle.valo@iki.fi> wrote:
>> > Here's my suggestion how to provide firmware and hardware version to
>> > user space. First I was thinking adding a new nl80211 command and
>> > it looked so ugly that I decided include the versions in struct wiphy
>> > instead.
>> >
>> > Please comment.
>>
>> What was the conclusion on ethtool stuff again? I forgot.
>
> IIRC, I suggested that the cfg80211 driver API (or just the wiphy
> data structure) could be extended for appropriate bits like this,
> then cfg80211 could catch ethtool operations in a way similar to how
> it catches wireless extensions now.

Oh, then I misunderstood our discussion at the summit, my
understanding was that we will use nl80211 anyway. Sorry about that.

But we want to export two strings to user space (at least for now), is
it really worth the effort to add ethtool support for such a minor
feature? Also I have understood that ethtool is implemented only for
ethernet drivers, I don't feel comfortable that we use ethernet driver
interfaces with 802.11 device drivers. They are so much different that
there isn't that much common functionality. That's why I prefer
nl80211 over ethtool.

What do people think?

Kalle

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[Luis R. Rodriguez]

On Fri, Sep 25, 2009 at 9:47 AM, Kalle Valo <kalle.valo@iki.fi> wrote:
> On Thu, Sep 24, 2009 at 9:42 PM, John W. Linville
> <linville@tuxdriver.com> wrote:
>> On Thu, Sep 24, 2009 at 01:20:35PM -0700, Luis R. Rodriguez wrote:
>>> On Thu, Sep 24, 2009 at 11:02 AM, Kalle Valo <kalle.valo@iki.fi> wrote:
>>> > Here's my suggestion how to provide firmware and hardware version to
>>> > user space. First I was thinking adding a new nl80211 command and
>>> > it looked so ugly that I decided include the versions in struct wiphy
>>> > instead.
>>> >
>>> > Please comment.
>>>
>>> What was the conclusion on ethtool stuff again? I forgot.
>>
>> IIRC, I suggested that the cfg80211 driver API (or just the wiphy
>> data structure) could be extended for appropriate bits like this,
>> then cfg80211 could catch ethtool operations in a way similar to how
>> it catches wireless extensions now.
>
> Oh, then I misunderstood our discussion at the summit, my
> understanding was that we will use nl80211 anyway. Sorry about that.
>
> But we want to export two strings to user space (at least for now), is
> it really worth the effort to add ethtool support for such a minor
> feature? Also I have understood that ethtool is implemented only for
> ethernet drivers, I don't feel comfortable that we use ethernet driver
> interfaces with 802.11 device drivers. They are so much different that
> there isn't that much common functionality. That's why I prefer
> nl80211 over ethtool.
>
> What do people think?

So for Wake-on-Wireless I ran into the same, ethtool just did not
offer the same wake up events needed for wireless. I could have
technically used ethtool and expanded it to support wireless but it
just seemed dirty.

I agree that using ethtool seems overkill compared to the patches you posted.

  Luis

Re: [PATCH 2/2] at76c50x-usb: set firmware and hardware version in wiphy

[Kalle Valo]

On Thu, Sep 24, 2009 at 2:13 PM, Joerg Albert <jal2@gmx.de> wrote:
> On 09/24/2009 08:02 PM, Kalle Valo wrote:
>
>> +     len = sizeof(wiphy->fw_version);
>> +     snprintf(wiphy->fw_version, len, "%d.%d.%d-%d",
>> +              priv->fw_version.major, priv->fw_version.minor,
>> +              priv->fw_version.patch, priv->fw_version.build);
>> +
>> +     len = sizeof(wiphy->hw_version);
>> +     snprintf(wiphy->hw_version, len, "%d", priv->board_type);
>> +
>> +     /* null terminate the strings in case they were truncated */
>> +     wiphy->fw_version[len - 1] = '\0';
>> +     wiphy->hw_version[len - 1] = '\0';
>
> This only works as long as sizeof(wiphy->fw_version) == sizeof(wiphy->hw_version) - which is currently the case.
> For sizeof(wiphy->fw_version) < sizeof(wiphy_hw_version) it overwrites memory behind wiphy->fw_version.

Good point, thanks for catching that.

> IMHO this is more robust against changes in the lengths of the char arrays:
>
> +       wiphy->fw_version[sizeof(wiphy->fw_version) - 1] = '\0';
> +       wiphy->hw_version[sizeof(wiphy->hw_version) - 1] = '\0';

Actually Christian pointed out that snprintf() always null terminates
the string and all this unnecessary. So I'll just remove this in v2.

Kalle

Re: [PATCH 2/2] at76c50x-usb: set firmware and hardware version in wiphy

[Kalle Valo]

On Thu, Sep 24, 2009 at 1:11 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On Thu, Sep 24, 2009 at 12:10 PM, Kalle Valo <kalle.valo@iki.fi> wrote:
>
>> BTW, is there an easy way to get the module name for the interface?
>> That's also helpful information for the user.
>
> If you can map the interface to PCI ID then I think its possible,
> lspci -k seems to do it.

Not all drivers use PCI, we have USB, SDIO, SPI and so on.

> It would be a nice addition to iw output as well IMHO.

I agree. I'm not going to work on adding the driver name right now,
but maybe in the future.

Kalle

Re: [PATCH 2/2] at76c50x-usb: set firmware and hardware version in wiphy

[Luis R. Rodriguez]

On Fri, Sep 25, 2009 at 12:11 PM, Kalle Valo <kalle.valo@iki.fi> wrote:
> On Thu, Sep 24, 2009 at 1:11 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
>> On Thu, Sep 24, 2009 at 12:10 PM, Kalle Valo <kalle.valo@iki.fi> wrote:
>>
>>> BTW, is there an easy way to get the module name for the interface?
>>> That's also helpful information for the user.
>>
>> If you can map the interface to PCI ID then I think its possible,
>> lspci -k seems to do it.
>
> Not all drivers use PCI, we have USB, SDIO, SPI and so on.

Sure, it was just an example of a transport where this does exist, I
am hoping this doesn't use PCI specific stuff and based on a simple
strace of "strace -o foo lspci -k -s 03:00.0"  I see it actually ends
up reading a  /lib/modules/ file and am hoping this is what it uses to
do the mapping. But you still first need the bus ID for the netdevice,
not sure how to best to do that. I do see the sysfs ieee80211 class
maps the phy%d to the bus:

mcgrof@tux ~ $ ls -ld /sys/class/ieee80211/phy0/device
lrwxrwxrwx 1 root root 0 2009-09-25 12:25
/sys/class/ieee80211/phy0/device -> ../../../0000:03:00.0

If you can extract the bus from here and then the device:vendor id I
think you can read the /lib/modules/ map file for the driver.

Check these files out:

ls /lib/modules/2.6.31-wl/modules.*map

  Luis

Re: [PATCH 2/2] at76c50x-usb: set firmware and hardware version in wiphy

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 232 bytes --]

On Fri, 2009-09-25 at 12:11 -0700, Kalle Valo wrote:

> I agree. I'm not going to work on adding the driver name right now,
> but maybe in the future.

$ readlink /sys/class/net/wlan0/phy80211/device/driver/module

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] at76c50x-usb: set firmware and hardware version in wiphy

[Kalle Valo]

On Sat, Sep 26, 2009 at 5:07 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2009-09-25 at 12:11 -0700, Kalle Valo wrote:
>
>> I agree. I'm not going to work on adding the driver name right now,
>> but maybe in the future.
>
> $ readlink /sys/class/net/wlan0/phy80211/device/driver/module

Excellent, that shows the driver name easily enough. Thanks.

Kalle

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[John W. Linville]

On Fri, Sep 25, 2009 at 09:53:35AM -0700, Luis R. Rodriguez wrote:
> On Fri, Sep 25, 2009 at 9:47 AM, Kalle Valo <kalle.valo@iki.fi> wrote:

> > But we want to export two strings to user space (at least for now), is
> > it really worth the effort to add ethtool support for such a minor
> > feature? Also I have understood that ethtool is implemented only for
> > ethernet drivers, I don't feel comfortable that we use ethernet driver
> > interfaces with 802.11 device drivers. They are so much different that
> > there isn't that much common functionality. That's why I prefer
> > nl80211 over ethtool.
> >
> > What do people think?
> 
> So for Wake-on-Wireless I ran into the same, ethtool just did not
> offer the same wake up events needed for wireless. I could have
> technically used ethtool and expanded it to support wireless but it
> just seemed dirty.
> 
> I agree that using ethtool seems overkill compared to the patches you posted.

I think you either overestimate the amount of trouble for implementing
(minimal) ethtool support or you underestimate the amount of
functionality available through that interface.  That, or you just
don't like using something named "eth"tool for wireless -- but hey,
let's be honest about the frames we send/receive to/from the kernel...
:-)

The ethtool interface provides functionality for viewing and modifying
eeprom contents, dumping registers, trigger self-tests, basic driver
info, getting and setting message reporting levels, external card
identification (hey, _could_ be useful!), and some other bits like
checksum offload that might(?) be useful in the future.  I understand
regarding the WoW vs. WoL issue but probably the answer is just to
add a new method for WoW...?

I'll post a patch series based on what Kalle sent but using ethtool
instead of nl80211...

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

[PATCH 1/3] wireless: implement basic ethtool support for cfg80211 devices

[John W. Linville]

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 net/wireless/Makefile  |    2 +-
 net/wireless/core.c    |    3 +++
 net/wireless/ethtool.c |   27 +++++++++++++++++++++++++++
 net/wireless/ethtool.h |   10 ++++++++++
 4 files changed, 41 insertions(+), 1 deletions(-)
 create mode 100644 net/wireless/ethtool.c
 create mode 100644 net/wireless/ethtool.h

diff --git a/net/wireless/Makefile b/net/wireless/Makefile
index c814150..f07c8dc 100644
--- a/net/wireless/Makefile
+++ b/net/wireless/Makefile
@@ -10,7 +10,7 @@ obj-$(CONFIG_WEXT_SPY) += wext-spy.o
 obj-$(CONFIG_WEXT_PRIV) += wext-priv.o
 
 cfg80211-y += core.o sysfs.o radiotap.o util.o reg.o scan.o nl80211.o
-cfg80211-y += mlme.o ibss.o sme.o chan.o
+cfg80211-y += mlme.o ibss.o sme.o chan.o ethtool.o
 cfg80211-$(CONFIG_CFG80211_DEBUGFS) += debugfs.o
 cfg80211-$(CONFIG_CFG80211_WEXT) += wext-compat.o wext-sme.o
 
diff --git a/net/wireless/core.c b/net/wireless/core.c
index c761532..faada5c 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -21,6 +21,7 @@
 #include "sysfs.h"
 #include "debugfs.h"
 #include "wext-compat.h"
+#include "ethtool.h"
 
 /* name for sysfs, %d is appended */
 #define PHY_NAME "phy"
@@ -683,6 +684,8 @@ static int cfg80211_netdev_notifier_call(struct notifier_block * nb,
 				wdev->wext.ps = false;
 			}
 #endif
+		if (!dev->ethtool_ops)
+			dev->ethtool_ops = &cfg80211_ethtool_ops;
 		break;
 	case NETDEV_GOING_DOWN:
 		switch (wdev->iftype) {
diff --git a/net/wireless/ethtool.c b/net/wireless/ethtool.c
new file mode 100644
index 0000000..94ca377
--- /dev/null
+++ b/net/wireless/ethtool.c
@@ -0,0 +1,27 @@
+#include <linux/utsrelease.h>
+#include <net/cfg80211.h>
+#include "ethtool.h"
+
+const struct ethtool_ops cfg80211_ethtool_ops = {
+	.get_drvinfo = cfg80211_get_drvinfo,
+	.get_link = ethtool_op_get_link,
+};
+
+void cfg80211_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
+{
+	struct wireless_dev *wdev = dev->ieee80211_ptr;
+
+	strncpy(info->driver, wiphy_dev(wdev->wiphy)->driver->name,
+		sizeof(info->driver));
+	info->driver[sizeof(info->driver) - 1] = '\0';
+
+	strncpy(info->version, UTS_RELEASE, sizeof(info->version));
+	info->version[sizeof(info->version) - 1] = '\0';
+
+	strncpy(info->fw_version, "N/A", sizeof(info->fw_version));
+	info->fw_version[sizeof(info->fw_version) - 1] = '\0';
+
+	strncpy(info->bus_info, dev_name(wiphy_dev(wdev->wiphy)),
+		sizeof(info->bus_info));
+	info->bus_info[sizeof(info->bus_info) - 1] = '\0';
+}
diff --git a/net/wireless/ethtool.h b/net/wireless/ethtool.h
new file mode 100644
index 0000000..a51b470
--- /dev/null
+++ b/net/wireless/ethtool.h
@@ -0,0 +1,10 @@
+#ifndef __CFG80211_ETHTOOL__
+#define __CFG80211_ETHTOOL__
+
+#include <linux/ethtool.h>
+
+extern void cfg80211_get_drvinfo(struct net_device *, struct ethtool_drvinfo *);
+
+extern const struct ethtool_ops cfg80211_ethtool_ops;
+
+#endif /* __CFG80211_ETHTOOL__ */
-- 
1.6.2.5

[PATCH 1/3] wireless: implement basic ethtool support for cfg80211 devices

[John W. Linville]

Signed-off-by: John W. Linville <linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
---
 net/wireless/Makefile  |    2 +-
 net/wireless/core.c    |    3 +++
 net/wireless/ethtool.c |   27 +++++++++++++++++++++++++++
 net/wireless/ethtool.h |   10 ++++++++++
 4 files changed, 41 insertions(+), 1 deletions(-)
 create mode 100644 net/wireless/ethtool.c
 create mode 100644 net/wireless/ethtool.h

diff --git a/net/wireless/Makefile b/net/wireless/Makefile
index c814150..f07c8dc 100644
--- a/net/wireless/Makefile
+++ b/net/wireless/Makefile
@@ -10,7 +10,7 @@ obj-$(CONFIG_WEXT_SPY) += wext-spy.o
 obj-$(CONFIG_WEXT_PRIV) += wext-priv.o
 
 cfg80211-y += core.o sysfs.o radiotap.o util.o reg.o scan.o nl80211.o
-cfg80211-y += mlme.o ibss.o sme.o chan.o
+cfg80211-y += mlme.o ibss.o sme.o chan.o ethtool.o
 cfg80211-$(CONFIG_CFG80211_DEBUGFS) += debugfs.o
 cfg80211-$(CONFIG_CFG80211_WEXT) += wext-compat.o wext-sme.o
 
diff --git a/net/wireless/core.c b/net/wireless/core.c
index c761532..faada5c 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -21,6 +21,7 @@
 #include "sysfs.h"
 #include "debugfs.h"
 #include "wext-compat.h"
+#include "ethtool.h"
 
 /* name for sysfs, %d is appended */
 #define PHY_NAME "phy"
@@ -683,6 +684,8 @@ static int cfg80211_netdev_notifier_call(struct notifier_block * nb,
 				wdev->wext.ps = false;
 			}
 #endif
+		if (!dev->ethtool_ops)
+			dev->ethtool_ops = &cfg80211_ethtool_ops;
 		break;
 	case NETDEV_GOING_DOWN:
 		switch (wdev->iftype) {
diff --git a/net/wireless/ethtool.c b/net/wireless/ethtool.c
new file mode 100644
index 0000000..94ca377
--- /dev/null
+++ b/net/wireless/ethtool.c
@@ -0,0 +1,27 @@
+#include <linux/utsrelease.h>
+#include <net/cfg80211.h>
+#include "ethtool.h"
+
+const struct ethtool_ops cfg80211_ethtool_ops = {
+	.get_drvinfo = cfg80211_get_drvinfo,
+	.get_link = ethtool_op_get_link,
+};
+
+void cfg80211_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
+{
+	struct wireless_dev *wdev = dev->ieee80211_ptr;
+
+	strncpy(info->driver, wiphy_dev(wdev->wiphy)->driver->name,
+		sizeof(info->driver));
+	info->driver[sizeof(info->driver) - 1] = '\0';
+
+	strncpy(info->version, UTS_RELEASE, sizeof(info->version));
+	info->version[sizeof(info->version) - 1] = '\0';
+
+	strncpy(info->fw_version, "N/A", sizeof(info->fw_version));
+	info->fw_version[sizeof(info->fw_version) - 1] = '\0';
+
+	strncpy(info->bus_info, dev_name(wiphy_dev(wdev->wiphy)),
+		sizeof(info->bus_info));
+	info->bus_info[sizeof(info->bus_info) - 1] = '\0';
+}
diff --git a/net/wireless/ethtool.h b/net/wireless/ethtool.h
new file mode 100644
index 0000000..a51b470
--- /dev/null
+++ b/net/wireless/ethtool.h
@@ -0,0 +1,10 @@
+#ifndef __CFG80211_ETHTOOL__
+#define __CFG80211_ETHTOOL__
+
+#include <linux/ethtool.h>
+
+extern void cfg80211_get_drvinfo(struct net_device *, struct ethtool_drvinfo *);
+
+extern const struct ethtool_ops cfg80211_ethtool_ops;
+
+#endif /* __CFG80211_ETHTOOL__ */
-- 
1.6.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] cfg80211: add firmware and hardware version to wiphy

[John W. Linville]

From: Kalle Valo <kalle.valo@nokia.com>

It's useful to provide firmware and hardware version to user space and have a
generic interface to retrieve them. Users can provide the version information
in bug reports etc.

Add fields for firmware and hardware version to struct wiphy.

(Dropped nl80211 bits for now and modified remaining bits in favor of
ethtool. -- JWL)

Cc: Kalle Valo <kalle.valo@nokia.com>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 include/net/cfg80211.h |    3 +++
 net/wireless/ethtool.c |   23 ++++++++++++++++++++++-
 net/wireless/ethtool.h |    3 +++
 3 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 241ea14..6f4862b 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1142,6 +1142,9 @@ struct wiphy {
 	u32 frag_threshold;
 	u32 rts_threshold;
 
+	char fw_version[ETHTOOL_BUSINFO_LEN];
+	u32 hw_version;
+
 	/* If multiple wiphys are registered and you're handed e.g.
 	 * a regular netdev with assigned ieee80211_ptr, you won't
 	 * know whether it points to a wiphy your driver has registered
diff --git a/net/wireless/ethtool.c b/net/wireless/ethtool.c
index 94ca377..3c59549 100644
--- a/net/wireless/ethtool.c
+++ b/net/wireless/ethtool.c
@@ -4,6 +4,8 @@
 
 const struct ethtool_ops cfg80211_ethtool_ops = {
 	.get_drvinfo = cfg80211_get_drvinfo,
+	.get_regs_len = cfg80211_get_regs_len,
+	.get_regs = cfg80211_get_regs,
 	.get_link = ethtool_op_get_link,
 };
 
@@ -18,10 +20,29 @@ void cfg80211_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
 	strncpy(info->version, UTS_RELEASE, sizeof(info->version));
 	info->version[sizeof(info->version) - 1] = '\0';
 
-	strncpy(info->fw_version, "N/A", sizeof(info->fw_version));
+	if (wdev->wiphy->fw_version[0])
+		strncpy(info->fw_version, wdev->wiphy->fw_version,
+			sizeof(info->fw_version));
+	else
+		strncpy(info->fw_version, "N/A", sizeof(info->fw_version));
 	info->fw_version[sizeof(info->fw_version) - 1] = '\0';
 
 	strncpy(info->bus_info, dev_name(wiphy_dev(wdev->wiphy)),
 		sizeof(info->bus_info));
 	info->bus_info[sizeof(info->bus_info) - 1] = '\0';
 }
+
+int cfg80211_get_regs_len(struct net_device *dev)
+{
+	/* For now, return 0... */
+	return 0;
+}
+
+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;
+}
diff --git a/net/wireless/ethtool.h b/net/wireless/ethtool.h
index a51b470..2d4602a 100644
--- a/net/wireless/ethtool.h
+++ b/net/wireless/ethtool.h
@@ -4,6 +4,9 @@
 #include <linux/ethtool.h>
 
 extern void cfg80211_get_drvinfo(struct net_device *, struct ethtool_drvinfo *);
+extern int cfg80211_get_regs_len(struct net_device *);
+extern void cfg80211_get_regs(struct net_device *, struct ethtool_regs *,
+				void *);
 
 extern const struct ethtool_ops cfg80211_ethtool_ops;
 
-- 
1.6.2.5

[PATCH 3/3] at76c50x-usb: set firmware and hardware version in wiphy

[John W. Linville]

From: Kalle Valo <kalle.valo@iki.fi>

Set firmware and hardware version in wiphy so that user space can access
it.

(Modification from original in favor of cfg80211 ethtool support. -- JWL)

Cc: Kalle Valo <kalle.valo@iki.fi>
Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 drivers/net/wireless/at76c50x-usb.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index 8e1a55d..533954d 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -2217,6 +2217,8 @@ static struct ieee80211_supported_band at76_supported_band = {
 static int at76_init_new_device(struct at76_priv *priv,
 				struct usb_interface *interface)
 {
+	struct wiphy *wiphy;
+	size_t len;
 	int ret;
 
 	/* set up the endpoint information */
@@ -2254,6 +2256,7 @@ static int at76_init_new_device(struct at76_priv *priv,
 	priv->device_unplugged = 0;
 
 	/* mac80211 initialisation */
+	wiphy = priv->hw->wiphy;
 	priv->hw->wiphy->max_scan_ssids = 1;
 	priv->hw->wiphy->max_scan_ie_len = 0;
 	priv->hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION);
@@ -2265,6 +2268,15 @@ static int at76_init_new_device(struct at76_priv *priv,
 	SET_IEEE80211_DEV(priv->hw, &interface->dev);
 	SET_IEEE80211_PERM_ADDR(priv->hw, priv->mac_addr);
 
+	len = sizeof(wiphy->fw_version);
+	snprintf(wiphy->fw_version, len, "%d.%d.%d-%d",
+		 priv->fw_version.major, priv->fw_version.minor,
+		 priv->fw_version.patch, priv->fw_version.build);
+	/* null terminate the strings in case they were truncated */
+	wiphy->fw_version[len - 1] = '\0';
+
+	wiphy->hw_version = priv->board_type;
+
 	ret = ieee80211_register_hw(priv->hw);
 	if (ret) {
 		printk(KERN_ERR "cannot register mac80211 hw (status %d)!\n",
-- 
1.6.2.5

[PATCH 3/3] at76c50x-usb: set firmware and hardware version in wiphy

[John W. Linville]

From: Kalle Valo <kalle.valo-X3B1VOXEql0@public.gmane.org>

Set firmware and hardware version in wiphy so that user space can access
it.

(Modification from original in favor of cfg80211 ethtool support. -- JWL)

Cc: Kalle Valo <kalle.valo-X3B1VOXEql0@public.gmane.org>
Signed-off-by: John W. Linville <linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
---
 drivers/net/wireless/at76c50x-usb.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index 8e1a55d..533954d 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -2217,6 +2217,8 @@ static struct ieee80211_supported_band at76_supported_band = {
 static int at76_init_new_device(struct at76_priv *priv,
 				struct usb_interface *interface)
 {
+	struct wiphy *wiphy;
+	size_t len;
 	int ret;
 
 	/* set up the endpoint information */
@@ -2254,6 +2256,7 @@ static int at76_init_new_device(struct at76_priv *priv,
 	priv->device_unplugged = 0;
 
 	/* mac80211 initialisation */
+	wiphy = priv->hw->wiphy;
 	priv->hw->wiphy->max_scan_ssids = 1;
 	priv->hw->wiphy->max_scan_ie_len = 0;
 	priv->hw->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION);
@@ -2265,6 +2268,15 @@ static int at76_init_new_device(struct at76_priv *priv,
 	SET_IEEE80211_DEV(priv->hw, &interface->dev);
 	SET_IEEE80211_PERM_ADDR(priv->hw, priv->mac_addr);
 
+	len = sizeof(wiphy->fw_version);
+	snprintf(wiphy->fw_version, len, "%d.%d.%d-%d",
+		 priv->fw_version.major, priv->fw_version.minor,
+		 priv->fw_version.patch, priv->fw_version.build);
+	/* null terminate the strings in case they were truncated */
+	wiphy->fw_version[len - 1] = '\0';
+
+	wiphy->hw_version = priv->board_type;
+
 	ret = ieee80211_register_hw(priv->hw);
 	if (ret) {
 		printk(KERN_ERR "cannot register mac80211 hw (status %d)!\n",
-- 
1.6.2.5

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] wireless: implement basic ethtool support for cfg80211 devices

[Ben Hutchings]

On Wed, 2009-09-30 at 21:19 -0400, John W. Linville wrote:
[...]
> +void cfg80211_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
> +{
> +	struct wireless_dev *wdev = dev->ieee80211_ptr;
> +
> +	strncpy(info->driver, wiphy_dev(wdev->wiphy)->driver->name,
> +		sizeof(info->driver));
> +	info->driver[sizeof(info->driver) - 1] = '\0';
[...]

Use strlcpy() instead of these two statements.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 3/3] at76c50x-usb: set firmware and hardware version in wiphy

[Ben Hutchings]

On Wed, 2009-09-30 at 21:19 -0400, John W. Linville wrote:
[...]
> +	len = sizeof(wiphy->fw_version);
> +	snprintf(wiphy->fw_version, len, "%d.%d.%d-%d",
> +		 priv->fw_version.major, priv->fw_version.minor,
> +		 priv->fw_version.patch, priv->fw_version.build);
> +	/* null terminate the strings in case they were truncated */
> +	wiphy->fw_version[len - 1] = '\0';
[...]

This last statement is unnecessary; snprintf() always null-terminates
(unless the length is zero).

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 1/3] wireless: implement basic ethtool support for cfg80211 devices

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

On Wed, 2009-09-30 at 21:19 -0400, John W. Linville wrote:

> +		if (!dev->ethtool_ops)
> +			dev->ethtool_ops = &cfg80211_ethtool_ops;
>  		break;

I might go so far and do it unconditionally so we get consistent
functionality across things. OTOH, full-mac drivers might be able to
support more.

> +const struct ethtool_ops cfg80211_ethtool_ops = {
> +	.get_drvinfo = cfg80211_get_drvinfo,
> +	.get_link = ethtool_op_get_link,
> +};
> +
> +void cfg80211_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)

if you change the order, you can make the latter static

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/3] wireless: implement basic ethtool support for cfg80211 devices

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

On Wed, 2009-09-30 at 21:19 -0400, John W. Linville wrote:

> +		if (!dev->ethtool_ops)
> +			dev->ethtool_ops = &cfg80211_ethtool_ops;
>  		break;

I might go so far and do it unconditionally so we get consistent
functionality across things. OTOH, full-mac drivers might be able to
support more.

> +const struct ethtool_ops cfg80211_ethtool_ops = {
> +	.get_drvinfo = cfg80211_get_drvinfo,
> +	.get_link = ethtool_op_get_link,
> +};
> +
> +void cfg80211_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)

if you change the order, you can make the latter static

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[Kalle Valo]

"John W. Linville" <linville@tuxdriver.com> writes:

> On Fri, Sep 25, 2009 at 09:53:35AM -0700, Luis R. Rodriguez wrote:
>
>> So for Wake-on-Wireless I ran into the same, ethtool just did not
>> offer the same wake up events needed for wireless. I could have
>> technically used ethtool and expanded it to support wireless but it
>> just seemed dirty.
>> 
>> I agree that using ethtool seems overkill compared to the patches
>> you posted.
>
> I think you either overestimate the amount of trouble for implementing
> (minimal) ethtool support or you underestimate the amount of
> functionality available through that interface.

I'm not worried about the implementation complexity, and as your
patches show it was easy. My concern is the overall design for
wireless devices. Instead of using nl80211 for everything, with some
features we would use nl80211/iw and with some ethtool. That's just
confusing and I don't like that. I would prefer that nl80211 provides
everything, it makes things so much easier.

> That, or you just don't like using something named "eth"tool for
> wireless -- but hey, let's be honest about the frames we
> send/receive to/from the kernel... :-)

I don't have a problem with the name :) But ethernet is still so much
different from 802.11 that there isn't that much to share and we in
wireless will need different features.

One example is the hw version, ethtool only provides u32 to userspace
and moves the burden of translating hw id to the user. For us a string
is much better choise because when debuggin we need to often (or
always?) know the chip version.

But this is not something I will start fighting about. If you still
think that ethtool is the way to go, I'm perfectly fine with it.

>> The ethtool interface provides functionality for viewing and modifying
> eeprom contents, dumping registers, trigger self-tests, basic driver
> info, getting and setting message reporting levels, external card
> identification (hey, _could_ be useful!), and some other bits like
> checksum offload that might(?) be useful in the future.  I understand
> regarding the WoW vs. WoL issue but probably the answer is just to
> add a new method for WoW...?

I took a look at ethtool help output from debian unstable and I think
this is the set of features we can use in wireless:

        ethtool -i|--driver DEVNAME     Show driver information
        ethtool -d|--register-dump DEVNAME      Do a register dump
                [ raw on|off ]
                [ file FILENAME ]
        ethtool -e|--eeprom-dump DEVNAME        Do a EEPROM dump
                [ raw on|off ]
                [ offset N ]
                [ length N ]
        ethtool -E|--change-eeprom DEVNAME      Change bytes in device
        EEPROM
                [ magic N ]
                [ offset N ]
                [ value N ]
        ethtool -p|--identify DEVNAME   Show visible port
        identification (e.g. blinking)
               [ TIME-IN-SECONDS ]
        ethtool -t|--test DEVNAME       Execute adapter self test
               [ online | offline ]

But here are the features which I doubt we will ever use:

        ethtool -s|--change DEVNAME     Change generic options
                [ speed %%d ]
                [ duplex half|full ]
                [ port tp|aui|bnc|mii|fibre ]
                [ autoneg on|off ]
                [ advertise %%x ]
                [ phyad %%d ]
                [ xcvr internal|external ]
                [ wol p|u|m|b|a|g|s|d... ]
                [ sopass %%x:%%x:%%x:%%x:%%x:%%x ]
                [ msglvl %%d ] 
        ethtool -a|--show-pause DEVNAME Show pause options
        ethtool -A|--pause DEVNAME      Set pause options
                [ autoneg on|off ]
                [ rx on|off ]
                [ tx on|off ]
        ethtool -c|--show-coalesce DEVNAME      Show coalesce options
        ethtool -C|--coalesce DEVNAME   Set coalesce options
                [adaptive-rx on|off]
                [adaptive-tx on|off]
                [rx-usecs N]
                [rx-frames N]
                [rx-usecs-irq N]
                [rx-frames-irq N]
                [tx-usecs N]
                [tx-frames N]
                [tx-usecs-irq N]
                [tx-frames-irq N]
                [stats-block-usecs N]
                [pkt-rate-low N]
                [rx-usecs-low N]
                [rx-frames-low N]
                [tx-usecs-low N]
                [tx-frames-low N]
                [pkt-rate-high N]
                [rx-usecs-high N]
                [rx-frames-high N]
                [tx-usecs-high N]
                [tx-frames-high N]
                [sample-interval N]
        ethtool -g|--show-ring DEVNAME  Query RX/TX ring parameters
        ethtool -G|--set-ring DEVNAME   Set RX/TX ring parameters
                [ rx N ]
                [ rx-mini N ]
                [ rx-jumbo N ]
                [ tx N ]
        ethtool -k|--show-offload DEVNAME       Get protocol offload
                information
        ethtool -K|--offload DEVNAME    Set protocol offload
                [ rx on|off ]
                [ tx on|off ]
                [ sg on|off ]
                [ tso on|off ]
                [ ufo on|off ]
                [ gso on|off ]
                [ gro on|off ]
                [ lro on|off ]
        ethtool -r|--negotiate DEVNAME  Restart N-WAY negotation
        ethtool -n|--show-nfc DEVNAME   Show Rx network flow
                classificationoptions
                [ rx-flow-hash
                tcp4|udp4|ah4|sctp4|tcp6|udp6|ah6|sctp6 ]
        ethtool -N|--config-nfc DEVNAME Configure Rx network flow
                classification options
                [ rx-flow-hash tcp4|udp4|ah4|sctp4|tcp6|udp6|ah6|sctp6
                m|v|t|s|d|f|n|r... ]

-- 
Kalle Valo

Re: [PATCH 3/3] at76c50x-usb: set firmware and hardware version in wiphy

[Kalle Valo]

Ben Hutchings <bhutchings@solarflare.com> writes:

> On Wed, 2009-09-30 at 21:19 -0400, John W. Linville wrote:
> [...]
>> +	len = sizeof(wiphy->fw_version);
>> +	snprintf(wiphy->fw_version, len, "%d.%d.%d-%d",
>> +		 priv->fw_version.major, priv->fw_version.minor,
>> +		 priv->fw_version.patch, priv->fw_version.build);
>> +	/* null terminate the strings in case they were truncated */
>> +	wiphy->fw_version[len - 1] = '\0';
> [...]
>
> This last statement is unnecessary; snprintf() always null-terminates
> (unless the length is zero).

Yes, the extra null termination is unnecessary. This was my mistake in
the first patchset I sent.

-- 
Kalle Valo

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[John W. Linville]

On Thu, Oct 01, 2009 at 05:18:33PM +0300, Kalle Valo wrote:
> "John W. Linville" <linville@tuxdriver.com> writes:
> 
> > On Fri, Sep 25, 2009 at 09:53:35AM -0700, Luis R. Rodriguez wrote:
> >
> >> So for Wake-on-Wireless I ran into the same, ethtool just did not
> >> offer the same wake up events needed for wireless. I could have
> >> technically used ethtool and expanded it to support wireless but it
> >> just seemed dirty.
> >> 
> >> I agree that using ethtool seems overkill compared to the patches
> >> you posted.
> >
> > I think you either overestimate the amount of trouble for implementing
> > (minimal) ethtool support or you underestimate the amount of
> > functionality available through that interface.
> 
> I'm not worried about the implementation complexity, and as your
> patches show it was easy. My concern is the overall design for
> wireless devices. Instead of using nl80211 for everything, with some
> features we would use nl80211/iw and with some ethtool. That's just
> confusing and I don't like that. I would prefer that nl80211 provides
> everything, it makes things so much easier.

Well, if the hw/fw version numbers were the only thing then I'd
probably say it's not a big deal.  But having ethtool support is nice
in that it makes a familiar tool work for us.  Among other things,
this probably helps with some distro scripts that don't work quite
right without it.  Plus, there is lots of debugging stuff that could
be turned-on without having to write new tools.

I suppose I understand the 'one API' idea, but why duplicate
functionality?  Anyway, adding a couple of ioctl calls isn't a
big deal.  And don't forget, we are still network drivers too...

> > That, or you just don't like using something named "eth"tool for
> > wireless -- but hey, let's be honest about the frames we
> > send/receive to/from the kernel... :-)
> 
> I don't have a problem with the name :) But ethernet is still so much
> different from 802.11 that there isn't that much to share and we in
> wireless will need different features.
> 
> One example is the hw version, ethtool only provides u32 to userspace
> and moves the burden of translating hw id to the user. For us a string
> is much better choise because when debuggin we need to often (or
> always?) know the chip version.

Look at the way most drivers set the version (using each byte as a
field).  If you want prettier output, adding a parser to the userland
ethtool is fairly trivial.  It looks something like the patch below...

> But this is not something I will start fighting about. If you still
> think that ethtool is the way to go, I'm perfectly fine with it.
> 
> >> The ethtool interface provides functionality for viewing and modifying
> > eeprom contents, dumping registers, trigger self-tests, basic driver
> > info, getting and setting message reporting levels, external card
> > identification (hey, _could_ be useful!), and some other bits like
> > checksum offload that might(?) be useful in the future.  I understand
> > regarding the WoW vs. WoL issue but probably the answer is just to
> > add a new method for WoW...?
> 
> I took a look at ethtool help output from debian unstable and I think
> this is the set of features we can use in wireless:
> 
>         ethtool -i|--driver DEVNAME     Show driver information
>         ethtool -d|--register-dump DEVNAME      Do a register dump
>                 [ raw on|off ]
>                 [ file FILENAME ]
>         ethtool -e|--eeprom-dump DEVNAME        Do a EEPROM dump
>                 [ raw on|off ]
>                 [ offset N ]
>                 [ length N ]
>         ethtool -E|--change-eeprom DEVNAME      Change bytes in device
>         EEPROM
>                 [ magic N ]
>                 [ offset N ]
>                 [ value N ]
>         ethtool -p|--identify DEVNAME   Show visible port
>         identification (e.g. blinking)
>                [ TIME-IN-SECONDS ]
>         ethtool -t|--test DEVNAME       Execute adapter self test
>                [ online | offline ]
 
I agree with the above.

> But here are the features which I doubt we will ever use:
> 
>         ethtool -s|--change DEVNAME     Change generic options
>                 [ speed %%d ]
>                 [ duplex half|full ]
>                 [ port tp|aui|bnc|mii|fibre ]
>                 [ autoneg on|off ]
>                 [ advertise %%x ]
>                 [ phyad %%d ]
>                 [ xcvr internal|external ]
>                 [ wol p|u|m|b|a|g|s|d... ]
>                 [ sopass %%x:%%x:%%x:%%x:%%x:%%x ]
>                 [ msglvl %%d ] 
>         ethtool -a|--show-pause DEVNAME Show pause options
>         ethtool -A|--pause DEVNAME      Set pause options
>                 [ autoneg on|off ]
>                 [ rx on|off ]
>                 [ tx on|off ]

I agree that the above are ethernet-specific.

>         ethtool -c|--show-coalesce DEVNAME      Show coalesce options
>         ethtool -C|--coalesce DEVNAME   Set coalesce options
>                 [adaptive-rx on|off]
>                 [adaptive-tx on|off]
>                 [rx-usecs N]
>                 [rx-frames N]
>                 [rx-usecs-irq N]
>                 [rx-frames-irq N]
>                 [tx-usecs N]
>                 [tx-frames N]
>                 [tx-usecs-irq N]
>                 [tx-frames-irq N]
>                 [stats-block-usecs N]
>                 [pkt-rate-low N]
>                 [rx-usecs-low N]
>                 [rx-frames-low N]
>                 [tx-usecs-low N]
>                 [tx-frames-low N]
>                 [pkt-rate-high N]
>                 [rx-usecs-high N]
>                 [rx-frames-high N]
>                 [tx-usecs-high N]
>                 [tx-frames-high N]
>                 [sample-interval N]

These _could_ be useful if wireless becomes more
performance-oriented...

>         ethtool -g|--show-ring DEVNAME  Query RX/TX ring parameters
>         ethtool -G|--set-ring DEVNAME   Set RX/TX ring parameters
>                 [ rx N ]
>                 [ rx-mini N ]
>                 [ rx-jumbo N ]
>                 [ tx N ]

Wireless devices have ring buffers, no?

>         ethtool -k|--show-offload DEVNAME       Get protocol offload
>                 information
>         ethtool -K|--offload DEVNAME    Set protocol offload
>                 [ rx on|off ]
>                 [ tx on|off ]
>                 [ sg on|off ]
>                 [ tso on|off ]
>                 [ ufo on|off ]
>                 [ gso on|off ]
>                 [ gro on|off ]
>                 [ lro on|off ]

Again, if wireless devices become performance-oriented...

>         ethtool -r|--negotiate DEVNAME  Restart N-WAY negotation

Ethernet-specific...might could be overloaded for wireless to trigger
reassoc...?

>         ethtool -n|--show-nfc DEVNAME   Show Rx network flow
>                 classificationoptions
>                 [ rx-flow-hash
>                 tcp4|udp4|ah4|sctp4|tcp6|udp6|ah6|sctp6 ]
>         ethtool -N|--config-nfc DEVNAME Configure Rx network flow
>                 classification options
>                 [ rx-flow-hash tcp4|udp4|ah4|sctp4|tcp6|udp6|ah6|sctp6
>                 m|v|t|s|d|f|n|r... ]

Long-shot, but no reason it couldn't be used in wireless... :-)

Anyway, it doesn't really matter if we don't use the whole API -- many
older ethernet devices don't support all these features.  The point
is that the API exists and has some overlap with our needs.  It is a
driver-oriented API, with nitty-gritty stuff that need not clutter a
configuraiton API like cfg80211.  There is even the potential of us
adding our own extensions (e.g. WoW) that are also device-oriented.

Anyway, between the link detection and making distro scripts work
plus enabling a familiar tool for basic driver info I think this is
a win.  So much the better if some drivers move to ethtool for register
dumping, setting message verbosity, querying/changing eeprom values,
etc, etc...

John

P.S.  The aforementioned path for userland ethtool...(theorhetical,
not even compiled...)

>From aa92d32ac1cca57bdd3439013b0c7777bdf1217c Mon Sep 17 00:00:00 2001
From: John W. Linville <linville@tuxdriver.com>
Date: Thu, 1 Oct 2009 11:01:32 -0400
Subject: [PATCH] add support for at76c50x-usb driver.

Signed-off-by: John W. Linville <linville@tuxdriver.com>
---
 Makefile.am    |    2 +-
 at76c50x-usb.c |   32 ++++++++++++++++++++++++++++++++
 ethtool.c      |    1 +
 3 files changed, 34 insertions(+), 1 deletions(-)
 create mode 100644 at76c50x-usb.c

diff --git a/Makefile.am b/Makefile.am
index eac65fe..a384949 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -8,7 +8,7 @@ ethtool_SOURCES = ethtool.c ethtool-copy.h ethtool-util.h	\
 		  amd8111e.c de2104x.c e100.c e1000.c igb.c	\
 		  fec_8xx.c ibm_emac.c ixgb.c ixgbe.c natsemi.c	\
 		  pcnet32.c realtek.c tg3.c marvell.c vioc.c	\
-		  smsc911x.c
+		  smsc911x.c at76c50x-usb.c
 
 dist-hook:
 	cp $(top_srcdir)/ethtool.spec $(distdir)
diff --git a/at76c50x-usb.c b/at76c50x-usb.c
new file mode 100644
index 0000000..295d1cb
--- /dev/null
+++ b/at76c50x-usb.c
@@ -0,0 +1,32 @@
+#include <stdio.h>
+#include "ethtool-util.h"
+
+static char hw_versions[] = {
+        "503_ISL3861",
+        "503_ISL3863",
+        "        503",
+        "    503_ACC",
+        "        505",
+        "   505_2958",
+        "       505A",
+        "     505AMX",
+};
+
+int
+at76c50x_usb_dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs)
+{
+	u8 version = (u8)(regs->version >> 24);
+	u8 rev_id = (u8)(regs->version);
+	char *ver_string;
+
+	if(version != 0)
+		return -1;
+
+	ver_string = hw_versions[rev_id];
+	fprintf(stdout,
+		"Hardware Version                    %s\n",
+		ver_string);
+
+	return 0;
+}
+
diff --git a/ethtool.c b/ethtool.c
index 0110682..7608750 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -1189,6 +1189,7 @@ static struct {
 	{ "sky2", sky2_dump_regs },
         { "vioc", vioc_dump_regs },
         { "smsc911x", smsc911x_dump_regs },
+        { "at76c50x-usb", at76c50x_usb_dump_regs },
 };
 
 static int dump_regs(struct ethtool_drvinfo *info, struct ethtool_regs *regs)
-- 
1.6.2.5
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[Ben Hutchings]

On Thu, 2009-10-01 at 11:18 -0400, John W. Linville wrote:
[...]
> > But here are the features which I doubt we will ever use:
> > 
> >         ethtool -s|--change DEVNAME     Change generic options
> >                 [ speed %%d ]
> >                 [ duplex half|full ]
> >                 [ port tp|aui|bnc|mii|fibre ]
> >                 [ autoneg on|off ]
> >                 [ advertise %%x ]
> >                 [ phyad %%d ]
> >                 [ xcvr internal|external ]
> >                 [ wol p|u|m|b|a|g|s|d... ]
> >                 [ sopass %%x:%%x:%%x:%%x:%%x:%%x ]
> >                 [ msglvl %%d ] 
> >         ethtool -a|--show-pause DEVNAME Show pause options
> >         ethtool -A|--pause DEVNAME      Set pause options
> >                 [ autoneg on|off ]
> >                 [ rx on|off ]
> >                 [ tx on|off ]
> 
> I agree that the above are ethernet-specific.
[...]

Message level isn't and WoL arguably isn't.  It's a shame that these
original ethtool settings are still bundled together...

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[Kalle Valo]

"John W. Linville" <linville@tuxdriver.com> writes:

> On Thu, Oct 01, 2009 at 05:18:33PM +0300, Kalle Valo wrote:
>> 
>> I'm not worried about the implementation complexity, and as your
>> patches show it was easy. My concern is the overall design for
>> wireless devices. Instead of using nl80211 for everything, with some
>> features we would use nl80211/iw and with some ethtool. That's just
>> confusing and I don't like that. I would prefer that nl80211 provides
>> everything, it makes things so much easier.
>
> Well, if the hw/fw version numbers were the only thing then I'd
> probably say it's not a big deal.  But having ethtool support is nice
> in that it makes a familiar tool work for us.  Among other things,
> this probably helps with some distro scripts that don't work quite
> right without it.  Plus, there is lots of debugging stuff that could
> be turned-on without having to write new tools.

Agreed, maybe expect the distro scripts part. To me that just sounds
as a bug in the scripts.

> I suppose I understand the 'one API' idea, but why duplicate
> functionality?

Just because the common functionality in this case isn't high enough.
I'm worried that we will use 10% of the functionality in nl80211 and
the rest 90% will be something we can't use and have to reimplement in
nl80211.

> Anyway, adding a couple of ioctl calls isn't a big deal.

Sure, but we need to support this forever. If, say after two years, we
decide that ethtool is not the way to go, it's very difficult to
remove it. The less interfaces we have, the easier it is to maintain
them.

> And don't forget, we are still network drivers too...

I hope ethtool isn't a strict requirement for a network driver, at
least I haven't heard about that.

>> One example is the hw version, ethtool only provides u32 to userspace
>> and moves the burden of translating hw id to the user. For us a string
>> is much better choise because when debuggin we need to often (or
>> always?) know the chip version.
>
> Look at the way most drivers set the version (using each byte as a
> field).

Yes, that's how it is also with wl1251. A number like '0x7030101' is
just not that user friendly.

> If you want prettier output, adding a parser to the userland ethtool
> is fairly trivial. It looks something like the patch below...

Oh wow, that's cool and a truly useful feature. One complaint less
from me :)

>>         ethtool -c|--show-coalesce DEVNAME      Show coalesce options
>>         ethtool -C|--coalesce DEVNAME   Set coalesce options
>>                 [adaptive-rx on|off]
>>                 [adaptive-tx on|off]
>>                 [rx-usecs N]
>>                 [rx-frames N]
>>                 [rx-usecs-irq N]
>>                 [rx-frames-irq N]
>>                 [tx-usecs N]
>>                 [tx-frames N]
>>                 [tx-usecs-irq N]
>>                 [tx-frames-irq N]
>>                 [stats-block-usecs N]
>>                 [pkt-rate-low N]
>>                 [rx-usecs-low N]
>>                 [rx-frames-low N]
>>                 [tx-usecs-low N]
>>                 [tx-frames-low N]
>>                 [pkt-rate-high N]
>>                 [rx-usecs-high N]
>>                 [rx-frames-high N]
>>                 [tx-usecs-high N]
>>                 [tx-frames-high N]
>>                 [sample-interval N]
>
> These _could_ be useful if wireless becomes more
> performance-oriented...

Maybe, or maybe not. We will only find out within the next few years.

And what will we do if the parameters are actually a bit different? Is
it ok to extend ethtool for supporting wireless or do we later on have
to add separate support to nl80211? The latter would suck big time.

>>         ethtool -g|--show-ring DEVNAME  Query RX/TX ring parameters
>>         ethtool -G|--set-ring DEVNAME   Set RX/TX ring parameters
>>                 [ rx N ]
>>                 [ rx-mini N ]
>>                 [ rx-jumbo N ]
>>                 [ tx N ]
>
> Wireless devices have ring buffers, no?

Yes, there is hardware which have them but again the question is this
relevant for wireless devices. In ethernet the hardware is the
bottleneck but in 802.11 the wireless medium is the bottleneck, so the
parameters we need to configure are usually different.

>>         ethtool -r|--negotiate DEVNAME  Restart N-WAY negotation
>
> Ethernet-specific...might could be overloaded for wireless to trigger
> reassoc...?

Please no, I don't want to see any reassociation or anything else
802.11 state related in ethtool, nl80211 was created for this. This is
something I would object loudly :)

> Anyway, it doesn't really matter if we don't use the whole API -- many
> older ethernet devices don't support all these features.  The point
> is that the API exists and has some overlap with our needs.  It is a
> driver-oriented API, with nitty-gritty stuff that need not clutter a
> configuraiton API like cfg80211.  There is even the potential of us
> adding our own extensions (e.g. WoW) that are also device-oriented.
>
> Anyway, between the link detection and making distro scripts work
> plus enabling a familiar tool for basic driver info I think this is
> a win.  So much the better if some drivers move to ethtool for register
> dumping, setting message verbosity, querying/changing eeprom values,
> etc, etc...

Sounds good enough. As I said in my earlier email, I'm not going argue
about this for too long. You know this better than I do. So let's go
forward with ethtool. 

Thanks for listening to my concerns.

-- 
Kalle Valo

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[John W. Linville]

On Thu, Oct 01, 2009 at 04:33:19PM +0100, Ben Hutchings wrote:
> On Thu, 2009-10-01 at 11:18 -0400, John W. Linville wrote:
> [...]
> > > But here are the features which I doubt we will ever use:
> > > 
> > >         ethtool -s|--change DEVNAME     Change generic options
> > >                 [ speed %%d ]
> > >                 [ duplex half|full ]
> > >                 [ port tp|aui|bnc|mii|fibre ]
> > >                 [ autoneg on|off ]
> > >                 [ advertise %%x ]
> > >                 [ phyad %%d ]
> > >                 [ xcvr internal|external ]
> > >                 [ wol p|u|m|b|a|g|s|d... ]
> > >                 [ sopass %%x:%%x:%%x:%%x:%%x:%%x ]
> > >                 [ msglvl %%d ] 
> > >         ethtool -a|--show-pause DEVNAME Show pause options
> > >         ethtool -A|--pause DEVNAME      Set pause options
> > >                 [ autoneg on|off ]
> > >                 [ rx on|off ]
> > >                 [ tx on|off ]
> > 
> > I agree that the above are ethernet-specific.
> [...]
> 
> Message level isn't and WoL arguably isn't.  It's a shame that these
> original ethtool settings are still bundled together...

Oh, yes!  Missed those in the noise...

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[John W. Linville]

On Thu, Oct 01, 2009 at 04:33:19PM +0100, Ben Hutchings wrote:
> On Thu, 2009-10-01 at 11:18 -0400, John W. Linville wrote:
> [...]
> > > But here are the features which I doubt we will ever use:
> > > 
> > >         ethtool -s|--change DEVNAME     Change generic options
> > >                 [ speed %%d ]
> > >                 [ duplex half|full ]
> > >                 [ port tp|aui|bnc|mii|fibre ]
> > >                 [ autoneg on|off ]
> > >                 [ advertise %%x ]
> > >                 [ phyad %%d ]
> > >                 [ xcvr internal|external ]
> > >                 [ wol p|u|m|b|a|g|s|d... ]
> > >                 [ sopass %%x:%%x:%%x:%%x:%%x:%%x ]
> > >                 [ msglvl %%d ] 
> > >         ethtool -a|--show-pause DEVNAME Show pause options
> > >         ethtool -A|--pause DEVNAME      Set pause options
> > >                 [ autoneg on|off ]
> > >                 [ rx on|off ]
> > >                 [ tx on|off ]
> > 
> > I agree that the above are ethernet-specific.
> [...]
> 
> Message level isn't and WoL arguably isn't.  It's a shame that these
> original ethtool settings are still bundled together...

Oh, yes!  Missed those in the noise...

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org			might be all we have.  Be ready.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[John W. Linville]

On Thu, Oct 01, 2009 at 07:20:09PM +0300, Kalle Valo wrote:
> "John W. Linville" <linville@tuxdriver.com> writes:
> 
> > On Thu, Oct 01, 2009 at 05:18:33PM +0300, Kalle Valo wrote:

> > Anyway, adding a couple of ioctl calls isn't a big deal.
> 
> Sure, but we need to support this forever. If, say after two years, we
> decide that ethtool is not the way to go, it's very difficult to
> remove it. The less interfaces we have, the easier it is to maintain
> them.

Just to be clear, I was taling about adding ioctl calls to a
userland application (if you didn't want to use the ethtool utility).
The required ioctls are already defined for ethtool in the kernel.

> >>         ethtool -r|--negotiate DEVNAME  Restart N-WAY negotation
> >
> > Ethernet-specific...might could be overloaded for wireless to trigger
> > reassoc...?
> 
> Please no, I don't want to see any reassociation or anything else
> 802.11 state related in ethtool, nl80211 was created for this. This is
> something I would object loudly :)

Well, it was just a thought... :-)

> > Anyway, between the link detection and making distro scripts work
> > plus enabling a familiar tool for basic driver info I think this is
> > a win.  So much the better if some drivers move to ethtool for register
> > dumping, setting message verbosity, querying/changing eeprom values,
> > etc, etc...
> 
> Sounds good enough. As I said in my earlier email, I'm not going argue
> about this for too long. You know this better than I do. So let's go
> forward with ethtool. 
> 
> Thanks for listening to my concerns.

Sure, np.  And FWIW, I don't predict a huge problem if there are
valid extensions required for use by wireless drivers in the future.
But for now, I'd like to see us make use of some of the debugging
facilities available in the ethtool API -- hopefully the iwlwifi guys
are listening... ;-)

John
-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[Luis R. Rodriguez]

On Thu, Oct 1, 2009 at 5:07 PM, John W. Linville <linville@tuxdriver.com> wrote:

> I don't predict a huge problem if there are
> valid extensions required for use by wireless drivers in the future.
> But for now, I'd like to see us make use of some of the debugging
> facilities available in the ethtool API -- hopefully the iwlwifi guys
> are listening... ;-)

Does the same apply to wimax then? Ethtool for 802.11 and wimax? Eh.

  Luis

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[Luis R. Rodriguez]

On Thu, Oct 1, 2009 at 5:07 PM, John W. Linville <linville-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org> wrote:

> I don't predict a huge problem if there are
> valid extensions required for use by wireless drivers in the future.
> But for now, I'd like to see us make use of some of the debugging
> facilities available in the ethtool API -- hopefully the iwlwifi guys
> are listening... ;-)

Does the same apply to wimax then? Ethtool for 802.11 and wimax? Eh.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] cfg80211: firmware and hardware version

[Inaky Perez-Gonzalez]

On Thu, 2009-10-01 at 13:56 -0600, Luis R. Rodriguez wrote:
> On Thu, Oct 1, 2009 at 5:07 PM, John W. Linville <linville@tuxdriver.com> wrote:
> 
> > I don't predict a huge problem if there are
> > valid extensions required for use by wireless drivers in the future.
> > But for now, I'd like to see us make use of some of the debugging
> > facilities available in the ethtool API -- hopefully the iwlwifi guys
> > are listening... ;-)
> 
> Does the same apply to wimax then? Ethtool for 802.11 and wimax? Eh.

Not really -- WiMAX is not eth-frame based, but IP based.

The WiMAX stack doesn't require any type of framing/network device
typing requirement. That is left up to the device driver writer
(although yes, emulating eth is easier).

-- 
-- Inaky