[PATCH/RFC] b43: remove input device usage for rfkill

[PATCH/RFC] b43: remove input device usage for rfkill

[Adel Gadllah]

Hi,
The attached patch removes the input device dependency and replaces
the polldev by a timer.
The timer polls the device and sets the rfkill state.
I build tested the patch only because I don't have access to the
hardware, hence the RFC.
Can someone with the access to the hardware test and verify this?
If it works I will submit a similar patch for b43legacy.

-----------------------
This patch removes the dependency on the input device and replaces the
polldev with a timer for polling the rfkill state.

Signed-off-by: Adel Gadllah <adel.gadllah@gmail.com>

diff --git a/drivers/net/wireless/b43/Kconfig b/drivers/net/wireless/b43/Kconfig
index 1fa043d..af15e5c 100644
--- a/drivers/net/wireless/b43/Kconfig
+++ b/drivers/net/wireless/b43/Kconfig
@@ -91,7 +91,7 @@ config B43_LEDS
 # if it's possible.
 config B43_RFKILL
 	bool
-	depends on B43 && (RFKILL = y || RFKILL = B43) && RFKILL_INPUT &&
(INPUT_POLLDEV = y || INPUT_POLLDEV = B43)
+	depends on B43 && (RFKILL = y || RFKILL = B43)
 	default y

 config B43_DEBUG
diff --git a/drivers/net/wireless/b43/rfkill.c
b/drivers/net/wireless/b43/rfkill.c
index fec5645..631c09d 100644
--- a/drivers/net/wireless/b43/rfkill.c
+++ b/drivers/net/wireless/b43/rfkill.c
@@ -61,34 +61,29 @@ static void b43_rfkill_update_state(struct b43_wldev *dev)
 }

 /* The poll callback for the hardware button. */
-static void b43_rfkill_poll(struct input_polled_dev *poll_dev)
+static void b43_rfkill_poll(unsigned long data)
 {
-	struct b43_wldev *dev = poll_dev->private;
+	struct b43_wldev *dev = (struct b43_wldev *) data;
 	struct b43_wl *wl = dev->wl;
+	struct b43_rfkill *rfk = &(dev->wl->rfkill);
 	bool enabled;
-	bool report_change = 0;

 	mutex_lock(&wl->mutex);
-	if (unlikely(b43_status(dev) < B43_STAT_INITIALIZED)) {
-		mutex_unlock(&wl->mutex);
-		return;
-	}
+	if (unlikely(b43_status(dev) < B43_STAT_INITIALIZED))
+		goto out;
+
 	enabled = b43_is_hw_radio_enabled(dev);
 	if (unlikely(enabled != dev->radio_hw_enable)) {
 		dev->radio_hw_enable = enabled;
-		report_change = 1;
 		b43_rfkill_update_state(dev);
 		b43info(wl, "Radio hardware status changed to %s\n",
 			enabled ? "ENABLED" : "DISABLED");
 	}
-	mutex_unlock(&wl->mutex);
+out:
+	rfk->poll_timer->expires += B43_RFKILL_POLL_DELAY;
+	add_timer(rfk->poll_timer);

-	/* send the radio switch event to the system - note both a key press
-	 * and a release are required */
-	if (unlikely(report_change)) {
-		input_report_key(poll_dev->input, KEY_WLAN, 1);
-		input_report_key(poll_dev->input, KEY_WLAN, 0);
-	}
+	mutex_unlock(&wl->mutex);
 }

 /* Called when the RFKILL toggled in software. */
@@ -158,51 +153,23 @@ void b43_rfkill_init(struct b43_wldev *dev)
 	rfk->rfkill->toggle_radio = b43_rfkill_soft_toggle;
 	rfk->rfkill->user_claim_unsupported = 1;

-	rfk->poll_dev = input_allocate_polled_device();
-	if (!rfk->poll_dev) {
-		rfkill_free(rfk->rfkill);
-		goto err_freed_rfk;
-	}
-
-	rfk->poll_dev->private = dev;
-	rfk->poll_dev->poll = b43_rfkill_poll;
-	rfk->poll_dev->poll_interval = 1000; /* msecs */
-
-	rfk->poll_dev->input->name = rfk->name;
-	rfk->poll_dev->input->id.bustype = BUS_HOST;
-	rfk->poll_dev->input->id.vendor = dev->dev->bus->boardinfo.vendor;
-	rfk->poll_dev->input->evbit[0] = BIT(EV_KEY);
-	set_bit(KEY_WLAN, rfk->poll_dev->input->keybit);
+	setup_timer(rfk->poll_timer, &b43_rfkill_poll, (unsigned long) dev);
+	rfk->poll_timer->expires = jiffies + B43_RFKILL_POLL_DELAY;

 	err = rfkill_register(rfk->rfkill);
 	if (err)
-		goto err_free_polldev;
-
-#ifdef CONFIG_RFKILL_INPUT_MODULE
-	/* B43 RF-kill isn't useful without the rfkill-input subsystem.
-	 * Try to load the module. */
-	err = request_module("rfkill-input");
-	if (err)
-		b43warn(wl, "Failed to load the rfkill-input module. "
-			"The built-in radio LED will not work.\n");
-#endif /* CONFIG_RFKILL_INPUT */
-
-	err = input_register_polled_device(rfk->poll_dev);
-	if (err)
-		goto err_unreg_rfk;
+		goto err_freed_rfk;

 	rfk->registered = 1;
+	add_timer(rfk->poll_timer);

 	return;
-err_unreg_rfk:
-	rfkill_unregister(rfk->rfkill);
-err_free_polldev:
-	input_free_polled_device(rfk->poll_dev);
-	rfk->poll_dev = NULL;
+
 err_freed_rfk:
 	rfk->rfkill = NULL;
 out_error:
 	rfk->registered = 0;
+	del_timer_sync(rfk->poll_timer);
 	b43warn(wl, "RF-kill button init failed\n");
 }

@@ -213,10 +180,7 @@ void b43_rfkill_exit(struct b43_wldev *dev)
 	if (!rfk->registered)
 		return;
 	rfk->registered = 0;
-
-	input_unregister_polled_device(rfk->poll_dev);
+	del_timer_sync(rfk->poll_timer);
 	rfkill_unregister(rfk->rfkill);
-	input_free_polled_device(rfk->poll_dev);
-	rfk->poll_dev = NULL;
 	rfk->rfkill = NULL;
 }
diff --git a/drivers/net/wireless/b43/rfkill.h
b/drivers/net/wireless/b43/rfkill.h
index adacf93..d25fbbd 100644
--- a/drivers/net/wireless/b43/rfkill.h
+++ b/drivers/net/wireless/b43/rfkill.h
@@ -7,14 +7,16 @@ struct b43_wldev;
 #ifdef CONFIG_B43_RFKILL

 #include <linux/rfkill.h>
-#include <linux/input-polldev.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>

+#define B43_RFKILL_POLL_DELAY msecs_to_jiffies(1000)

 struct b43_rfkill {
 	/* The RFKILL subsystem data structure */
 	struct rfkill *rfkill;
-	/* The poll device for the RFKILL input button */
-	struct input_polled_dev *poll_dev;
+	/* Timer for polling the rfkill state */
+	struct timer_list *poll_timer;
 	/* Did initialization succeed? Used for freeing. */
 	bool registered;
 	/* The unique name of this rfkill switch */

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Johannes Berg]

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


> +	rfk->poll_timer->expires = jiffies + B43_RFKILL_POLL_DELAY;

round_jiffies.

johannes

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

[PATCH/RFC v2] b43: remove input device usage for rfkill

[Adel Gadllah]

2008/7/1 Johannes Berg <johannes@sipsolutions.net>:
>
>> +     rfk->poll_timer->expires = jiffies + B43_RFKILL_POLL_DELAY;
>
> round_jiffies.

v2 attached.
-----
This patch removes the dependency on the input device and replaces the
polldev with a timer for polling the rfkill state.

Signed-off-by: Adel Gadllah <adel.gadllah@gmail.com>

diff --git a/drivers/net/wireless/b43/Kconfig b/drivers/net/wireless/b43/Kconfig
index 1fa043d..af15e5c 100644
--- a/drivers/net/wireless/b43/Kconfig
+++ b/drivers/net/wireless/b43/Kconfig
@@ -91,7 +91,7 @@ config B43_LEDS
 # if it's possible.
 config B43_RFKILL
 	bool
-	depends on B43 && (RFKILL = y || RFKILL = B43) && RFKILL_INPUT &&
(INPUT_POLLDEV = y || INPUT_POLLDEV = B43)
+	depends on B43 && (RFKILL = y || RFKILL = B43)
 	default y

 config B43_DEBUG
diff --git a/drivers/net/wireless/b43/rfkill.c
b/drivers/net/wireless/b43/rfkill.c
index fec5645..dde5cd4 100644
--- a/drivers/net/wireless/b43/rfkill.c
+++ b/drivers/net/wireless/b43/rfkill.c
@@ -61,34 +61,29 @@ static void b43_rfkill_update_state(struct b43_wldev *dev)
 }

 /* The poll callback for the hardware button. */
-static void b43_rfkill_poll(struct input_polled_dev *poll_dev)
+static void b43_rfkill_poll(unsigned long data)
 {
-	struct b43_wldev *dev = poll_dev->private;
+	struct b43_wldev *dev = (struct b43_wldev *) data;
 	struct b43_wl *wl = dev->wl;
+	struct b43_rfkill *rfk = &(dev->wl->rfkill);
 	bool enabled;
-	bool report_change = 0;

 	mutex_lock(&wl->mutex);
-	if (unlikely(b43_status(dev) < B43_STAT_INITIALIZED)) {
-		mutex_unlock(&wl->mutex);
-		return;
-	}
+	if (unlikely(b43_status(dev) < B43_STAT_INITIALIZED))
+		goto out;
+
 	enabled = b43_is_hw_radio_enabled(dev);
 	if (unlikely(enabled != dev->radio_hw_enable)) {
 		dev->radio_hw_enable = enabled;
-		report_change = 1;
 		b43_rfkill_update_state(dev);
 		b43info(wl, "Radio hardware status changed to %s\n",
 			enabled ? "ENABLED" : "DISABLED");
 	}
-	mutex_unlock(&wl->mutex);
+out:
+	rfk->poll_timer->expires += B43_RFKILL_POLL_DELAY;
+	add_timer(rfk->poll_timer);

-	/* send the radio switch event to the system - note both a key press
-	 * and a release are required */
-	if (unlikely(report_change)) {
-		input_report_key(poll_dev->input, KEY_WLAN, 1);
-		input_report_key(poll_dev->input, KEY_WLAN, 0);
-	}
+	mutex_unlock(&wl->mutex);
 }

 /* Called when the RFKILL toggled in software. */
@@ -158,51 +153,24 @@ void b43_rfkill_init(struct b43_wldev *dev)
 	rfk->rfkill->toggle_radio = b43_rfkill_soft_toggle;
 	rfk->rfkill->user_claim_unsupported = 1;

-	rfk->poll_dev = input_allocate_polled_device();
-	if (!rfk->poll_dev) {
-		rfkill_free(rfk->rfkill);
-		goto err_freed_rfk;
-	}
-
-	rfk->poll_dev->private = dev;
-	rfk->poll_dev->poll = b43_rfkill_poll;
-	rfk->poll_dev->poll_interval = 1000; /* msecs */
-
-	rfk->poll_dev->input->name = rfk->name;
-	rfk->poll_dev->input->id.bustype = BUS_HOST;
-	rfk->poll_dev->input->id.vendor = dev->dev->bus->boardinfo.vendor;
-	rfk->poll_dev->input->evbit[0] = BIT(EV_KEY);
-	set_bit(KEY_WLAN, rfk->poll_dev->input->keybit);
+	setup_timer(rfk->poll_timer, &b43_rfkill_poll, (unsigned long) dev);
+	rfk->poll_timer->expires =
+		round_jiffies(jiffies + B43_RFKILL_POLL_DELAY);

 	err = rfkill_register(rfk->rfkill);
 	if (err)
-		goto err_free_polldev;
-
-#ifdef CONFIG_RFKILL_INPUT_MODULE
-	/* B43 RF-kill isn't useful without the rfkill-input subsystem.
-	 * Try to load the module. */
-	err = request_module("rfkill-input");
-	if (err)
-		b43warn(wl, "Failed to load the rfkill-input module. "
-			"The built-in radio LED will not work.\n");
-#endif /* CONFIG_RFKILL_INPUT */
-
-	err = input_register_polled_device(rfk->poll_dev);
-	if (err)
-		goto err_unreg_rfk;
+		goto err_freed_rfk;

 	rfk->registered = 1;
+	add_timer(rfk->poll_timer);

 	return;
-err_unreg_rfk:
-	rfkill_unregister(rfk->rfkill);
-err_free_polldev:
-	input_free_polled_device(rfk->poll_dev);
-	rfk->poll_dev = NULL;
+
 err_freed_rfk:
 	rfk->rfkill = NULL;
 out_error:
 	rfk->registered = 0;
+	del_timer_sync(rfk->poll_timer);
 	b43warn(wl, "RF-kill button init failed\n");
 }

@@ -213,10 +181,7 @@ void b43_rfkill_exit(struct b43_wldev *dev)
 	if (!rfk->registered)
 		return;
 	rfk->registered = 0;
-
-	input_unregister_polled_device(rfk->poll_dev);
+	del_timer_sync(rfk->poll_timer);
 	rfkill_unregister(rfk->rfkill);
-	input_free_polled_device(rfk->poll_dev);
-	rfk->poll_dev = NULL;
 	rfk->rfkill = NULL;
 }
diff --git a/drivers/net/wireless/b43/rfkill.h
b/drivers/net/wireless/b43/rfkill.h
index adacf93..d25fbbd 100644
--- a/drivers/net/wireless/b43/rfkill.h
+++ b/drivers/net/wireless/b43/rfkill.h
@@ -7,14 +7,16 @@ struct b43_wldev;
 #ifdef CONFIG_B43_RFKILL

 #include <linux/rfkill.h>
-#include <linux/input-polldev.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>

+#define B43_RFKILL_POLL_DELAY msecs_to_jiffies(1000)

 struct b43_rfkill {
 	/* The RFKILL subsystem data structure */
 	struct rfkill *rfkill;
-	/* The poll device for the RFKILL input button */
-	struct input_polled_dev *poll_dev;
+	/* Timer for polling the rfkill state */
+	struct timer_list *poll_timer;
 	/* Did initialization succeed? Used for freeing. */
 	bool registered;
 	/* The unique name of this rfkill switch */

Re: [PATCH/RFC v2] b43: remove input device usage for rfkill

[Johannes Berg]

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

On Tue, 2008-07-01 at 12:05 +0200, Adel Gadllah wrote:
> 2008/7/1 Johannes Berg <johannes@sipsolutions.net>:
> >
> >> +     rfk->poll_timer->expires = jiffies + B43_RFKILL_POLL_DELAY;
> >
> > round_jiffies.
> 
> v2 attached.

That wasn't meant _literally_ but _conceptually_. You want that in the
other places too.

johannes

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

[PATCH/RFC v3] b43: remove input device usage for rfkill

[Adel Gadllah]

2008/7/1 Johannes Berg <johannes@sipsolutions.net>:
> On Tue, 2008-07-01 at 12:05 +0200, Adel Gadllah wrote:
>> 2008/7/1 Johannes Berg <johannes@sipsolutions.net>:
>> >
>> >> +     rfk->poll_timer->expires = jiffies + B43_RFKILL_POLL_DELAY;
>> >
>> > round_jiffies.
>>
>> v2 attached.
>
> That wasn't meant _literally_ but _conceptually_. You want that in the
> other places too.

ok, done in v3.

------------------------
This patch removes the dependency on the input device and replaces the
polldev with a timer for polling the rfkill state.

Signed-off-by: Adel Gadllah <adel.gadllah@gmail.com>

diff --git a/drivers/net/wireless/b43/Kconfig b/drivers/net/wireless/b43/Kconfig
index 1fa043d..af15e5c 100644
--- a/drivers/net/wireless/b43/Kconfig
+++ b/drivers/net/wireless/b43/Kconfig
@@ -91,7 +91,7 @@ config B43_LEDS
 # if it's possible.
 config B43_RFKILL
 	bool
-	depends on B43 && (RFKILL = y || RFKILL = B43) && RFKILL_INPUT &&
(INPUT_POLLDEV = y || INPUT_POLLDEV = B43)
+	depends on B43 && (RFKILL = y || RFKILL = B43)
 	default y

 config B43_DEBUG
diff --git a/drivers/net/wireless/b43/rfkill.c
b/drivers/net/wireless/b43/rfkill.c
index fec5645..8a2f604 100644
--- a/drivers/net/wireless/b43/rfkill.c
+++ b/drivers/net/wireless/b43/rfkill.c
@@ -61,34 +61,29 @@ static void b43_rfkill_update_state(struct b43_wldev *dev)
 }

 /* The poll callback for the hardware button. */
-static void b43_rfkill_poll(struct input_polled_dev *poll_dev)
+static void b43_rfkill_poll(unsigned long data)
 {
-	struct b43_wldev *dev = poll_dev->private;
+	struct b43_wldev *dev = (struct b43_wldev *) data;
 	struct b43_wl *wl = dev->wl;
+	struct b43_rfkill *rfk = &(dev->wl->rfkill);
 	bool enabled;
-	bool report_change = 0;

 	mutex_lock(&wl->mutex);
-	if (unlikely(b43_status(dev) < B43_STAT_INITIALIZED)) {
-		mutex_unlock(&wl->mutex);
-		return;
-	}
+	if (unlikely(b43_status(dev) < B43_STAT_INITIALIZED))
+		goto out;
+
 	enabled = b43_is_hw_radio_enabled(dev);
 	if (unlikely(enabled != dev->radio_hw_enable)) {
 		dev->radio_hw_enable = enabled;
-		report_change = 1;
 		b43_rfkill_update_state(dev);
 		b43info(wl, "Radio hardware status changed to %s\n",
 			enabled ? "ENABLED" : "DISABLED");
 	}
-	mutex_unlock(&wl->mutex);
+out:
+	rfk->poll_timer->expires += round_jiffies(B43_RFKILL_POLL_DELAY);
+	add_timer(rfk->poll_timer);

-	/* send the radio switch event to the system - note both a key press
-	 * and a release are required */
-	if (unlikely(report_change)) {
-		input_report_key(poll_dev->input, KEY_WLAN, 1);
-		input_report_key(poll_dev->input, KEY_WLAN, 0);
-	}
+	mutex_unlock(&wl->mutex);
 }

 /* Called when the RFKILL toggled in software. */
@@ -158,51 +153,24 @@ void b43_rfkill_init(struct b43_wldev *dev)
 	rfk->rfkill->toggle_radio = b43_rfkill_soft_toggle;
 	rfk->rfkill->user_claim_unsupported = 1;

-	rfk->poll_dev = input_allocate_polled_device();
-	if (!rfk->poll_dev) {
-		rfkill_free(rfk->rfkill);
-		goto err_freed_rfk;
-	}
-
-	rfk->poll_dev->private = dev;
-	rfk->poll_dev->poll = b43_rfkill_poll;
-	rfk->poll_dev->poll_interval = 1000; /* msecs */
-
-	rfk->poll_dev->input->name = rfk->name;
-	rfk->poll_dev->input->id.bustype = BUS_HOST;
-	rfk->poll_dev->input->id.vendor = dev->dev->bus->boardinfo.vendor;
-	rfk->poll_dev->input->evbit[0] = BIT(EV_KEY);
-	set_bit(KEY_WLAN, rfk->poll_dev->input->keybit);
+	setup_timer(rfk->poll_timer, &b43_rfkill_poll, (unsigned long) dev);
+	rfk->poll_timer->expires =
+		round_jiffies(jiffies + B43_RFKILL_POLL_DELAY);

 	err = rfkill_register(rfk->rfkill);
 	if (err)
-		goto err_free_polldev;
-
-#ifdef CONFIG_RFKILL_INPUT_MODULE
-	/* B43 RF-kill isn't useful without the rfkill-input subsystem.
-	 * Try to load the module. */
-	err = request_module("rfkill-input");
-	if (err)
-		b43warn(wl, "Failed to load the rfkill-input module. "
-			"The built-in radio LED will not work.\n");
-#endif /* CONFIG_RFKILL_INPUT */
-
-	err = input_register_polled_device(rfk->poll_dev);
-	if (err)
-		goto err_unreg_rfk;
+		goto err_freed_rfk;

 	rfk->registered = 1;
+	add_timer(rfk->poll_timer);

 	return;
-err_unreg_rfk:
-	rfkill_unregister(rfk->rfkill);
-err_free_polldev:
-	input_free_polled_device(rfk->poll_dev);
-	rfk->poll_dev = NULL;
+
 err_freed_rfk:
 	rfk->rfkill = NULL;
 out_error:
 	rfk->registered = 0;
+	del_timer_sync(rfk->poll_timer);
 	b43warn(wl, "RF-kill button init failed\n");
 }

@@ -213,10 +181,7 @@ void b43_rfkill_exit(struct b43_wldev *dev)
 	if (!rfk->registered)
 		return;
 	rfk->registered = 0;
-
-	input_unregister_polled_device(rfk->poll_dev);
+	del_timer_sync(rfk->poll_timer);
 	rfkill_unregister(rfk->rfkill);
-	input_free_polled_device(rfk->poll_dev);
-	rfk->poll_dev = NULL;
 	rfk->rfkill = NULL;
 }
diff --git a/drivers/net/wireless/b43/rfkill.h
b/drivers/net/wireless/b43/rfkill.h
index adacf93..d25fbbd 100644
--- a/drivers/net/wireless/b43/rfkill.h
+++ b/drivers/net/wireless/b43/rfkill.h
@@ -7,14 +7,16 @@ struct b43_wldev;
 #ifdef CONFIG_B43_RFKILL

 #include <linux/rfkill.h>
-#include <linux/input-polldev.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>

+#define B43_RFKILL_POLL_DELAY msecs_to_jiffies(1000)

 struct b43_rfkill {
 	/* The RFKILL subsystem data structure */
 	struct rfkill *rfkill;
-	/* The poll device for the RFKILL input button */
-	struct input_polled_dev *poll_dev;
+	/* Timer for polling the rfkill state */
+	struct timer_list *poll_timer;
 	/* Did initialization succeed? Used for freeing. */
 	bool registered;
 	/* The unique name of this rfkill switch */

Re: [PATCH/RFC v3] b43: remove input device usage for rfkill

[Johannes Berg]

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


> +	rfk->poll_timer->expires += round_jiffies(B43_RFKILL_POLL_DELAY);

Umm, no. Can you please read the docs for round_jiffies?

johannes

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

Re: [PATCH/RFC v3] b43: remove input device usage for rfkill

[drago01]

On Tue, Jul 1, 2008 at 12:23 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> +     rfk->poll_timer->expires += round_jiffies(B43_RFKILL_POLL_DELAY);
>
> Umm, no. Can you please read the docs for round_jiffies?

did now, sorry for being stupid.

Re: [PATCH/RFC v3] b43: remove input device usage for rfkill

[Michael Buesch]

On Tuesday 01 July 2008 12:19:35 Adel Gadllah wrote:

> +out:
> +	rfk->poll_timer->expires += round_jiffies(B43_RFKILL_POLL_DELAY);

round_jiffies does only make sense for absolute times.
So this needs to be 
	rfk->poll_timer->expires = round_jiffies(jiffies + B43_RFKILL_POLL_DELAY);
However, timer is broken. Use a workqueue.



-- 
Greetings Michael.

Re: [PATCH/RFC v3] b43: remove input device usage for rfkill

[drago01]

On Tue, Jul 1, 2008 at 12:29 PM, Michael Buesch <mb@bu3sch.de> wrote:
> On Tuesday 01 July 2008 12:19:35 Adel Gadllah wrote:
>
>> +out:
>> +     rfk->poll_timer->expires += round_jiffies(B43_RFKILL_POLL_DELAY);
>
> round_jiffies does only make sense for absolute times.
> So this needs to be
>        rfk->poll_timer->expires = round_jiffies(jiffies + B43_RFKILL_POLL_DELAY);
> However, timer is broken. Use a workqueue.

OK, will submit a new patch if we still want to remove the input device.

Re: [PATCH/RFC v3] b43: remove input device usage for rfkill

[Ivo van Doorn]

> -	/* send the radio switch event to the system - note both a key press
> -	 * and a release are required */
> -	if (unlikely(report_change)) {
> -		input_report_key(poll_dev->input, KEY_WLAN, 1);
> -		input_report_key(poll_dev->input, KEY_WLAN, 0);
> -	}

So how will b43 report the status change now? Shouldn't the above be replaced
with a call to rfkill_force_state()?

Ivo

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Michael Buesch]

On Tuesday 01 July 2008 11:55:11 Adel Gadllah wrote:
> Hi,
> The attached patch removes the input device dependency and replaces
> the polldev by a timer.
> The timer polls the device and sets the rfkill state.
> I build tested the patch only because I don't have access to the
> hardware, hence the RFC.
> Can someone with the access to the hardware test and verify this?
> If it works I will submit a similar patch for b43legacy.
> 
> -----------------------
> This patch removes the dependency on the input device and replaces the
> polldev with a timer for polling the rfkill state.


I'm pretty sure this will generate a lot of bugreports complaining that
rfkill silently broke, as the userspace is not setup correctly.


> Signed-off-by: Adel Gadllah <adel.gadllah@gmail.com>
> 
> diff --git a/drivers/net/wireless/b43/Kconfig b/drivers/net/wireless/b43/Kconfig
> index 1fa043d..af15e5c 100644
> --- a/drivers/net/wireless/b43/Kconfig
> +++ b/drivers/net/wireless/b43/Kconfig
> @@ -91,7 +91,7 @@ config B43_LEDS
>  # if it's possible.
>  config B43_RFKILL
>  	bool
> -	depends on B43 && (RFKILL = y || RFKILL = B43) && RFKILL_INPUT &&
> (INPUT_POLLDEV = y || INPUT_POLLDEV = B43)
> +	depends on B43 && (RFKILL = y || RFKILL = B43)
>  	default y
> 
>  config B43_DEBUG
> diff --git a/drivers/net/wireless/b43/rfkill.c
> b/drivers/net/wireless/b43/rfkill.c
> index fec5645..631c09d 100644
> --- a/drivers/net/wireless/b43/rfkill.c
> +++ b/drivers/net/wireless/b43/rfkill.c
> @@ -61,34 +61,29 @@ static void b43_rfkill_update_state(struct b43_wldev *dev)
>  }
> 
>  /* The poll callback for the hardware button. */
> -static void b43_rfkill_poll(struct input_polled_dev *poll_dev)
> +static void b43_rfkill_poll(unsigned long data)
>  {
> -	struct b43_wldev *dev = poll_dev->private;
> +	struct b43_wldev *dev = (struct b43_wldev *) data;
>  	struct b43_wl *wl = dev->wl;
> +	struct b43_rfkill *rfk = &(dev->wl->rfkill);
>  	bool enabled;
> -	bool report_change = 0;
> 
>  	mutex_lock(&wl->mutex);

We cannot sleep in a timer. Please use a deferred work instead of s timer.
Queue the work on the mac80211 workqueue.

> -	if (unlikely(b43_status(dev) < B43_STAT_INITIALIZED)) {
> -		mutex_unlock(&wl->mutex);
> -		return;
> -	}
> +	if (unlikely(b43_status(dev) < B43_STAT_INITIALIZED))
> +		goto out;
> +
>  	enabled = b43_is_hw_radio_enabled(dev);
>  	if (unlikely(enabled != dev->radio_hw_enable)) {
>  		dev->radio_hw_enable = enabled;
> -		report_change = 1;
>  		b43_rfkill_update_state(dev);
>  		b43info(wl, "Radio hardware status changed to %s\n",
>  			enabled ? "ENABLED" : "DISABLED");
>  	}
> -	mutex_unlock(&wl->mutex);
> +out:
> +	rfk->poll_timer->expires += B43_RFKILL_POLL_DELAY;
> +	add_timer(rfk->poll_timer);
> 
> -	/* send the radio switch event to the system - note both a key press
> -	 * and a release are required */
> -	if (unlikely(report_change)) {
> -		input_report_key(poll_dev->input, KEY_WLAN, 1);
> -		input_report_key(poll_dev->input, KEY_WLAN, 0);
> -	}
> +	mutex_unlock(&wl->mutex);
>  }
> 
>  /* Called when the RFKILL toggled in software. */
> @@ -158,51 +153,23 @@ void b43_rfkill_init(struct b43_wldev *dev)
>  	rfk->rfkill->toggle_radio = b43_rfkill_soft_toggle;
>  	rfk->rfkill->user_claim_unsupported = 1;
> 
> -	rfk->poll_dev = input_allocate_polled_device();
> -	if (!rfk->poll_dev) {
> -		rfkill_free(rfk->rfkill);
> -		goto err_freed_rfk;
> -	}
> -
> -	rfk->poll_dev->private = dev;
> -	rfk->poll_dev->poll = b43_rfkill_poll;
> -	rfk->poll_dev->poll_interval = 1000; /* msecs */
> -
> -	rfk->poll_dev->input->name = rfk->name;
> -	rfk->poll_dev->input->id.bustype = BUS_HOST;
> -	rfk->poll_dev->input->id.vendor = dev->dev->bus->boardinfo.vendor;
> -	rfk->poll_dev->input->evbit[0] = BIT(EV_KEY);
> -	set_bit(KEY_WLAN, rfk->poll_dev->input->keybit);
> +	setup_timer(rfk->poll_timer, &b43_rfkill_poll, (unsigned long) dev);
> +	rfk->poll_timer->expires = jiffies + B43_RFKILL_POLL_DELAY;
> 
>  	err = rfkill_register(rfk->rfkill);
>  	if (err)
> -		goto err_free_polldev;
> -
> -#ifdef CONFIG_RFKILL_INPUT_MODULE
> -	/* B43 RF-kill isn't useful without the rfkill-input subsystem.
> -	 * Try to load the module. */
> -	err = request_module("rfkill-input");
> -	if (err)
> -		b43warn(wl, "Failed to load the rfkill-input module. "
> -			"The built-in radio LED will not work.\n");
> -#endif /* CONFIG_RFKILL_INPUT */
> -
> -	err = input_register_polled_device(rfk->poll_dev);
> -	if (err)
> -		goto err_unreg_rfk;
> +		goto err_freed_rfk;
> 
>  	rfk->registered = 1;
> +	add_timer(rfk->poll_timer);
> 
>  	return;
> -err_unreg_rfk:
> -	rfkill_unregister(rfk->rfkill);
> -err_free_polldev:
> -	input_free_polled_device(rfk->poll_dev);
> -	rfk->poll_dev = NULL;
> +
>  err_freed_rfk:
>  	rfk->rfkill = NULL;
>  out_error:
>  	rfk->registered = 0;
> +	del_timer_sync(rfk->poll_timer);
>  	b43warn(wl, "RF-kill button init failed\n");
>  }
> 
> @@ -213,10 +180,7 @@ void b43_rfkill_exit(struct b43_wldev *dev)
>  	if (!rfk->registered)
>  		return;
>  	rfk->registered = 0;
> -
> -	input_unregister_polled_device(rfk->poll_dev);
> +	del_timer_sync(rfk->poll_timer);
>  	rfkill_unregister(rfk->rfkill);
> -	input_free_polled_device(rfk->poll_dev);
> -	rfk->poll_dev = NULL;
>  	rfk->rfkill = NULL;
>  }
> diff --git a/drivers/net/wireless/b43/rfkill.h
> b/drivers/net/wireless/b43/rfkill.h
> index adacf93..d25fbbd 100644
> --- a/drivers/net/wireless/b43/rfkill.h
> +++ b/drivers/net/wireless/b43/rfkill.h
> @@ -7,14 +7,16 @@ struct b43_wldev;
>  #ifdef CONFIG_B43_RFKILL
> 
>  #include <linux/rfkill.h>
> -#include <linux/input-polldev.h>
> +#include <linux/timer.h>
> +#include <linux/jiffies.h>
> 
> +#define B43_RFKILL_POLL_DELAY msecs_to_jiffies(1000)
> 
>  struct b43_rfkill {
>  	/* The RFKILL subsystem data structure */
>  	struct rfkill *rfkill;
> -	/* The poll device for the RFKILL input button */
> -	struct input_polled_dev *poll_dev;
> +	/* Timer for polling the rfkill state */
> +	struct timer_list *poll_timer;
>  	/* Did initialization succeed? Used for freeing. */
>  	bool registered;
>  	/* The unique name of this rfkill switch */
> 
> 



-- 
Greetings Michael.

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[drago01]

On Tue, Jul 1, 2008 at 12:27 PM, Michael Buesch <mb@bu3sch.de> wrote:
> On Tuesday 01 July 2008 11:55:11 Adel Gadllah wrote:
>> Hi,
>> The attached patch removes the input device dependency and replaces
>> the polldev by a timer.
>> The timer polls the device and sets the rfkill state.
>> I build tested the patch only because I don't have access to the
>> hardware, hence the RFC.
>> Can someone with the access to the hardware test and verify this?
>> If it works I will submit a similar patch for b43legacy.
>>
>> -----------------------
>> This patch removes the dependency on the input device and replaces the
>> polldev with a timer for polling the rfkill state.
>
>
> I'm pretty sure this will generate a lot of bugreports complaining that
> rfkill silently broke, as the userspace is not setup correctly.

OK, so how to proceed? Just leave the input device?
Henrique suggested that it should be removed.

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Michael Buesch]

On Tuesday 01 July 2008 12:33:17 drago01 wrote:
> On Tue, Jul 1, 2008 at 12:27 PM, Michael Buesch <mb@bu3sch.de> wrote:
> > On Tuesday 01 July 2008 11:55:11 Adel Gadllah wrote:
> >> Hi,
> >> The attached patch removes the input device dependency and replaces
> >> the polldev by a timer.
> >> The timer polls the device and sets the rfkill state.
> >> I build tested the patch only because I don't have access to the
> >> hardware, hence the RFC.
> >> Can someone with the access to the hardware test and verify this?
> >> If it works I will submit a similar patch for b43legacy.
> >>
> >> -----------------------
> >> This patch removes the dependency on the input device and replaces the
> >> polldev with a timer for polling the rfkill state.
> >
> >
> > I'm pretty sure this will generate a lot of bugreports complaining that
> > rfkill silently broke, as the userspace is not setup correctly.
> 
> OK, so how to proceed? Just leave the input device?

No well. This probably is the right way to go. However I just wanted to
say that I'm pretty sure it will generate lots of bugreports that I
will ignore. ;)

But I am wondering _why_ we need to turn all this upside down.
What's so bad about having an input device in the kernel? What's the
advantage of having userspace do the work?

-- 
Greetings Michael.

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Henrique de Moraes Holschuh]

Ok, let's take it slow and one question/answer at a time, then.

First, I need to know some details about the B43 hardware, to help with
this.

1. It has an input pin that sometimes people connect buttons/switches
to.

Does that input pin act as an input for a FLIP-FLOP (and that flip-flop
output is the hardware rfkill line), or is it the hardware rfkill line
itself?

input for a flip-flop means it toggles the kill/unkill behaviour when
the input pin detects an edge (press/release of a button).

direct hardware rfkill line means you need a SWITCH in that pin, because
it directly sets the state.  If the pin is active, the hardware rfkill
line is active.  If the pin is inactive, the hardware rfkill line is
inactive.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Johannes Berg]

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


> 1. It has an input pin that sometimes people connect buttons/switches
> to.
> 
> Does that input pin act as an input for a FLIP-FLOP (and that flip-flop
> output is the hardware rfkill line), or is it the hardware rfkill line
> itself?

Neither. It's not hw kill, but it's not just edge detect either, it does
have 'kill' and 'not kill' states.

johannes

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

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Henrique de Moraes Holschuh]

On Tue, 01 Jul 2008, Johannes Berg wrote:
> > 1. It has an input pin that sometimes people connect buttons/switches
> > to.
> > 
> > Does that input pin act as an input for a FLIP-FLOP (and that flip-flop
> > output is the hardware rfkill line), or is it the hardware rfkill line
> > itself?
> 
> Neither. It's not hw kill, but it's not just edge detect either, it does
> have 'kill' and 'not kill' states.

What exactly the input pin does?  what exactly the hardware does when
the input pin state changes?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Johannes Berg]

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

On Tue, 2008-07-01 at 13:50 -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 01 Jul 2008, Johannes Berg wrote:
> > > 1. It has an input pin that sometimes people connect buttons/switches
> > > to.
> > > 
> > > Does that input pin act as an input for a FLIP-FLOP (and that flip-flop
> > > output is the hardware rfkill line), or is it the hardware rfkill line
> > > itself?
> > 
> > Neither. It's not hw kill, but it's not just edge detect either, it does
> > have 'kill' and 'not kill' states.
> 
> What exactly the input pin does?  what exactly the hardware does when
> the input pin state changes?

The hardware does nothing, it just sets a bit high or low depending on
the input pin.

johannes

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

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Larry Finger]

Johannes Berg wrote:
> On Tue, 2008-07-01 at 13:50 -0300, Henrique de Moraes Holschuh wrote:
>> On Tue, 01 Jul 2008, Johannes Berg wrote:
>>>> 1. It has an input pin that sometimes people connect buttons/switches
>>>> to.
>>>>
>>>> Does that input pin act as an input for a FLIP-FLOP (and that flip-flop
>>>> output is the hardware rfkill line), or is it the hardware rfkill line
>>>> itself?
>>> Neither. It's not hw kill, but it's not just edge detect either, it does
>>> have 'kill' and 'not kill' states.
>> What exactly the input pin does?  what exactly the hardware does when
>> the input pin state changes?
> 
> The hardware does nothing, it just sets a bit high or low depending on
> the input pin.

On my HP laptop using the RFKILL code currently in wireless-testing, 
sliding the "Wireless switch" results in b43_rfkill_poll being called. 
It then issues KEY_WLAN press and release events. This changes the 
state of the wireless light and the contents of a read-only bit in the 
status register of the device. I believe that if this bit is off, the 
radio is blocked by the hardware.

Larry

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Henrique de Moraes Holschuh]

On Tue, 01 Jul 2008, Larry Finger wrote:
> On my HP laptop using the RFKILL code currently in wireless-testing,  
> sliding the "Wireless switch" results in b43_rfkill_poll being called.  

Can you confirm (remove rfkill-input, hp-wmi, and anything else that could
mess with the results) that the HP laptop wireless switch is wired directly
to that b43 input pin?

Is it wired to anything else? e.g. can hp-wmi see that switch?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Larry Finger]

Henrique de Moraes Holschuh wrote:
> On Tue, 01 Jul 2008, Larry Finger wrote:
>> On my HP laptop using the RFKILL code currently in wireless-testing,  
>> sliding the "Wireless switch" results in b43_rfkill_poll being called.  
> 
> Can you confirm (remove rfkill-input, hp-wmi, and anything else that could
> mess with the results) that the HP laptop wireless switch is wired directly
> to that b43 input pin?
> 
> Is it wired to anything else? e.g. can hp-wmi see that switch?

I removed rfkill_input and toggled the switch off - b43 reported that 
the hardware switch was set to DISABLED, but the light remained on. 
When the switch was turned on, b43 reported that it was ENABLED. It 
certainly seems that the switch is wired to some pin on the PCIe 
connector used by my BCM4312.

 From what I know, hp-wmi is a Windows process - when I need to use 
Windows, I go to a different computer.

Larry

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Dan Williams]

On Tue, 2008-07-01 at 13:21 -0500, Larry Finger wrote:
> Henrique de Moraes Holschuh wrote:
> > On Tue, 01 Jul 2008, Larry Finger wrote:
> >> On my HP laptop using the RFKILL code currently in wireless-testing,  
> >> sliding the "Wireless switch" results in b43_rfkill_poll being called.  
> > 
> > Can you confirm (remove rfkill-input, hp-wmi, and anything else that could
> > mess with the results) that the HP laptop wireless switch is wired directly
> > to that b43 input pin?
> > 
> > Is it wired to anything else? e.g. can hp-wmi see that switch?
> 
> I removed rfkill_input and toggled the switch off - b43 reported that 
> the hardware switch was set to DISABLED, but the light remained on. 
> When the switch was turned on, b43 reported that it was ENABLED. It 
> certainly seems that the switch is wired to some pin on the PCIe 
> connector used by my BCM4312.
> 
>  From what I know, hp-wmi is a Windows process - when I need to use 
> Windows, I go to a different computer.

No, it's a linux kernel module for HP laptops that talks to BIOS
routines to get the state of the killswitch on some HP laptops.
Unfortunately this has to happen through WMI hooks, even on Linux.

Dan

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Larry Finger]

Dan Williams wrote:
> 
> No, it's a linux kernel module for HP laptops that talks to BIOS
> routines to get the state of the killswitch on some HP laptops.
> Unfortunately this has to happen through WMI hooks, even on Linux.

I do not have that module configured, thus it cannot get in the way.

Larry

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Henrique de Moraes Holschuh]

On Tue, 01 Jul 2008, Johannes Berg wrote:
> On Tue, 2008-07-01 at 13:50 -0300, Henrique de Moraes Holschuh wrote:
> > On Tue, 01 Jul 2008, Johannes Berg wrote:
> > > > 1. It has an input pin that sometimes people connect buttons/switches
> > > > to.
> > > > 
> > > > Does that input pin act as an input for a FLIP-FLOP (and that flip-flop
> > > > output is the hardware rfkill line), or is it the hardware rfkill line
> > > > itself?
> > > 
> > > Neither. It's not hw kill, but it's not just edge detect either, it does
> > > have 'kill' and 'not kill' states.
> > 
> > What exactly the input pin does?  what exactly the hardware does when
> > the input pin state changes?
> 
> The hardware does nothing, it just sets a bit high or low depending on
> the input pin.

THIS is very interesting alright...  and it is *extremely* important, so
let's triple verify it, shall we?

Please hack the driver to try to transmit data with that bit set to high, and
also with that bit set to low.  The best way to go about it is probably to
disable the rfkill support (so that it doesn't get in the way), hack the
check-the-bit function to just printk its state, and see if the radio can
effectively transmit and communicate no matter what state the input pin is
at.

This is very important.  It will caracterize that input pin as either an
hardware rfkill line, or as an input device (in which case I would be wrong
when I asked to remove the input support from b43, but one step at a time).

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Johannes Berg]

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


> THIS is very interesting alright...  and it is *extremely* important, so
> let's triple verify it, shall we?
> 
> Please hack the driver to try to transmit data with that bit set to high, and
> also with that bit set to low.  The best way to go about it is probably to
> disable the rfkill support (so that it doesn't get in the way), hack the
> check-the-bit function to just printk its state, and see if the radio can
> effectively transmit and communicate no matter what state the input pin is
> at.
> 
> This is very important.  It will caracterize that input pin as either an
> hardware rfkill line, or as an input device (in which case I would be wrong
> when I asked to remove the input support from b43, but one step at a time).

Sorry, no, I think I got confused, it should be a hardware rfkill line.

johannes

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

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Henrique de Moraes Holschuh]

On Tue, 01 Jul 2008, Johannes Berg wrote:
> > THIS is very interesting alright...  and it is *extremely* important, so
> > let's triple verify it, shall we?
> > 
> > Please hack the driver to try to transmit data with that bit set to high, and
> > also with that bit set to low.  The best way to go about it is probably to
> > disable the rfkill support (so that it doesn't get in the way), hack the
> > check-the-bit function to just printk its state, and see if the radio can
> > effectively transmit and communicate no matter what state the input pin is
> > at.
> > 
> > This is very important.  It will caracterize that input pin as either an
> > hardware rfkill line, or as an input device (in which case I would be wrong
> > when I asked to remove the input support from b43, but one step at a time).
> 
> Sorry, no, I think I got confused, it should be a hardware rfkill line.

I'd like to have a certain response for this.  Can you test, and assert
whether it is a rfkill line or not?

And, if it IS a rfkill line, I still need to know if it is flip-flop-like or
not...  I am not certain I got the right idea (I understood it was NOT
flip-flop-like).

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Johannes Berg]

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

On Tue, 2008-07-01 at 15:41 -0300, Henrique de Moraes Holschuh wrote:
> On Tue, 01 Jul 2008, Johannes Berg wrote:
> > > THIS is very interesting alright...  and it is *extremely* important, so
> > > let's triple verify it, shall we?
> > > 
> > > Please hack the driver to try to transmit data with that bit set to high, and
> > > also with that bit set to low.  The best way to go about it is probably to
> > > disable the rfkill support (so that it doesn't get in the way), hack the
> > > check-the-bit function to just printk its state, and see if the radio can
> > > effectively transmit and communicate no matter what state the input pin is
> > > at.
> > > 
> > > This is very important.  It will caracterize that input pin as either an
> > > hardware rfkill line, or as an input device (in which case I would be wrong
> > > when I asked to remove the input support from b43, but one step at a time).
> > 
> > Sorry, no, I think I got confused, it should be a hardware rfkill line.
> 
> I'd like to have a certain response for this.  Can you test, and assert
> whether it is a rfkill line or not?
> 
> And, if it IS a rfkill line, I still need to know if it is flip-flop-like or
> not...  I am not certain I got the right idea (I understood it was NOT
> flip-flop-like).

No, it's not flip-flop, there's definitely an on/off bit that always
matches the button/hardware state.

johannes

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

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Henrique de Moraes Holschuh]

Dmitry (input layer maintainer) added to CC.  This is a [gross misuse of
the] input layer issue.

On Tue, 01 Jul 2008, Johannes Berg wrote:
> On Tue, 2008-07-01 at 15:41 -0300, Henrique de Moraes Holschuh wrote:
> > On Tue, 01 Jul 2008, Johannes Berg wrote:
> > > > This is very important.  It will caracterize that input pin as either an
> > > > hardware rfkill line, or as an input device (in which case I would be wrong
> > > > when I asked to remove the input support from b43, but one step at a time).
> > > 
> > > Sorry, no, I think I got confused, it should be a hardware rfkill line.
> > 
> > I'd like to have a certain response for this.  Can you test, and assert
> > whether it is a rfkill line or not?
> > 
> > And, if it IS a rfkill line, I still need to know if it is flip-flop-like or
> > not...  I am not certain I got the right idea (I understood it was NOT
> > flip-flop-like).
> 
> No, it's not flip-flop, there's definitely an on/off bit that always
> matches the button/hardware state.

Ok, so based on the above information, here is the sad truth:  B43 has been
buggy all along, and in more ways than I would have expected.

1. That input pin is a hardware rfkill line.  It has to contribute to the
device-wide rfkill class state.  The rfkill API, before my patchset, simply
did NOT support such a thing (but some where in the incorrect impression
that it could, through the input layer.  That's not true).

2. hardware rfkill lines are NOT flip-flop, so if it is ever connected to an
input device, it has to work like a switch.   It has to tell the world
whether it is active or not active.  It doesn't go "pressed/unpressed" to
switch from active to inactive, then "pressed/unpressed" to switch to
inactive back to active (THAT would be a KEY_*).

So, b43 should NEVER have issued KEY_WLAN in the first place.  It should
have asked for a EV_SW SW_WLAN event, added EV_SW SW_WLAN handling to
rfkill-input (for completeness), and used that one instead of KEY_WLAN.

And this has *nothing* to do with rfkill at all.  It is pure input layer
stuff.

For those who are not used to the input layer, the rule is very simple (as
it was explained to me): Input events come from anywhere and evertwhere, and
are all equal.  You are not, and I do mean NOT allowed to have KEY_WLAN
coming from b43 be any more special to the system than KEY_WLAN coming from
a USB keyboard that was plugged into the system.

The entire system fights against you if you try to go against this rule.
Userspace doesn't, as a rule, expect that EV_KEY KEY_FOO from one input
device is to be treated any differently than EV_KEY KEY_FOO from some other
input device.  rfkill-input and other in-kernel input handlers don't,
either.

b43 cannot even add an input handle to trap and update its "emulation of a
switch through KEY_*" state, as it has no way of knowing WHAT userspace or
rfkill-input is going to do with KEY_WLAN in the first place.  What if it is
not a toggle, but rather it is being diverted to launch an application that
asks the user?  What if it is a multi-value selector, that changes state at
every KEY_WLAN event, and has more than two states?

THIS is the reason why the input layer must not EVER be used as an IPC to
propagate state changes, BTW.  Handlers *are* allowed to just ignore events,
for one.

So, unless that input pin in b43 is really a flip-flop and point (1) above
is wrong (thus making most of this email not apply to b43), I see no way to
salvage this mess, at all.

IMO, this all means that the input layer support in b43 has to go.  It is
not the best place to add a EV_SW SW_WLAN-issuing input device anyway, and
userspace will already need to be reconfigured to deal with the fact that
b43 simply cannot issue KEY_WLAN, so it is not even worth the effort to add
a temporary hack to issue such events.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Johannes Berg]

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

On Tue, 2008-07-01 at 19:52 -0300, Henrique de Moraes Holschuh wrote:
> Dmitry (input layer maintainer) added to CC.  This is a [gross misuse of
> the] input layer issue.

FWIW, to me half of that mail reads like a rant and I'm ignoring it. If
you know so much better, why don't you continue working on fixing it?

To address one point though: userspace does want a notification when the
hw-rfkill is activated so it (e.g. network manager) can ask people to
un-rfkill their device.

johannes

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

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Henrique de Moraes Holschuh]

On Wed, 02 Jul 2008, Johannes Berg wrote:
> On Tue, 2008-07-01 at 19:52 -0300, Henrique de Moraes Holschuh wrote:
> > Dmitry (input layer maintainer) added to CC.  This is a [gross misuse of
> > the] input layer issue.
> 
> FWIW, to me half of that mail reads like a rant and I'm ignoring it. If
> you know so much better, why don't you continue working on fixing it?

I apologise.  Re-reading it, it does look like a rant, indeed.  It wasn't
supposed to be, but the tone came up nasty and quite defensive.  I didn't
intend it to.

The sheer ammount of explanations I have had to type over the whole rfkill
business is, apparently, getting to me a lot more than expected, and I guess
that put me in the defensive.  There is no excuse for that.

So, please accept my appologies on the tone of the last email.  Please
reconsider reading that email in full, and please try to ignore the rantness
smell in it.  I did try to explain the technical reasons why some changes
would be needed in b43.

I am sorry I didn't send you any patches.  I don't have any access to a b43
device, and as a rule, I try to avoid modifying code I will not be able to
test, especially when I don't know the code to begin with and the changes
are not going to be small, obvious ones.

> To address one point though: userspace does want a notification when the
> hw-rfkill is activated so it (e.g. network manager) can ask people to
> un-rfkill their device.

I tried to add such notifications directly to the rfkill class.  rfkill now
issues uevents and calls a notification chain for any state changes.  It has
been modified to differentiate hw-rfkill from sw-rfkill, and export that
information to userspace for network-manager and other applications to use.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Larry Finger]

Henrique de Moraes Holschuh wrote:
> On Wed, 02 Jul 2008, Johannes Berg wrote:
> I apologise.  Re-reading it, it does look like a rant, indeed.  It wasn't
> supposed to be, but the tone came up nasty and quite defensive.  I didn't
> intend it to.
> 
> The sheer ammount of explanations I have had to type over the whole rfkill
> business is, apparently, getting to me a lot more than expected, and I guess
> that put me in the defensive.  There is no excuse for that.
> 
> So, please accept my appologies on the tone of the last email.  Please
> reconsider reading that email in full, and please try to ignore the rantness
> smell in it.  I did try to explain the technical reasons why some changes
> would be needed in b43.
> 
> I am sorry I didn't send you any patches.  I don't have any access to a b43
> device, and as a rule, I try to avoid modifying code I will not be able to
> test, especially when I don't know the code to begin with and the changes
> are not going to be small, obvious ones.
> 
>> To address one point though: userspace does want a notification when the
>> hw-rfkill is activated so it (e.g. network manager) can ask people to
>> un-rfkill their device.
> 
> I tried to add such notifications directly to the rfkill class.  rfkill now
> issues uevents and calls a notification chain for any state changes.  It has
> been modified to differentiate hw-rfkill from sw-rfkill, and export that
> information to userspace for network-manager and other applications to use.

I am more than happy to test any b43 patches that you generate. My b43 
device has a wireless switch.

Larry

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Johannes Berg]

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


> So, please accept my appologies on the tone of the last email.  Please
> reconsider reading that email in full, and please try to ignore the rantness
> smell in it.  I did try to explain the technical reasons why some changes
> would be needed in b43.

Ok. I actually did read it but decided not to reply to most things. I
don't think it's that everybody needs to be educated on the input layer
(I've written real input drivers before) but rather that nobody really
knows how userspace/kernel interaction wrt. rfkill should work.

> > To address one point though: userspace does want a notification when the
> > hw-rfkill is activated so it (e.g. network manager) can ask people to
> > un-rfkill their device.
> 
> I tried to add such notifications directly to the rfkill class.  rfkill now
> issues uevents and calls a notification chain for any state changes.  It has
> been modified to differentiate hw-rfkill from sw-rfkill, and export that
> information to userspace for network-manager and other applications to use.

Sounds good.

johannes

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

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Johannes Berg]

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


> (I've written real input drivers before) but rather that nobody really
> knows how userspace/kernel interaction wrt. rfkill should work.

Oh and there's no need to explain on the mailing list, just make sure
the new rfkill docs are clear wrt. the way forward.

Another thing that just occurred to me: it might be nice to have a few
(per-hardware, global?) LED triggers that can be bound to LEDs and that
always reflect the state the driver says the radio has (hw-rfkill) or
the driver is asked to enforce (sw-rfkill)

johannes

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

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Henrique de Moraes Holschuh]

On Wed, 02 Jul 2008, Johannes Berg wrote:
> > (I've written real input drivers before) but rather that nobody really
> > knows how userspace/kernel interaction wrt. rfkill should work.
> 
> Oh and there's no need to explain on the mailing list, just make sure
> the new rfkill docs are clear wrt. the way forward.

Ok.  Is what we currently have (i.e. what is in -next/wireless-testing)
already enough?  Are there any topics I should try to clarify better on
Documentation/rfkill.txt ?

> Another thing that just occurred to me: it might be nice to have a few
> (per-hardware, global?) LED triggers that can be bound to LEDs and that
> always reflect the state the driver says the radio has (hw-rfkill) or
> the driver is asked to enforce (sw-rfkill)

Well, it should be doable with the new rfkill notify chain.  The chain is
called on every rfkill state change, and it will also get called when global
state changes (I am working on exposing the global state to userspace and
the rest of the kernel, now.  I don't exactly know the best way to go about
it, expect an RFC post soon about it).

So, one could write a separate module (e.g. rfkill-led) that registers with
the notify chain, and feeds the events to as many led triggers as he wants
to.  That would add LED trigger support to every rfkill driver in one go.

You could have different triggers for global states (there is one global
state per switch type), as well as triggers for each individual kill switch,
the notify chain exports enough data (a pointer to the rfkill structure) for
that (and if it doesn't, we ought to fix it so that it will :-) ).

Would "rfkill switch registered" and "rfkill switch unregistered"
notifications on the rfkill notify chain help write something like a led
trigger driver?  If so, I will add them.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH/RFC] b43: remove input device usage for rfkill

[Dmitry Torokhov]

On Wed, Jul 02, 2008 at 09:21:10AM +0200, Johannes Berg wrote:
> 
> > So, please accept my appologies on the tone of the last email.  Please
> > reconsider reading that email in full, and please try to ignore the rantness
> > smell in it.  I did try to explain the technical reasons why some changes
> > would be needed in b43.
> 
> Ok. I actually did read it but decided not to reply to most things. I
> don't think it's that everybody needs to be educated on the input layer
> (I've written real input drivers before) but rather that nobody really
> knows how userspace/kernel interaction wrt. rfkill should work.
> 

I guess I just need to re-iterate that using input layer to propagate
state change was not the best idea, because at any moment userspace
can issue EVIOCGRAB on the event device preventing anyone from getting
such event. I think I mentioned this to Michael once before.

-- 
Dmitry