Sysfs interface for accessing non-volatile configuration.

PATCH In review Javier Cardona 4 participants 14 messages May 20, 2008 – May 22, 2008 Networking drivers (wireless)

lore

Share & tools

lore

Javier Cardona

author

2008-05-20 03:14 UTC #0 lore raw

PATCH 2/2 Sysfs interface for accessing non-volatile configuration.

This will create the following sysfs directories:
/sys/class/net/ethX

		...
		|-- boot_options
		|   |-- bootflag
		|   `-- boottime
		...
		|-- mesh_ie
		|   |-- capability
		|   |-- mesh_id
		|   |-- metric_id
		|   `-- protocol_id

Signed-off-by Javier Cardona <[email protected]>

---
 drivers/net/wireless/libertas/Makefile     |    1 +
 drivers/net/wireless/libertas/defs.h       |    4 +-
 drivers/net/wireless/libertas/main.c       |    3 +
 drivers/net/wireless/libertas/persistcfg.c |  422 ++++++++++++++++++++++++++++
 drivers/net/wireless/libertas/persistcfg.h |    5 +
 5 files changed, 434 insertions(+), 1 deletions(-)
 create mode 100644 drivers/net/wireless/libertas/persistcfg.c
 create mode 100644 drivers/net/wireless/libertas/persistcfg.h

drivers/net/wireless/libertas/Makefile +1 -0

index f0724e3..09b6353 100644
@@ -3,6 +3,7 @@ libertas-objs := main.o wext.o \
 		cmdresp.o scan.o	  \
 		11d.o 		  \
 		debugfs.o	  \
+		persistcfg.o	\
 		ethtool.o assoc.o
 
 usb8xxx-objs += if_usb.o

drivers/net/wireless/libertas/defs.h +3 -1

index 0120d6c..4a7f936 100644
@@ -40,6 +40,7 @@
 #define LBS_DEB_THREAD	0x00100000
 #define LBS_DEB_HEX	0x00200000
 #define LBS_DEB_SDIO	0x00400000
+#define LBS_DEB_SYSFS	0x00800000
 
 extern unsigned int lbs_debug;

@@ -81,7 +82,8 @@ do { if ((lbs_debug & (grp)) == (grp)) \
 #define lbs_deb_usbd(dev, fmt, args...) LBS_DEB_LL(LBS_DEB_USB, " usbd", "%s:" fmt, (dev)->bus_id, ##args)
 #define lbs_deb_cs(fmt, args...)        LBS_DEB_LL(LBS_DEB_CS, " cs", fmt, ##args)
 #define lbs_deb_thread(fmt, args...)    LBS_DEB_LL(LBS_DEB_THREAD, " thread", fmt, ##args)
-#define lbs_deb_sdio(fmt, args...)      LBS_DEB_LL(LBS_DEB_SDIO, " thread", fmt, ##args)
+#define lbs_deb_sdio(fmt, args...)      LBS_DEB_LL(LBS_DEB_SDIO, " sdio", fmt, ##args)
+#define lbs_deb_sysfs(fmt, args...)     LBS_DEB_LL(LBS_DEB_SYSFS, " sysfs", fmt, ##args)
 
 #define lbs_pr_info(format, args...) \
 	printk(KERN_INFO DRV_NAME": " format, ## args)

drivers/net/wireless/libertas/main.c +3 -0

index 5001888..27769cb 100644
@@ -23,6 +23,7 @@
 #include "scan.h"
 #include "assoc.h"
 #include "cmd.h"
+#include "persistcfg.h"
 
 #define DRIVER_RELEASE_VERSION "323.p0"
 const char lbs_driver_version[] = "COMM-USB8388-" DRIVER_RELEASE_VERSION

@@ -1218,6 +1219,7 @@ int lbs_start_card(struct lbs_private *priv)
 	if (device_create_file(&dev->dev, &dev_attr_lbs_rtap))
 		lbs_pr_err("cannot register lbs_rtap attribute\n");
 
+	lbs_persist_config_init(dev);
 	lbs_update_channel(priv);
 
 	/* 5.0.16p0 is known to NOT support any mesh */

@@ -1278,6 +1280,7 @@ int lbs_stop_card(struct lbs_private *priv)
 
 	lbs_debugfs_remove_one(priv);
 	device_remove_file(&dev->dev, &dev_attr_lbs_rtap);
+	lbs_persist_config_remove(dev);
 	if (priv->mesh_tlv)
 		device_remove_file(&dev->dev, &dev_attr_lbs_mesh);

drivers/net/wireless/libertas/persistcfg.c new file +422 -0

422 lines not on this page · raw message

drivers/net/wireless/libertas/persistcfg.h new file +5 -0

new file mode 100644
index 0000000..6ab9ff6
@@ -0,0 +1,5 @@
+#ifndef _LBS_PERSISTCFG_H_
+#define _LBS_PERSISTCFG_H_
+void lbs_persist_config_init(struct net_device *net);
+void lbs_persist_config_remove(struct net_device *net);
+#endif

Signature · 1 line

1.5.2.4

cmd: DNLD_CMD: command 0x00a3, seq 20, size 144
DNLD_CMD: a3 00 90 00 14 00 00 00 03 00 00 00 05 00 00 00
DNLD_CMD: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
DNLD_CMD: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
cmd: CMD_RESP: response 0x80a3, seq 20, size 144
CMD_RESP: a3 80 90 00 14 00 02 00 03 00 00 00 05 00 00 00
CMD_RESP: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
PREP_CMD: command 0x00a3 failed: 2

So I would like to have one (or both) of the following things:

*) a runtime check (e.g. by comparing firmware version) if
the firmware supports mesh at all, and then doing this only in that case. Probably as lbs_has_mesh() call, we're going to need this in several places
*) a kernel-config option, because many people use CF/SDIO
cards in embedded environmnents and like to have things small. No need for #ifdef/#endif, because the compiler can savely optimize things out: "#define lbs_has_mesh() (0)" if mesh is turned off via Kconfig.

Also, you create /sys/class/net/ethX/mesh_ie. This doesn't make sense to me. /sys/class/net/mshX/boot_options seems to be more logical. However, I don't know much about mesh ...

Javier Cardona

author

2008-05-21 05:53 UTC #2 lore raw

Holger,

Thanks for your comments.

Quoted 10 lines · Holger Schurig

*) a runtime check (e.g. by comparing firmware version) if
the firmware supports mesh at all, and then doing this only in that case. Probably as lbs_has_mesh() call, we're going to need this in several places
*) a kernel-config option, because many people use CF/SDIO
cards in embedded environmnents and like to have things small. No need for #ifdef/#endif, because the compiler can savely optimize things out: "#define lbs_has_mesh() (0)" if mesh is turned off via Kconfig.

Is David's suggestion acceptable to you?

Quoted 5 lines · David Woodhouse

As Holger points out, we should call those only if (priv->mesh_tlv).
Also, you create /sys/class/net/ethX/mesh_ie. This doesn't make sense to me. /sys/class/net/mshX/boot_options seems to be more logical. However, I don't know much about mesh ...

The entries in the mesh_ie directory groups the non-volatile parameters of the mesh Information Element that is included in all mesh beacons.

Javier

Signature · 2 lines

Javier Cardona
cozybit Inc.

Holger Schurig

2008-05-21 07:07 UTC #3 lore raw

Quoted 1 line

Is David's suggestion acceptable to you?

Yes, it is. For the long time, I'd like to have a kernel option. I'll probably make this by myself, then I can show numbers (e.g. if the saving is considerable). For now, go with checking mesh_XXX.

Quoted 3 lines

The entries in the mesh_ie directory groups the non-volatile parameters of the mesh Information Element that is included in all mesh beacons.

Ahh, ok. And since ethX can't produce mesh beacons, then mesh_ie entry belongs then to mshX's sysfs-entry, right?

David Woodhouse

2008-05-21 08:19 UTC #4 lore raw

Quoted 2 lines · Holger Schurig

Ahh, ok. And since ethX can't produce mesh beacons, then mesh_ie entry belongs then to mshX's sysfs-entry, right?

I moved it there already.

Signature · 1 line

dwmw2

David Woodhouse

2008-05-20 14:12 UTC #5 lore raw

In reply to Javier Cardona

Quoted 9 lines · Javier Cardona

drivers/net/wireless/libertas/main.c +1 -0

@@ -1218,6 +1219,7 @@ int lbs_start_card(struct lbs_private *priv)
        if (device_create_file(&dev->dev, &dev_attr_lbs_rtap))
                lbs_pr_err("cannot register lbs_rtap attribute\n");
 
+       lbs_persist_config_init(dev);
        lbs_update_channel(priv);
 
        /* 5.0.16p0 is known to NOT support any mesh */

drivers/net/wireless/libertas/main.c int lbs_stop_card(struct lbs_private *priv) · line 1286

Quoted 7 lines

drivers/net/wireless/libertas/main.c +1 -0

@@ -1278,6 +1280,7 @@ int lbs_stop_card(struct lbs_private *priv)
 
        lbs_debugfs_remove_one(priv);
        device_remove_file(&dev->dev, &dev_attr_lbs_rtap);
+       lbs_persist_config_remove(dev);
        if (priv->mesh_tlv)
                device_remove_file(&dev->dev, &dev_attr_lbs_mesh);

As Holger points out, we should call those only if (priv->mesh_tlv).

drivers/net/wireless/libertas/persistcfg.c line 53

Quoted 2 lines

drivers/net/wireless/libertas/persistcfg.c new file +2 -0
+	return snprintf(buf, 12, "0x%X\n",
+		le32_to_cpu(cpu_to_le32(defs.bootflag)));

Que? Sparse loves that one :)

drivers/net/wireless/libertas/persistcfg.c line 72

Quoted 1 line

drivers/net/wireless/libertas/persistcfg.c new file +1 -0
+	*((uint32_t *)&cmd.data[0]) = cpu_to_le32(!!datum);

Sparse doesn't much like that either. It should be (__le32 *)

Quoted 4 lines

drivers/net/wireless/libertas/persistcfg.c new file +4 -0

+static ssize_t boottime_get(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct lbs_private *priv = to_net_dev(dev)->priv;

drivers/net/wireless/libertas/persistcfg.c line 98

Quoted 10 lines

drivers/net/wireless/libertas/persistcfg.c new file +10 -0

+	struct cmd_ds_mesh_config cmd;
+	struct mrvl_mesh_defaults *defs;
+	int ret;
+
+	memset(&cmd, 0, sizeof(struct cmd_ds_mesh_config));
+	ret = lbs_mesh_config_send(priv, &cmd, CMD_ACT_MESH_CONFIG_GET,
+			CMD_TYPE_MESH_GET_DEFAULTS);
+
+	if (ret)
+		return -EOPNOTSUPP;

Why not using mesh_get_default_parameters()?

drivers/net/wireless/libertas/persistcfg.c line 129

Quoted 9 lines

drivers/net/wireless/libertas/persistcfg.c new file +9 -0

+	/* A too small boot time will result in the device booting into
+	 * standalone (no-host) mode before the host can take control of it,
+	 * so the change will be hard to revert.  This may be a desired
+	 * feature (e.g to configure a very fast boot time for devices that
+	 * will not be attached to a host), but dangerous.  So I'm enforcing a
+	 * lower limit of 20 seconds:  remove and recompile the driver if this
+	 * does not work for you.
+	 */
+	datum = (datum < 20) ? 20 : datum;

Hm. The driver should be able to cope with a device which is already booted -- it sends it a reset command and then initialises it. I don't think we have to be so paranoid, do we? If there's a lower limit on the boottime, it would be the time it takes for an already-active driver to react when the device resets. Which is much less than 20 seconds.

Signature · 1 line

dwmw2

Javier Cardona

author

2008-05-21 19:06 UTC #6 lore raw

David,

Quoted 11 lines · David Woodhouse

drivers/net/wireless/libertas/persistcfg.c line 129

Quoted 9 lines · Javier Cardona

drivers/net/wireless/libertas/persistcfg.c new file +9 -0

+     /* A too small boot time will result in the device booting into
+      * standalone (no-host) mode before the host can take control of it,
+      * so the change will be hard to revert.  This may be a desired
+      * feature (e.g to configure a very fast boot time for devices that
+      * will not be attached to a host), but dangerous.  So I'm enforcing a
+      * lower limit of 20 seconds:  remove and recompile the driver if this
+      * does not work for you.
+      */
+     datum = (datum < 20) ? 20 : datum;

Quoted 5 lines

Hm. The driver should be able to cope with a device which is already booted -- it sends it a reset command and then initialises it. I don't think we have to be so paranoid, do we? If there's a lower limit on the boottime, it would be the time it takes for an already-active driver to react when the device resets. Which is much less than 20 seconds.

If the boottime is set smaller than the host boot time, the device will transition into standalone mode before it is detected by the host. The only recovery when that happens is to either reset the device via an external reset line or to re-insert it. That lower boundary was just intended to avoid support issues of users seeing their devices disappear after a reboot.

Javier

Signature · 2 lines

Javier Cardona
cozybit Inc.

David Woodhouse

2008-05-21 19:06 UTC #7 lore raw

Quoted 5 lines · Javier Cardona

If the boottime is set smaller than the host boot time, the device will transition into standalone mode before it is detected by the host. The only recovery when that happens is to either reset the device via an external reset line or to re-insert it.

Nah, the driver will send it a CMD_802_11_RESET when it finds it not responding properly to bootcmds, and it should be fine.

Quoted 2 lines

That lower boundary was just intended to avoid support issues of
users seeing their devices disappear after a reboot.

I remember that well :)

Signature · 1 line

dwmw2

Javier Cardona

author

2008-05-21 19:18 UTC #8 lore raw

David,

Quoted 9 lines · David Woodhouse

Quoted 5 lines · Javier Cardona
If the boottime is set smaller than the host boot time, the device will transition into standalone mode before it is detected by the host. The only recovery when that happens is to either reset the device via an external reset line or to re-insert it.
Nah, the driver will send it a CMD_802_11_RESET when it finds it not responding properly to bootcmds, and it should be fine.

You need a live endpoint to send a CMD_802_11_RESET, which you won't have if the device did not respond to USB enumeration because it was in standalone mode.

Javier

Signature · 2 lines

Javier Cardona
cozybit Inc.

David Woodhouse

2008-05-21 19:28 UTC #9 lore raw

Quoted 3 lines · Javier Cardona

You need a live endpoint to send a CMD_802_11_RESET, which you won't have if the device did not respond to USB enumeration because it was in standalone mode.

Hm, did that behaviour change recently? I'm sure this recovery was working before.

Signature · 1 line

dwmw2

Javier Cardona

author

2008-05-21 19:58 UTC #10 lore raw

David,

Quoted 7 lines · David Woodhouse

Quoted 3 lines · Javier Cardona
You need a live endpoint to send a CMD_802_11_RESET, which you won't have if the device did not respond to USB enumeration because it was in standalone mode.
Hm, did that behaviour change recently? I'm sure this recovery was working before.

I don't think we've ever been able to communicate over USB with a dongle that is in standalone mode.

Javier

Signature · 2 lines

Javier Cardona
cozybit Inc.

David Woodhouse

2008-05-21 20:01 UTC #11 lore raw

Quoted 8 lines · Javier Cardona

David,
Quoted 5 lines · David Woodhouse
Quoted 3 lines · Javier Cardona
You need a live endpoint to send a CMD_802_11_RESET, which you won't have if the device did not respond to USB enumeration because it was in standalone mode.

Quoted 5 lines

Quoted 2 lines
Hm, did that behaviour change recently? I'm sure this recovery was working before.
I don't think we've ever been able to communicate over USB with a dongle that is in standalone mode.

Hm, I thought we had, but maybe I'm confused. At one point were we resetting the device when we unloaded the module, and we could only reinitialise the driver if we did so within 5 seconds (the 31.09 boot2 timeout). I could have sworn I'd fixed that.

It would be useful to be able to reset the device from software when it's in standalone mode -- how hard would it be to fix that?

Signature · 1 line

dwmw2

Dan Williams

2008-05-21 20:05 UTC #12 lore raw

In reply to Javier Cardona

Quoted 8 lines · Javier Cardona

David,
Quoted 5 lines · David Woodhouse
Quoted 3 lines · Javier Cardona
You need a live endpoint to send a CMD_802_11_RESET, which you won't have if the device did not respond to USB enumeration because it was in standalone mode.

Quoted 5 lines

Quoted 2 lines
Hm, did that behaviour change recently? I'm sure this recovery was working before.
I don't think we've ever been able to communicate over USB with a dongle that is in standalone mode.

And they probably don't respond to USB port resets either, do they? Our recovery mode was always:

1) CMD_802_11_RESET
2) if that didn't work, do a USB port reset

Dan

Javier Cardona

author

2008-05-22 05:24 UTC #13 lore raw

Dan,

Quoted 1 line · Dan Williams

And they probably don't respond to USB port resets either, do they?

Don't think so.

Quoted 4 lines

Our recovery mode was always:
1) CMD_802_11_RESET
2) if that didn't work, do a USB port reset

I don't think this ever worked on an active antenna in standalone mode. This may have been the recovery mode for firmware/usb problems.

Javier

Signature · 2 lines

Javier Cardona
cozybit Inc.