[RFC] b43: rework rfkill code

[RFC] b43: rework rfkill code

[Matthew Garrett]

I've reworked the rfkill code in b43. This ought to be more consistent 
with the other drivers and it seems to work on the machines I've tested 
it on here, but it'd be good to get some feedback.

Firstly, I've replaced the polled input device. It's just some delayed 
work now. It polls the hardware every second to determine whether the 
radio has been hardware killed or not. If it has, it sets the rfkill 
state to HARD_BLOCKED. If the radio is enabled and the previous state 
was HARD_BLOCKED, it resets the state to UNBLOCKED. If the radio is 
enabled and the previous state was SOFT_BLOCKED, it leaves the state as 
is.

I also removed some of the complexity from the rfkill toggle function, 
since the rfkill core will handle the case of the user requesting a 
change from HARD_BLOCKED without the driver needing to care.

The final change is that I removed the code for changing the wireless 
state in response to the txpower configuration in mac80211. Right now, I 
can't see any way for this to work correctly - if the user disables the 
radio via rfkill, mac80211 doesn't flag the radio as disabled. As a 
result, the next time the configuration callback is called, b43 
reenables the radio again, even though the user has explicitly disabled 
it. I don't think any of the other drivers handle this case, so I'm not 
really sure what the best way to handle this in future is. The current 
situation certainly seems broken.

How does this look to people?

diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index c34c589..9231eea 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -3384,21 +3384,6 @@ static int b43_op_config(struct ieee80211_hw *hw, u32 changed)
 	    b43_is_mode(wl, NL80211_IFTYPE_MESH_POINT))
 		b43_set_beacon_int(dev, conf->beacon_int);
 
-	if (!!conf->radio_enabled != phy->radio_on) {
-		if (conf->radio_enabled) {
-			b43_software_rfkill(dev, RFKILL_STATE_UNBLOCKED);
-			b43info(dev->wl, "Radio turned on by software\n");
-			if (!dev->radio_hw_enable) {
-				b43info(dev->wl, "The hardware RF-kill button "
-					"still turns the radio physically off. "
-					"Press the button to turn it on.\n");
-			}
-		} else {
-			b43_software_rfkill(dev, RFKILL_STATE_SOFT_BLOCKED);
-			b43info(dev->wl, "Radio turned off by software\n");
-		}
-	}
-
 	spin_lock_irqsave(&wl->irq_lock, flags);
 	b43_interrupt_enable(dev, savedirqs);
 	mmiowb();
diff --git a/drivers/net/wireless/b43/rfkill.c b/drivers/net/wireless/b43/rfkill.c
index 7137537..4c2907f 100644
--- a/drivers/net/wireless/b43/rfkill.c
+++ b/drivers/net/wireless/b43/rfkill.c
@@ -28,7 +28,6 @@
 
 #include <linux/kmod.h>
 
-
 /* Returns TRUE, if the radio is enabled in hardware. */
 static bool b43_is_hw_radio_enabled(struct b43_wldev *dev)
 {
@@ -45,33 +44,43 @@ static bool b43_is_hw_radio_enabled(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(struct work_struct *work)
 {
-	struct b43_wldev *dev = poll_dev->private;
+	struct b43_rfkill *rfk = container_of(work, struct b43_rfkill,
+					      work.work);
+	struct b43_wldev *dev = rfk->rfkill->data;
 	struct b43_wl *wl = dev->wl;
 	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_unlock;
+
 	enabled = b43_is_hw_radio_enabled(dev);
 	if (unlikely(enabled != dev->radio_hw_enable)) {
+		/*
+		 *  If the hardware is enabled and the state isn't
+		 *  hard blocked then we're soft blocked and shouldn't
+		 *  change the state
+		 */
+		if (enabled && rfk->rfkill->state != RFKILL_STATE_HARD_BLOCKED)
+			goto out_unlock;
+
 		dev->radio_hw_enable = enabled;
-		report_change = 1;
+
+		if (enabled)
+			rfkill_force_state(rfk->rfkill, RFKILL_STATE_UNBLOCKED);
+		else
+			rfkill_force_state(rfk->rfkill,
+					   RFKILL_STATE_HARD_BLOCKED);
+
 		b43info(wl, "Radio hardware status changed to %s\n",
 			enabled ? "ENABLED" : "DISABLED");
 	}
+out_unlock:
 	mutex_unlock(&wl->mutex);
-
-	/* 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);
-	}
+	schedule_delayed_work(&rfk->work, msecs_to_jiffies(1000));
 }
 
 /* Called when the RFKILL toggled in software. */
@@ -87,26 +96,9 @@ static int b43_rfkill_soft_toggle(void *data, enum rfkill_state state)
 	mutex_lock(&wl->mutex);
 	if (b43_status(dev) < B43_STAT_INITIALIZED)
 		goto out_unlock;
+
 	err = 0;
-	switch (state) {
-	case RFKILL_STATE_UNBLOCKED:
-		if (!dev->radio_hw_enable) {
-			/* No luck. We can't toggle the hardware RF-kill
-			 * button from software. */
-			err = -EBUSY;
-			goto out_unlock;
-		}
-		if (!dev->phy.radio_on)
-			b43_software_rfkill(dev, state);
-		break;
-	case RFKILL_STATE_SOFT_BLOCKED:
-		if (dev->phy.radio_on)
-			b43_software_rfkill(dev, state);
-		break;
-	default:
-		b43warn(wl, "Received unexpected rfkill state %d.\n", state);
-		break;
-	}
+	b43_software_rfkill(dev, state);
 out_unlock:
 	mutex_unlock(&wl->mutex);
 
@@ -141,52 +133,17 @@ 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);
-
 	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 */
-
-#if !defined(CONFIG_RFKILL_INPUT) && !defined(CONFIG_RFKILL_INPUT_MODULE)
-	b43warn(wl, "The rfkill-input subsystem is not available. "
-		"The built-in radio LED will not work.\n");
-#endif
-
-	err = input_register_polled_device(rfk->poll_dev);
-	if (err)
-		goto err_unreg_rfk;
+		goto err_freed_rfk;
 
 	rfk->registered = 1;
 
+	INIT_DELAYED_WORK(&rfk->work, b43_rfkill_poll);
+	schedule_delayed_work(&rfk->work, msecs_to_jiffies(1000));
+
 	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:
@@ -202,9 +159,8 @@ void b43_rfkill_exit(struct b43_wldev *dev)
 		return;
 	rfk->registered = 0;
 
-	input_unregister_polled_device(rfk->poll_dev);
+	cancel_delayed_work_sync(&rfk->work);
+
 	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..4efdb4a 100644
--- a/drivers/net/wireless/b43/rfkill.h
+++ b/drivers/net/wireless/b43/rfkill.h
@@ -13,8 +13,8 @@ struct b43_wldev;
 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;
+	/* The work queue for the RFKILL input button */
+	struct delayed_work work;
 	/* Did initialization succeed? Used for freeing. */
 	bool registered;
 	/* The unique name of this rfkill switch */


-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC] b43: rework rfkill code

[Johannes Berg]

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

On Wed, 2008-12-10 at 15:09 +0000, Matthew Garrett wrote:

> The final change is that I removed the code for changing the wireless 
> state in response to the txpower configuration in mac80211. Right now, I 
> can't see any way for this to work correctly - if the user disables the 
> radio via rfkill, mac80211 doesn't flag the radio as disabled. As a 
> result, the next time the configuration callback is called, b43 
> reenables the radio again, even though the user has explicitly disabled 
> it. I don't think any of the other drivers handle this case, so I'm not 
> really sure what the best way to handle this in future is. The current 
> situation certainly seems broken.

We're going to have to integrate rfkill with mac80211, but nobody cares.

johannes

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

Re: [RFC] b43: rework rfkill code

[Michael Buesch]

On Wednesday 10 December 2008 16:09:35 Matthew Garrett wrote:
> I've reworked the rfkill code in b43. This ought to be more consistent 
...

I'm fine with this, as long as you take over the responsibility for the whole b43-rfkill code.
I'm not going to touch it anymore. I'm only going to fix it by either forwarding bugreports
to somebody else (you, in that case) or by reverting patches until it starts working again.

-- 
Greetings, Michael.

Re: [RFC] b43: rework rfkill code

[Matthew Garrett]

On Wed, Dec 10, 2008 at 04:48:29PM +0100, Michael Buesch wrote:
> On Wednesday 10 December 2008 16:09:35 Matthew Garrett wrote:
> > I've reworked the rfkill code in b43. This ought to be more consistent 
> ...
> 
> I'm fine with this, as long as you take over the responsibility for the whole b43-rfkill code.
> I'm not going to touch it anymore. I'm only going to fix it by either forwarding bugreports
> to somebody else (you, in that case) or by reverting patches until it starts working again.

Works for me, though I'd appreciate it if people could give it a quick 
sanity test. The main thing I'm worried about is LED control, which I 
think /ought/ to still work but is dependent on everything in the rfkill 
core working properly.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC] b43: rework rfkill code

[Ivo van Doorn]

On Wednesday 10 December 2008, Johannes Berg wrote:
> On Wed, 2008-12-10 at 15:09 +0000, Matthew Garrett wrote:
> 
> > The final change is that I removed the code for changing the wireless 
> > state in response to the txpower configuration in mac80211. Right now, I 
> > can't see any way for this to work correctly - if the user disables the 
> > radio via rfkill, mac80211 doesn't flag the radio as disabled. As a 
> > result, the next time the configuration callback is called, b43 
> > reenables the radio again, even though the user has explicitly disabled 
> > it. I don't think any of the other drivers handle this case, so I'm not 
> > really sure what the best way to handle this in future is. The current 
> > situation certainly seems broken.
> 
> We're going to have to integrate rfkill with mac80211, but nobody cares.

It's on the rt2x00 todo list since quite some time already in the hope to
find a volunteer who was willing to do it.
I can't promise any patches for at least a month in this area, but I'll see if
I can take a shot at this in a couple of weeks time.

Ivo

Re: [RFC] b43: rework rfkill code

[Marcel Holtmann]

Hi Johannes,

> > The final change is that I removed the code for changing the wireless 
> > state in response to the txpower configuration in mac80211. Right now, I 
> > can't see any way for this to work correctly - if the user disables the 
> > radio via rfkill, mac80211 doesn't flag the radio as disabled. As a 
> > result, the next time the configuration callback is called, b43 
> > reenables the radio again, even though the user has explicitly disabled 
> > it. I don't think any of the other drivers handle this case, so I'm not 
> > really sure what the best way to handle this in future is. The current 
> > situation certainly seems broken.
> 
> We're going to have to integrate rfkill with mac80211, but nobody cares.

if you figured out on how to do it the best way, then let me know,
because I have to do the same thing for Bluetooth.

Regards

Marcel

Re: [RFC] b43: rework rfkill code

[Johannes Berg]

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

[warning: this is going to be a long mail and will tell you why and how
I think rfkill needs to be rewritten]

On Wed, 2008-12-10 at 17:51 +0100, Marcel Holtmann wrote:

> if you figured out on how to do it the best way, then let me know,
> because I have to do the same thing for Bluetooth.

I'd suggest to start by rewriting rfkill to be not such a mess to use.

We really need to disentangle the state tracking rfkill does. People
have said this a million times before, but nobody cares: drivers need to
register hard-blocked and get soft-block states independently, not in a
single enum; API for drivers needs to be, above all, EASY to use.

For mac80211, we really need to register an rfkill structure for each
physical device that mac80211 knows about. Drivers would not be allowed
to get access to this structure, because the toggle_radio() callback is
assigned to a mac80211 function that calls the driver's ->config()
callback and, at some point, also disallows configuring interfaces that
belong to a device that is rfkilled.

To properly implement this, however, we need to disentangle rfkill
states and provide just a single API function to change the state:

rfkill_hw_kill(struct rfkill *rfkill, bool killed);

this would be shadowed by mac80211 with
	ieee80211_hw_rfkill(struct ieee80211_hw *hw, bool killed)
so mac80211 knows about hard-kills too, since they don't call
->toggle_radio() obviously.

I know what you're thinking at this point, but hear me out. This is
another thing where I think rfkill's design is fundamentally wrong. If
you're not thinking anything, hear me out anyway :)


Let's take a step back here and analyse what we really have and need:

 * we have devices with a plethora of rfkill setups:
   1) devices disappear from the bus (not handled here, not possible
      either)
   2) devices have hard kill button that just disables the radio
      (need to know about this so users aren't left in the dark)
   3) devices have soft kill buttons that just notify the driver
   4) soft kill switches that work through other input methods (ACPI
      comes to mind, but anything is possible)
 * we have various ways to kill the radio:
   1) like above
   2) through platform methods (toshiba ACPI I think?)
   3) through the driver only

The last two cases only differ in the radios that the button is supposed
to apply to, but this is more of a policy decision.

The point where I think rfkill's design is totally wrong is that it
intimately ties this software kill from case three to the rfkill
structure as well.

Instead, we should come to accept that this is just a button as well. A
button, however, that is *by default* tied to a particular radio.
Therefore, drivers for devices that provide such buttons, should, in my
opinion, register an rfkill button driver that
 a) by default is tied to their hardware by a pointer to the rfkill
    struct for the hardware, but this could be possible to override in
    sysfs like LEDs are, if you want the button to really kill all
    radios
 b) serves as input to the rfkill subsystem which will then look up the
    right rfkill struct(s) and ask them to kill the radio

This would add new API for a button, but again only a single function,
something like
	rfkill_button_update(struct rfkill_button *btn, bool killed)


Finally, rfkill-input will boil down to just registering a software
button that happens to control all radios by default, by setting the
controlled rfkill struct to NULL.


johannes

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

Re: [RFC] b43: rework rfkill code

[Johannes Berg]

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

On Wed, 2008-12-10 at 18:18 +0100, Johannes Berg wrote:

> We really need to disentangle the state tracking rfkill does. People
> have said this a million times before, but nobody cares: drivers need to
> register hard-blocked and get soft-block states independently, not in a
> single enum; API for drivers needs to be, above all, EASY to use.

Also, the rfkill struct itself is a mess. What's get_state() for? Why is
this not layered? How can get_state() work correctly, it doesn't poll
the device so it doesn't look like software will ever get a state
update.

Then there's user_claim_unsupported which is set by all drivers but
rt2x00, probably because they have hardware kill switches and thus they
have to set it even if it's not strictly true, because of the lacking
separation between these things (that I pointed out)

johannes

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

Re: [RFC] b43: rework rfkill code

[Johannes Berg]

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

On Wed, 2008-12-10 at 18:23 +0100, Johannes Berg wrote:

> Then there's user_claim_unsupported which is set by all drivers but
> rt2x00, probably because they have hardware kill switches and thus they
> have to set it even if it's not strictly true, because of the lacking
> separation between these things (that I pointed out)

IOW, correct me if I'm wrong, it seems to me that user_claim_unsupported
really is a wrong name for "has hw kill", which could be avoided if sw
and hw kill were a different thing and the rfkill structure was only
used, as I'm proposing, for hw kill and sw kill _notifications_, but not
the sw kill operation itself.

johannes

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

Re: [RFC] b43: rework rfkill code

[Michael Buesch]

On Wednesday 10 December 2008 18:23:40 Johannes Berg wrote:
> Then there's user_claim_unsupported which is set by all drivers but
> rt2x00, probably because they have hardware kill switches and thus they
> have to set it even if it's not strictly true, because of the lacking
> separation between these things (that I pointed out)

I introduced it when I ported b43 to rfkill.
Well, a lot of semantical changes were made _after_ that.
When I added it there only were two rfkill states and b43 handled these wrt the
actual hardware state (and I still think that's the right thing to do. The sw-state intermix
is confusing).
So when I added the flag it meant:
user_claim_unsupported = True means user cannot change the hardware kill state.
So basically it means the device has two states. One software state and one hardware
state.
However, I don't know what the semantics for the flag are today. Lots of code changed.

-- 
Greetings, Michael.

Re: [RFC] b43: rework rfkill code

[Johannes Berg]

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

On Wed, 2008-12-10 at 18:31 +0100, Michael Buesch wrote:

> I introduced it when I ported b43 to rfkill.
> Well, a lot of semantical changes were made _after_ that.
> When I added it there only were two rfkill states and b43 handled these wrt the
> actual hardware state (and I still think that's the right thing to do. The sw-state intermix
> is confusing).

Right.

> So when I added the flag it meant:
> user_claim_unsupported = True means user cannot change the hardware kill state.

Yeah, but the assumption that software can change the "hardware kill
state" is rather stupid to start with, I think.

> So basically it means the device has two states. One software state and one hardware
> state.
> However, I don't know what the semantics for the flag are today. Lots of code changed.

Ok. I think the fundamental flaw here is assuming that there's just a
single state. There isn't. The device can be turned off in hardware (in
which case sw won't be able do anything about it, but we want to know)
or in software (which we want to handle). Pretending that there's just a
single state that's either hw-off, sw-off or on is plain wrong. The
device can be hw-off and sw-off at the same time, and then if you turn
off the hw-off button it won't turn on (however, unless your system
integrator totally screwed up, you won't have a hw and a sw button on
your system)

johannes

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

Re: [RFC] b43: rework rfkill code

[Matthew Garrett]

On Wed, Dec 10, 2008 at 06:37:23PM +0100, Johannes Berg wrote:

> Ok. I think the fundamental flaw here is assuming that there's just a
> single state. There isn't. The device can be turned off in hardware (in
> which case sw won't be able do anything about it, but we want to know)
> or in software (which we want to handle). Pretending that there's just a
> single state that's either hw-off, sw-off or on is plain wrong. The
> device can be hw-off and sw-off at the same time, and then if you turn
> off the hw-off button it won't turn on (however, unless your system
> integrator totally screwed up, you won't have a hw and a sw button on
> your system)

They may not be physical buttons, but we can often control this anyway. 
For instance, my HP has a button that will perform a hardware disable of 
the wifi card. However, I can control that button's state through 
software with the hp-wmi driver. The way we currently handle that (and, 
I think, the only way we *can* handle that) is to provide two separate 
rfkill interfaces - one tied to the wireless device, one tied to the 
platform device.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC] b43: rework rfkill code

[Michael Buesch]

On Wednesday 10 December 2008 18:51:02 Matthew Garrett wrote:
> On Wed, Dec 10, 2008 at 06:37:23PM +0100, Johannes Berg wrote:
> 
> > Ok. I think the fundamental flaw here is assuming that there's just a
> > single state. There isn't. The device can be turned off in hardware (in
> > which case sw won't be able do anything about it, but we want to know)
> > or in software (which we want to handle). Pretending that there's just a
> > single state that's either hw-off, sw-off or on is plain wrong. The
> > device can be hw-off and sw-off at the same time, and then if you turn
> > off the hw-off button it won't turn on (however, unless your system
> > integrator totally screwed up, you won't have a hw and a sw button on
> > your system)
> 
> They may not be physical buttons, but we can often control this anyway. 

But we do not _want_ it.
If you can do it, keep it private to the driver. Do not export it to other layers.
If you need to to sw-rfkill through it, do it in the driver and multiplex
the hw-sw-states in the driver.

-- 
Greetings, Michael.

Re: [RFC] b43: rework rfkill code

[Johannes Berg]

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

On Wed, 2008-12-10 at 17:51 +0000, Matthew Garrett wrote:

> They may not be physical buttons, but we can often control this anyway. 
> For instance, my HP has a button that will perform a hardware disable of 
> the wifi card. However, I can control that button's state through 
> software with the hp-wmi driver. 

That's indeed a complication I wasn't aware of.

> The way we currently handle that (and, 
> I think, the only way we *can* handle that) is to provide two separate 
> rfkill interfaces - one tied to the wireless device, one tied to the 
> platform device.

Yes, but how do we currently do this?

Does the wireless driver get the notification about this from the
hardware, like it would if this was a real physical switch? Then it's
probably pretty simple: provide a rfkill struct from the driver that
updates hard-kill and provide a second rfkill struct for the platform
device that doesn't get hard-killed, but also provide a soft-kill input
form the platform device. That way, you can toggle that button, but you
can also software-enable the platform rfkill device and that in turn
re-enables the wifi-rfkill "hw" switch device.

If we need to tie them together in software it gets more complicated
though.

johannes

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

Re: [RFC] b43: rework rfkill code

[Matthew Garrett]

On Wed, Dec 10, 2008 at 07:05:43PM +0100, Johannes Berg wrote:

> Does the wireless driver get the notification about this from the
> hardware, like it would if this was a real physical switch?

Yes.

> Then it's probably pretty simple: provide a rfkill struct from the 
> driver that updates hard-kill and provide a second rfkill struct for 
> the platform device that doesn't get hard-killed, but also provide a 
> soft-kill input form the platform device. That way, you can toggle 
> that button, but you can also software-enable the platform rfkill 
> device and that in turn re-enables the wifi-rfkill "hw" switch device.

Right. That's prety close to the current situation.
 
> If we need to tie them together in software it gets more complicated
> though.

I think we can avoid that, thankfully.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC] b43: rework rfkill code

[Dan Williams]

On Wed, 2008-12-10 at 19:05 +0100, Johannes Berg wrote:
> On Wed, 2008-12-10 at 17:51 +0000, Matthew Garrett wrote:
> 
> > They may not be physical buttons, but we can often control this anyway. 
> > For instance, my HP has a button that will perform a hardware disable of 
> > the wifi card. However, I can control that button's state through 
> > software with the hp-wmi driver. 
> 
> That's indeed a complication I wasn't aware of.
> 
> > The way we currently handle that (and, 
> > I think, the only way we *can* handle that) is to provide two separate 
> > rfkill interfaces - one tied to the wireless device, one tied to the 
> > platform device.
> 
> Yes, but how do we currently do this?
> 
> Does the wireless driver get the notification about this from the
> hardware, like it would if this was a real physical switch? Then it's
> probably pretty simple: provide a rfkill struct from the driver that
> updates hard-kill and provide a second rfkill struct for the platform
> device that doesn't get hard-killed, but also provide a soft-kill input
> form the platform device. That way, you can toggle that button, but you
> can also software-enable the platform rfkill device and that in turn
> re-enables the wifi-rfkill "hw" switch device.

This sort of sucks for userspace, because we see the actual wifi card as
hardblocked, but some other random button as softblocked.  There's no
indication that changing the softblock one will affect the hardblocked
one.  What are userspace processes supposed to do here, assume that if a
non-radio-associated softblocked switch exists, that it can re-enable a
hardblocked radio of some random wifi card?

Dan

Re: [RFC] b43: rework rfkill code

[Johannes Berg]

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

On Wed, 2008-12-10 at 13:29 -0500, Dan Williams wrote:

> > Does the wireless driver get the notification about this from the
> > hardware, like it would if this was a real physical switch? Then it's
> > probably pretty simple: provide a rfkill struct from the driver that
> > updates hard-kill and provide a second rfkill struct for the platform
> > device that doesn't get hard-killed, but also provide a soft-kill input
> > form the platform device. That way, you can toggle that button, but you
> > can also software-enable the platform rfkill device and that in turn
> > re-enables the wifi-rfkill "hw" switch device.
> 
> This sort of sucks for userspace, because we see the actual wifi card as
> hardblocked, but some other random button as softblocked.  There's no
> indication that changing the softblock one will affect the hardblocked
> one.  What are userspace processes supposed to do here, assume that if a
> non-radio-associated softblocked switch exists, that it can re-enable a
> hardblocked radio of some random wifi card?

The other question is whether we actually care? So what if the hardware
can only be enabled with the button, why does that matter?

johannes

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

Re: [RFC] b43: rework rfkill code

[Dan Williams]

On Wed, 2008-12-10 at 19:33 +0100, Johannes Berg wrote:
> On Wed, 2008-12-10 at 13:29 -0500, Dan Williams wrote:
> 
> > > Does the wireless driver get the notification about this from the
> > > hardware, like it would if this was a real physical switch? Then it's
> > > probably pretty simple: provide a rfkill struct from the driver that
> > > updates hard-kill and provide a second rfkill struct for the platform
> > > device that doesn't get hard-killed, but also provide a soft-kill input
> > > form the platform device. That way, you can toggle that button, but you
> > > can also software-enable the platform rfkill device and that in turn
> > > re-enables the wifi-rfkill "hw" switch device.
> > 
> > This sort of sucks for userspace, because we see the actual wifi card as
> > hardblocked, but some other random button as softblocked.  There's no
> > indication that changing the softblock one will affect the hardblocked
> > one.  What are userspace processes supposed to do here, assume that if a
> > non-radio-associated softblocked switch exists, that it can re-enable a
> > hardblocked radio of some random wifi card?
> 
> The other question is whether we actually care? So what if the hardware
> can only be enabled with the button, why does that matter?

I guess it doesn't, as long as in Matthew's case, the actual radio
rfkill state is only ever softblocked, because it actually *can* be
re-enabled with the platform button or something.

Dan

Re: [RFC] b43: rework rfkill code

[Michael Buesch]

On Wednesday 10 December 2008 19:29:50 Dan Williams wrote:
> On Wed, 2008-12-10 at 19:05 +0100, Johannes Berg wrote:
> > On Wed, 2008-12-10 at 17:51 +0000, Matthew Garrett wrote:
> > 
> > > They may not be physical buttons, but we can often control this anyway. 
> > > For instance, my HP has a button that will perform a hardware disable of 
> > > the wifi card. However, I can control that button's state through 
> > > software with the hp-wmi driver. 
> > 
> > That's indeed a complication I wasn't aware of.
> > 
> > > The way we currently handle that (and, 
> > > I think, the only way we *can* handle that) is to provide two separate 
> > > rfkill interfaces - one tied to the wireless device, one tied to the 
> > > platform device.
> > 
> > Yes, but how do we currently do this?
> > 
> > Does the wireless driver get the notification about this from the
> > hardware, like it would if this was a real physical switch? Then it's
> > probably pretty simple: provide a rfkill struct from the driver that
> > updates hard-kill and provide a second rfkill struct for the platform
> > device that doesn't get hard-killed, but also provide a soft-kill input
> > form the platform device. That way, you can toggle that button, but you
> > can also software-enable the platform rfkill device and that in turn
> > re-enables the wifi-rfkill "hw" switch device.
> 
> This sort of sucks for userspace, because we see the actual wifi card as
> hardblocked, but some other random button as softblocked.  There's no
> indication that changing the softblock one will affect the hardblocked
> one.  What are userspace processes supposed to do here, assume that if a
> non-radio-associated softblocked switch exists, that it can re-enable a
> hardblocked radio of some random wifi card?

I don't see the problem.
If userspace wants to enable wifi, it should simply _try_ to do so:
Userspace sees hw-block and sw-block state:
- Unblock the sw state
- Re-fetch hw-block and sw-block state
- If either one is blocked, we can't enable the radio.
- Notify user.

-- 
Greetings, Michael.

Re: [RFC] b43: rework rfkill code

[Henrique de Moraes Holschuh]

On Wed, 10 Dec 2008, Johannes Berg wrote:
> On Wed, 2008-12-10 at 18:23 +0100, Johannes Berg wrote:
> > Then there's user_claim_unsupported which is set by all drivers but
> > rt2x00, probably because they have hardware kill switches and thus they
> > have to set it even if it's not strictly true, because of the lacking
> > separation between these things (that I pointed out)
> 
> IOW, correct me if I'm wrong, it seems to me that user_claim_unsupported
> really is a wrong name for "has hw kill", which could be avoided if sw

I never understood what user_claim_unsupported is for.  I left it alone
because of that, but it looks like some artifact of the old rfkill that did
horrible things to the input layer.

-- 
  "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: [RFC] b43: rework rfkill code

[Michael Buesch]

On Wednesday 10 December 2008 22:33:34 Henrique de Moraes Holschuh wrote:
> On Wed, 10 Dec 2008, Johannes Berg wrote:
> > On Wed, 2008-12-10 at 18:23 +0100, Johannes Berg wrote:
> > > Then there's user_claim_unsupported which is set by all drivers but
> > > rt2x00, probably because they have hardware kill switches and thus they
> > > have to set it even if it's not strictly true, because of the lacking
> > > separation between these things (that I pointed out)
> > 
> > IOW, correct me if I'm wrong, it seems to me that user_claim_unsupported
> > really is a wrong name for "has hw kill", which could be avoided if sw
> 
> I never understood what user_claim_unsupported is for.  I left it alone
> because of that, but it looks like some artifact of the old rfkill that did
> horrible things to the input layer.

No, as I just explained. It comes from a time when we didn't have all that input stuff at all.
It was a workaround. rfkill basically had a facility to change the hardware rfkill state from
userspace. As b43 does not support that, I introduced the flag.
Today we have three states (which is still broken, but you saw the rest of the thread...), so I guess
we can remove it again.
We cannot change the hardware state. That's what the flag is (was) for.

-- 
Greetings, Michael.

Re: [RFC] b43: rework rfkill code

[Julian Calaby]

On Thu, Dec 11, 2008 at 02:29, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2008-12-10 at 15:09 +0000, Matthew Garrett wrote:
>
>> The final change is that I removed the code for changing the wireless
>> state in response to the txpower configuration in mac80211. Right now, I
>> can't see any way for this to work correctly - if the user disables the
>> radio via rfkill, mac80211 doesn't flag the radio as disabled. As a
>> result, the next time the configuration callback is called, b43
>> reenables the radio again, even though the user has explicitly disabled
>> it. I don't think any of the other drivers handle this case, so I'm not
>> really sure what the best way to handle this in future is. The current
>> situation certainly seems broken.
>
> We're going to have to integrate rfkill with mac80211, but nobody cares.

What strikes me, watching this from the outside - is that rfkill and
power saving seem to be doing essentially the same thing: temporarily
powering down the radio / card.

If we're going to integrate rfkill and mac80211 or rewrite rfkill, why
not hook it into power saving too?

Just my $0.01

Thanks,

-- 

Julian Calaby

Email: julian.calaby@gmail.com

Re: [RFC] b43: rework rfkill code

[Michael Buesch]

On Thursday 11 December 2008 01:32:37 Julian Calaby wrote:
> On Thu, Dec 11, 2008 at 02:29, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Wed, 2008-12-10 at 15:09 +0000, Matthew Garrett wrote:
> >
> >> The final change is that I removed the code for changing the wireless
> >> state in response to the txpower configuration in mac80211. Right now, I
> >> can't see any way for this to work correctly - if the user disables the
> >> radio via rfkill, mac80211 doesn't flag the radio as disabled. As a
> >> result, the next time the configuration callback is called, b43
> >> reenables the radio again, even though the user has explicitly disabled
> >> it. I don't think any of the other drivers handle this case, so I'm not
> >> really sure what the best way to handle this in future is. The current
> >> situation certainly seems broken.
> >
> > We're going to have to integrate rfkill with mac80211, but nobody cares.
> 
> What strikes me, watching this from the outside - is that rfkill and
> power saving seem to be doing essentially the same thing: temporarily
> powering down the radio / card.

I think it's essentially a different thing.
rfkill means -> turn off the radio; no matter what.
PS means -> turn off the radio for whatever amount of microseconds and periodically
wake up to see what's up.
PS-core also takes place in the firmware of the device, where rfkill is a much higher layer thing.

-- 
Greetings, Michael.

Re: [RFC] b43: rework rfkill code

[Kalle Valo]

Michael Buesch <mb@bu3sch.de> writes:

> On Thursday 11 December 2008 01:32:37 Julian Calaby wrote:
>
>> What strikes me, watching this from the outside - is that rfkill and
>> power saving seem to be doing essentially the same thing: temporarily
>> powering down the radio / card.
>
> I think it's essentially a different thing. rfkill means -> turn off
> the radio; no matter what. PS means -> turn off the radio for
> whatever amount of microseconds and periodically wake up to see
> what's up. PS-core also takes place in the firmware of the device,
> where rfkill is a much higher layer thing.

I agree, they shouldn't be intermixed at all.

-- 
Kalle Valo

Re: [RFC] b43: rework rfkill code

[Larry Finger]

Matthew Garrett wrote:
> I've reworked the rfkill code in b43. This ought to be more consistent 
> with the other drivers and it seems to work on the machines I've tested 
> it on here, but it'd be good to get some feedback.
> 
> Firstly, I've replaced the polled input device. It's just some delayed 
> work now. It polls the hardware every second to determine whether the 
> radio has been hardware killed or not. If it has, it sets the rfkill 
> state to HARD_BLOCKED. If the radio is enabled and the previous state 
> was HARD_BLOCKED, it resets the state to UNBLOCKED. If the radio is 
> enabled and the previous state was SOFT_BLOCKED, it leaves the state as 
> is.
> 
> I also removed some of the complexity from the rfkill toggle function, 
> since the rfkill core will handle the case of the user requesting a 
> change from HARD_BLOCKED without the driver needing to care.
> 
> The final change is that I removed the code for changing the wireless 
> state in response to the txpower configuration in mac80211. Right now, I 
> can't see any way for this to work correctly - if the user disables the 
> radio via rfkill, mac80211 doesn't flag the radio as disabled. As a 
> result, the next time the configuration callback is called, b43 
> reenables the radio again, even though the user has explicitly disabled 
> it. I don't think any of the other drivers handle this case, so I'm not 
> really sure what the best way to handle this in future is. The current 
> situation certainly seems broken.
> 
> How does this look to people?

All this discussion about hard vs soft rfkill makes my head hurt and I have
stopped reading those posts.

With this patch, my b43 device and its LED still work.

Larry

Re: [RFC] b43: rework rfkill code

[Larry Finger]

Matthew Garrett wrote:
> I've reworked the rfkill code in b43. This ought to be more consistent 
> with the other drivers and it seems to work on the machines I've tested 
> it on here, but it'd be good to get some feedback.
> 
> Firstly, I've replaced the polled input device. It's just some delayed 
> work now. It polls the hardware every second to determine whether the 
> radio has been hardware killed or not. If it has, it sets the rfkill 
> state to HARD_BLOCKED. If the radio is enabled and the previous state 
> was HARD_BLOCKED, it resets the state to UNBLOCKED. If the radio is 
> enabled and the previous state was SOFT_BLOCKED, it leaves the state as 
> is.
> 
> I also removed some of the complexity from the rfkill toggle function, 
> since the rfkill core will handle the case of the user requesting a 
> change from HARD_BLOCKED without the driver needing to care.
> 
> The final change is that I removed the code for changing the wireless 
> state in response to the txpower configuration in mac80211. Right now, I 
> can't see any way for this to work correctly - if the user disables the 
> radio via rfkill, mac80211 doesn't flag the radio as disabled. As a 
> result, the next time the configuration callback is called, b43 
> reenables the radio again, even though the user has explicitly disabled 
> it. I don't think any of the other drivers handle this case, so I'm not 
> really sure what the best way to handle this in future is. The current 
> situation certainly seems broken.
> 
> How does this look to people?

All this discussion about hard vs soft rfkill makes my head hurt and I have
stopped reading those posts.

Correction to my earlier post. If the system is booted with the RF switch off,
the LED is on, whereas it should be off. The original code works correctly.

Larry

Re: [RFC] b43: rework rfkill code

[John W. Linville]

On Thu, Dec 11, 2008 at 10:28:19PM -0600, Larry Finger wrote:
> Matthew Garrett wrote:
> > I've reworked the rfkill code in b43. This ought to be more consistent 
> > with the other drivers and it seems to work on the machines I've tested 
> > it on here, but it'd be good to get some feedback.

<snip>

> > How does this look to people?
> 
> All this discussion about hard vs soft rfkill makes my head hurt and I have
> stopped reading those posts.
> 
> Correction to my earlier post. If the system is booted with the RF switch off,
> the LED is on, whereas it should be off. The original code works correctly.

Based on the above, I'm dropping this patch.  Please submit a
non-regressing version! :-)

John
-- 
John W. Linville		Linux should be at the core
linville@tuxdriver.com			of your literate lifestyle.

Re: [RFC] b43: rework rfkill code

[Johannes Berg]

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

Marcel,

> > We're going to have to integrate rfkill with mac80211, but nobody cares.
> 
> if you figured out on how to do it the best way, then let me know,
> because I have to do the same thing for Bluetooth.

I've now rewritten rfkill [beep] to [beep] -- the plan for mac80211 now
is to have _it_ register the rfkill struct, and let the driver only
provide the hard-block state (or a function to poll it where necessary),
and handle everything else via the regular config call in mac80211 with
radio_enabled. Then mac80211 can also react on unblock by reassociating
or whatever.

johannes

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