The setting of the "persistent" flag is also made more explicit using
a new rfkill_init_sw_state() function, instead of special-casing
rfkill_set_sw_state() when it is called before registration.
Suspend is a bit of a corner case so we try to get away without adding
another hack to rfkill-input - it's going to be removed soon.
If the state does change over suspend, users will simply have to prod
rfkill-input twice in order to toggle the state.
Userspace policy agents will be able to implement a more consistent user
experience. For example, they can avoid the above problem if they
toggle devices individually. Then there would be no "global state"
to get out of sync.
Currently there are only two rfkill drivers with persistent soft-blocked
state. thinkpad-acpi already checks the software state on resume.
eeepc-laptop will require modification.
Signed-off-by: Alan Jenkins <[email protected]>
CC: Marcel Holtmann <[email protected]>
---
drivers/platform/x86/eeepc-laptop.c | 8 +++---
drivers/platform/x86/thinkpad_acpi.c | 14 ++++++------
include/linux/rfkill.h | 28 +++++++++++++++++++----
net/rfkill/core.c | 40 +++++++++++++++++++++------------
4 files changed, 59 insertions(+), 31 deletions(-)
diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 03bf522..01682ec 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -675,8 +675,8 @@ static int eeepc_hotk_add(struct acpi_device *device)
if (!ehotk->eeepc_wlan_rfkill)
goto wlan_fail;
- rfkill_set_sw_state(ehotk->eeepc_wlan_rfkill,
- get_acpi(CM_ASL_WLAN) != 1);
+ rfkill_init_sw_state(ehotk->eeepc_wlan_rfkill,
+ get_acpi(CM_ASL_WLAN) != 1);
result = rfkill_register(ehotk->eeepc_wlan_rfkill);
if (result)
goto wlan_fail;
@@ -693,8 +693,8 @@ static int eeepc_hotk_add(struct acpi_device *device)
if (!ehotk->eeepc_bluetooth_rfkill)
goto bluetooth_fail;
- rfkill_set_sw_state(ehotk->eeepc_bluetooth_rfkill,
- get_acpi(CM_ASL_BLUETOOTH) != 1);
+ rfkill_init_sw_state(ehotk->eeepc_bluetooth_rfkill,
+ get_acpi(CM_ASL_BLUETOOTH) != 1);
result = rfkill_register(ehotk->eeepc_bluetooth_rfkill);
if (result)
goto bluetooth_fail;
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 86e9585..40d64c0 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -1163,8 +1163,8 @@ static int __init tpacpi_new_rfkill(const enum tpacpi_rfk_id id,
{
struct tpacpi_rfk *atp_rfk;
int res;
- bool initial_sw_state = false;
- int initial_sw_status;
+ bool sw_state = false;
+ int sw_status;
BUG_ON(id >= TPACPI_RFK_SW_MAX || tpacpi_rfkill_switches[id]);
@@ -1185,17 +1185,17 @@ static int __init tpacpi_new_rfkill(const enum tpacpi_rfk_id id,
atp_rfk->id = id;
atp_rfk->ops = tp_rfkops;
- initial_sw_status = (tp_rfkops->get_status)();
- if (initial_sw_status < 0) {
+ sw_status = (tp_rfkops->get_status)();
+ if (sw_status < 0) {
printk(TPACPI_ERR
"failed to read initial state for %s, error %d\n",
- name, initial_sw_status);
+ name, sw_status);
} else {
- initial_sw_state = (initial_sw_status == TPACPI_RFK_RADIO_OFF);
+ sw_state = (sw_status == TPACPI_RFK_RADIO_OFF);
if (set_default) {
/* try to keep the initial state, since we ask the
* firmware to preserve it across S5 in NVRAM */
- rfkill_set_sw_state(atp_rfk->rfkill, initial_sw_state);
+ rfkill_init_sw_state(atp_rfk->rfkill, sw_state);
}
}
rfkill_set_hw_state(atp_rfk->rfkill, tpacpi_rfk_check_hwblock_state());
diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index 16e39c7..84b6b9c 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -160,8 +160,9 @@ struct rfkill * __must_check rfkill_alloc(const char *name,
* the rfkill structure. Before calling this function the driver needs
* to be ready to service method calls from rfkill.
*
- * If the software blocked state is not set before registration,
- * set_block will be called to initialize it to a default value.
+ * If rfkill_init_sw_state() is not called before registration,
+ * set_block() will be called to initialize the software blocked state
+ * to a default value.
*
* If the hardware blocked state is not set before registration,
* it is assumed to be unblocked.
@@ -234,9 +235,11 @@ bool __must_check rfkill_set_hw_state(struct rfkill *rfkill, bool blocked);
* rfkill drivers that get events when the soft-blocked state changes
* (yes, some platforms directly act on input but allow changing again)
* use this function to notify the rfkill core (and through that also
- * userspace) of the current state. It is not necessary to notify on
- * resume; since hibernation can always change the soft-blocked state,
- * the rfkill core will unconditionally restore the previous state.
+ * userspace) of the current state.
+ *
+ * Drivers should also call this function after resume if the state has
+ * been changed by the user. This only makes sense for "persistent"
+ * devices (see rfkill_init_sw_state()).
*
* This function can be called in any context, even from within rfkill
* callbacks.
@@ -247,6 +250,21 @@ bool __must_check rfkill_set_hw_state(struct rfkill *rfkill, bool blocked);
bool rfkill_set_sw_state(struct rfkill *rfkill, bool blocked);
/**
+ * rfkill_init_sw_state - Initialize persistent software block state
+ * @rfkill: pointer to the rfkill class to modify.
+ * @state: the current software block state to set
+ *
+ * rfkill drivers that preserve their software block state over power off
+ * use this function to notify the rfkill core (and through that also
+ * userspace) of their initial state. It should only be used before
+ * registration.
+ *
+ * In addition, it marks the device as "persistent". Persistent devices
+ * are expected to preserve preserve their own state when suspended.
+ */
+void rfkill_init_sw_state(struct rfkill *rfkill, bool blocked);
+
+/**
* rfkill_set_states - Set the internal rfkill block states
* @rfkill: pointer to the rfkill class to modify.
* @sw: the current software block state to set
diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 868d79f..dcf8df7 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -56,7 +56,6 @@ struct rfkill {
u32 idx;
bool registered;
- bool suspended;
bool persistent;
const struct rfkill_ops *ops;
@@ -224,7 +223,7 @@ static void rfkill_send_events(struct rfkill *rfkill, enum rfkill_operation op)
static void rfkill_event(struct rfkill *rfkill)
{
- if (!rfkill->registered || rfkill->suspended)
+ if (!rfkill->registered)
return;
kobject_uevent(&rfkill->dev.kobj, KOBJ_CHANGE);
@@ -508,19 +507,32 @@ bool rfkill_set_sw_state(struct rfkill *rfkill, bool blocked)
blocked = blocked || hwblock;
spin_unlock_irqrestore(&rfkill->lock, flags);
- if (!rfkill->registered) {
- rfkill->persistent = true;
- } else {
- if (prev != blocked && !hwblock)
- schedule_work(&rfkill->uevent_work);
+ if (!rfkill->registered)
+ return blocked;
- rfkill_led_trigger_event(rfkill);
- }
+ if (prev != blocked && !hwblock)
+ schedule_work(&rfkill->uevent_work);
+
+ rfkill_led_trigger_event(rfkill);
return blocked;
}
EXPORT_SYMBOL(rfkill_set_sw_state);
+void rfkill_init_sw_state(struct rfkill *rfkill, bool blocked)
+{
+ unsigned long flags;
+
+ BUG_ON(!rfkill);
+ BUG_ON(rfkill->registered);
+
+ spin_lock_irqsave(&rfkill->lock, flags);
+ __rfkill_set_sw_state(rfkill, blocked);
+ rfkill->persistent = true;
+ spin_unlock_irqrestore(&rfkill->lock, flags);
+}
+EXPORT_SYMBOL(rfkill_init_sw_state);
+
void rfkill_set_states(struct rfkill *rfkill, bool sw, bool hw)
{
unsigned long flags;
@@ -718,8 +730,6 @@ static int rfkill_suspend(struct device *dev, pm_message_t state)
rfkill_pause_polling(rfkill);
- rfkill->suspended = true;
-
return 0;
}
@@ -728,10 +738,10 @@ static int rfkill_resume(struct device *dev)
struct rfkill *rfkill = to_rfkill(dev);
bool cur;
- cur = !!(rfkill->state & RFKILL_BLOCK_SW);
- rfkill_set_block(rfkill, cur);
-
- rfkill->suspended = false;
+ if (!rfkill->persistent) {
+ cur = !!(rfkill->state & RFKILL_BLOCK_SW);
+ rfkill_set_block(rfkill, cur);
+ }
rfkill_resume_polling(rfkill);
On Tue, 16 Jun 2009, Alan Jenkins wrote:
> The setting of the "persistent" flag is also made more explicit using
> a new rfkill_init_sw_state() function, instead of special-casing
> rfkill_set_sw_state() when it is called before registration.
>
> Suspend is a bit of a corner case so we try to get away without adding
> another hack to rfkill-input - it's going to be removed soon.
> If the state does change over suspend, users will simply have to prod
> rfkill-input twice in order to toggle the state.
>
> Userspace policy agents will be able to implement a more consistent user
> experience. For example, they can avoid the above problem if they
> toggle devices individually. Then there would be no "global state"
> to get out of sync.
>
> Currently there are only two rfkill drivers with persistent soft-blocked
> state. thinkpad-acpi already checks the software state on resume.
> eeepc-laptop will require modification.
>
> Signed-off-by: Alan Jenkins <[email protected]>
> CC: Marcel Holtmann <[email protected]>
Acked-by: Henrique de Moraes Holschuh <[email protected]>
--
"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
On Tue, 2009-06-16 at 14:53 +0100, Alan Jenkins wrote:
> /**
> + * rfkill_init_sw_state - Initialize persistent software block state
> + * @rfkill: pointer to the rfkill class to modify.
> + * @state: the current software block state to set
> + *
> + * rfkill drivers that preserve their software block state over power off
> + * use this function to notify the rfkill core (and through that also
> + * userspace) of their initial state. It should only be used before
> + * registration.
> + *
> + * In addition, it marks the device as "persistent". Persistent devices
> + * are expected to preserve preserve their own state when suspended.
> + */
> +void rfkill_init_sw_state(struct rfkill *rfkill, bool blocked);
> +
> +/**
> * rfkill_set_states - Set the internal rfkill block states
> * @rfkill: pointer to the rfkill class to modify.
> * @sw: the current software block state to set
You forgot the inline for !RFKILL.
Otherwise looks good.
johannes
Hi Alan,
> The setting of the "persistent" flag is also made more explicit using
> a new rfkill_init_sw_state() function, instead of special-casing
> rfkill_set_sw_state() when it is called before registration.
>
> Suspend is a bit of a corner case so we try to get away without adding
> another hack to rfkill-input - it's going to be removed soon.
> If the state does change over suspend, users will simply have to prod
> rfkill-input twice in order to toggle the state.
>
> Userspace policy agents will be able to implement a more consistent user
> experience. For example, they can avoid the above problem if they
> toggle devices individually. Then there would be no "global state"
> to get out of sync.
>
> Currently there are only two rfkill drivers with persistent soft-blocked
> state. thinkpad-acpi already checks the software state on resume.
> eeepc-laptop will require modification.
>
> Signed-off-by: Alan Jenkins <[email protected]>
> CC: Marcel Holtmann <[email protected]>
looks good to me and is way better than using a complicated API for this
non-volatile storage details.
Acked-by: Marcel Holtmann <[email protected]>
Regards
Marcel
The setting of the "persistent" flag is also made more explicit using
a new rfkill_init_sw_state() function, instead of special-casing
rfkill_set_sw_state() when it is called before registration.
Suspend is a bit of a corner case so we try to get away without adding
another hack to rfkill-input - it's going to be removed soon.
If the state does change over suspend, users will simply have to prod
rfkill-input twice in order to toggle the state.
Userspace policy agents will be able to implement a more consistent user
experience. For example, they can avoid the above problem if they
toggle devices individually. Then there would be no "global state"
to get out of sync.
Currently there are only two rfkill drivers with persistent soft-blocked
state. thinkpad-acpi already checks the software state on resume.
eeepc-laptop will require modification.
Signed-off-by: Alan Jenkins <[email protected]>
CC: Marcel Holtmann <[email protected]>
---
v2: build fix for RFKILL=n
drivers/platform/x86/eeepc-laptop.c | 8 +++---
drivers/platform/x86/thinkpad_acpi.c | 14 ++++++------
include/linux/rfkill.h | 32 +++++++++++++++++++++++----
net/rfkill/core.c | 40 +++++++++++++++++++++------------
4 files changed, 63 insertions(+), 31 deletions(-)
diff --git a/drivers/platform/x86/eeepc-laptop.c b/drivers/platform/x86/eeepc-laptop.c
index 03bf522..01682ec 100644
--- a/drivers/platform/x86/eeepc-laptop.c
+++ b/drivers/platform/x86/eeepc-laptop.c
@@ -675,8 +675,8 @@ static int eeepc_hotk_add(struct acpi_device *device)
if (!ehotk->eeepc_wlan_rfkill)
goto wlan_fail;
- rfkill_set_sw_state(ehotk->eeepc_wlan_rfkill,
- get_acpi(CM_ASL_WLAN) != 1);
+ rfkill_init_sw_state(ehotk->eeepc_wlan_rfkill,
+ get_acpi(CM_ASL_WLAN) != 1);
result = rfkill_register(ehotk->eeepc_wlan_rfkill);
if (result)
goto wlan_fail;
@@ -693,8 +693,8 @@ static int eeepc_hotk_add(struct acpi_device *device)
if (!ehotk->eeepc_bluetooth_rfkill)
goto bluetooth_fail;
- rfkill_set_sw_state(ehotk->eeepc_bluetooth_rfkill,
- get_acpi(CM_ASL_BLUETOOTH) != 1);
+ rfkill_init_sw_state(ehotk->eeepc_bluetooth_rfkill,
+ get_acpi(CM_ASL_BLUETOOTH) != 1);
result = rfkill_register(ehotk->eeepc_bluetooth_rfkill);
if (result)
goto bluetooth_fail;
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 86e9585..40d64c0 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -1163,8 +1163,8 @@ static int __init tpacpi_new_rfkill(const enum tpacpi_rfk_id id,
{
struct tpacpi_rfk *atp_rfk;
int res;
- bool initial_sw_state = false;
- int initial_sw_status;
+ bool sw_state = false;
+ int sw_status;
BUG_ON(id >= TPACPI_RFK_SW_MAX || tpacpi_rfkill_switches[id]);
@@ -1185,17 +1185,17 @@ static int __init tpacpi_new_rfkill(const enum tpacpi_rfk_id id,
atp_rfk->id = id;
atp_rfk->ops = tp_rfkops;
- initial_sw_status = (tp_rfkops->get_status)();
- if (initial_sw_status < 0) {
+ sw_status = (tp_rfkops->get_status)();
+ if (sw_status < 0) {
printk(TPACPI_ERR
"failed to read initial state for %s, error %d\n",
- name, initial_sw_status);
+ name, sw_status);
} else {
- initial_sw_state = (initial_sw_status == TPACPI_RFK_RADIO_OFF);
+ sw_state = (sw_status == TPACPI_RFK_RADIO_OFF);
if (set_default) {
/* try to keep the initial state, since we ask the
* firmware to preserve it across S5 in NVRAM */
- rfkill_set_sw_state(atp_rfk->rfkill, initial_sw_state);
+ rfkill_init_sw_state(atp_rfk->rfkill, sw_state);
}
}
rfkill_set_hw_state(atp_rfk->rfkill, tpacpi_rfk_check_hwblock_state());
diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index 16e39c7..dcac724 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -160,8 +160,9 @@ struct rfkill * __must_check rfkill_alloc(const char *name,
* the rfkill structure. Before calling this function the driver needs
* to be ready to service method calls from rfkill.
*
- * If the software blocked state is not set before registration,
- * set_block will be called to initialize it to a default value.
+ * If rfkill_init_sw_state() is not called before registration,
+ * set_block() will be called to initialize the software blocked state
+ * to a default value.
*
* If the hardware blocked state is not set before registration,
* it is assumed to be unblocked.
@@ -234,9 +235,11 @@ bool __must_check rfkill_set_hw_state(struct rfkill *rfkill, bool blocked);
* rfkill drivers that get events when the soft-blocked state changes
* (yes, some platforms directly act on input but allow changing again)
* use this function to notify the rfkill core (and through that also
- * userspace) of the current state. It is not necessary to notify on
- * resume; since hibernation can always change the soft-blocked state,
- * the rfkill core will unconditionally restore the previous state.
+ * userspace) of the current state.
+ *
+ * Drivers should also call this function after resume if the state has
+ * been changed by the user. This only makes sense for "persistent"
+ * devices (see rfkill_init_sw_state()).
*
* This function can be called in any context, even from within rfkill
* callbacks.
@@ -247,6 +250,21 @@ bool __must_check rfkill_set_hw_state(struct rfkill *rfkill, bool blocked);
bool rfkill_set_sw_state(struct rfkill *rfkill, bool blocked);
/**
+ * rfkill_init_sw_state - Initialize persistent software block state
+ * @rfkill: pointer to the rfkill class to modify.
+ * @state: the current software block state to set
+ *
+ * rfkill drivers that preserve their software block state over power off
+ * use this function to notify the rfkill core (and through that also
+ * userspace) of their initial state. It should only be used before
+ * registration.
+ *
+ * In addition, it marks the device as "persistent". Persistent devices
+ * are expected to preserve preserve their own state when suspended.
+ */
+void rfkill_init_sw_state(struct rfkill *rfkill, bool blocked);
+
+/**
* rfkill_set_states - Set the internal rfkill block states
* @rfkill: pointer to the rfkill class to modify.
* @sw: the current software block state to set
@@ -307,6 +325,10 @@ static inline bool rfkill_set_sw_state(struct rfkill *rfkill, bool blocked)
return blocked;
}
+static inline void rfkill_init_sw_state(struct rfkill *rfkill, bool blocked)
+{
+}
+
static inline void rfkill_set_states(struct rfkill *rfkill, bool sw, bool hw)
{
}
diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index 868d79f..dcf8df7 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -56,7 +56,6 @@ struct rfkill {
u32 idx;
bool registered;
- bool suspended;
bool persistent;
const struct rfkill_ops *ops;
@@ -224,7 +223,7 @@ static void rfkill_send_events(struct rfkill *rfkill, enum rfkill_operation op)
static void rfkill_event(struct rfkill *rfkill)
{
- if (!rfkill->registered || rfkill->suspended)
+ if (!rfkill->registered)
return;
kobject_uevent(&rfkill->dev.kobj, KOBJ_CHANGE);
@@ -508,19 +507,32 @@ bool rfkill_set_sw_state(struct rfkill *rfkill, bool blocked)
blocked = blocked || hwblock;
spin_unlock_irqrestore(&rfkill->lock, flags);
- if (!rfkill->registered) {
- rfkill->persistent = true;
- } else {
- if (prev != blocked && !hwblock)
- schedule_work(&rfkill->uevent_work);
+ if (!rfkill->registered)
+ return blocked;
- rfkill_led_trigger_event(rfkill);
- }
+ if (prev != blocked && !hwblock)
+ schedule_work(&rfkill->uevent_work);
+
+ rfkill_led_trigger_event(rfkill);
return blocked;
}
EXPORT_SYMBOL(rfkill_set_sw_state);
+void rfkill_init_sw_state(struct rfkill *rfkill, bool blocked)
+{
+ unsigned long flags;
+
+ BUG_ON(!rfkill);
+ BUG_ON(rfkill->registered);
+
+ spin_lock_irqsave(&rfkill->lock, flags);
+ __rfkill_set_sw_state(rfkill, blocked);
+ rfkill->persistent = true;
+ spin_unlock_irqrestore(&rfkill->lock, flags);
+}
+EXPORT_SYMBOL(rfkill_init_sw_state);
+
void rfkill_set_states(struct rfkill *rfkill, bool sw, bool hw)
{
unsigned long flags;
@@ -718,8 +730,6 @@ static int rfkill_suspend(struct device *dev, pm_message_t state)
rfkill_pause_polling(rfkill);
- rfkill->suspended = true;
-
return 0;
}
@@ -728,10 +738,10 @@ static int rfkill_resume(struct device *dev)
struct rfkill *rfkill = to_rfkill(dev);
bool cur;
- cur = !!(rfkill->state & RFKILL_BLOCK_SW);
- rfkill_set_block(rfkill, cur);
-
- rfkill->suspended = false;
+ if (!rfkill->persistent) {
+ cur = !!(rfkill->state & RFKILL_BLOCK_SW);
+ rfkill_set_block(rfkill, cur);
+ }
rfkill_resume_polling(rfkill);
On Sat, 2009-11-28 at 11:27 -0200, Henrique de Moraes Holschuh wrote:
> > static int rfkill_resume(struct device *dev)
> > {
> > struct rfkill *rfkill = to_rfkill(dev);
> > bool cur;
> >
> > if (!rfkill->persistent) {
> > cur = !!(rfkill->state & RFKILL_BLOCK_SW);
> > rfkill_set_block(rfkill, cur);
> > }
>
> The issue I am reporting is _only_ about persistent devices, so it is the
> if(!rfkill->persistent) that is the problem. It works perfectly for the
> other devices.
So then I don't understand -- if the device specifically said that it
was persistent, what kind of help does it need, and why does it not just
unset persistent if it needs it?
johannes
Henrique de Moraes Holschuh wrote:
> On Thu, 18 Jun 2009, Henrique de Moraes Holschuh wrote:
>
>>> The setting of the "persistent" flag is also made more explicit using
>>> a new rfkill_init_sw_state() function, instead of special-casing
>>> rfkill_set_sw_state() when it is called before registration.
>>>
>>> Suspend is a bit of a corner case so we try to get away without adding
>>> another hack to rfkill-input - it's going to be removed soon.
>>> If the state does change over suspend, users will simply have to prod
>>> rfkill-input twice in order to toggle the state.
>>>
>
> Well, I acked it, but I didn't do my job properly and didn't notice the
> above.
>
> Suspend/resume is not much of a corner case at all: unless overriden by
> some weird policy that doesn't even exist right now (the only one that comes
> to mind is to have it always off upon resume in highly-critical
> environments), we really want to restore the hardware to the previous soft
> state, subject to any changes on any hard state that happened while the
> machine was sleeping.
>
> And any "weird policy" we might implement would have to come through the
> core anyway. So, we really should be taking advantage of the fact that the
> rfkill class will resume *after* the device, and let the core call the
> backend (unconditionally, so it is also a way to make sure the
> firmware/hardware is in sync with the core) to set the proper state after a
> resume.
>
> The backend will be able to update any hard states before the rfkill class
> resume code runs, so this will always work fine. It also allows the
> backend driver to ask the platform to resume with radios disabled, so that
> if we _ever_ decide to change the core to have a different policy to what
> should be done to radios on resume (e.g. leave them off and wait for
> userspace to tell us what to do :) ), that will need no change to drivers
> (and radios won't get turned on just to be turned off, etc).
>
> It will also let us remove a few LOC from eeepc-laptop and avoid adding a
> few LOC to thinkpad-acpi (which has a regression since 2.6.30 because failed
> to notice I would have to handle resume).
>
> What do you guys think? I will cook up a patch to implement the above, but
> if there are any objections to the idea, I'd like to hear it ASAP, as I do
> have a regression to fix :)
>
> Note: eeepc-laptop and thinkpad-acpi are the only rfkill persistent devices
> in-tree
"Suspend/resume is not much of a corner case at all"
Sorry for my naff patch description. The actual corner case is when the
soft rfkill state changes between when we suspend and resume.
"we really want to restore the hardware to the previous soft state"
Context warning: this only affects "persistent" rfkill devices. We
currently do this for everything except thinkpad-acpi and eeepc-laptop.
Blame Marcel, the current choice was his suggestion :). I think his
argument was that restoring the state could impose policy, and that this
would be bad. I didn't resist too hard because his principle provides a
marginal feature in eeepc-laptop.
[That said, I later noticed that it speeds up resume from s2ram.
eeepc-laptop is 'special'; its WLDS method takes a whole second to run.]
To elaborate:
The state in NV-ram may be changed deliberately. On eeepc-laptop, you
can change it in the BIOS setup screen - and then resume from
hibernation. Marcel suggests that overriding this change would be a
policy decision, which userspace should be able to control. The simplest
way to do so is to simply preserve the state (and emit an event to
notify userspace of the change).
I thought thinkpad-acpi might also allow such changes. At least for some
controls, the BIOS defaults to implementing them itself, right? Even if
this isn't true for rfkill on the thinkpad, it's a possibility we may
encounter in future. Imagine a system where this happens:
- hibernate
- boot resume kernel
- user presses rfkill key -> BIOS toggles the rfkill state
- load kernel from hibernation image
- rfkill resume now unconditionally overrides the rfkill state
So your suggestion forces users to accept corner case behaviour like
this. Marcel's position appears to come from writing a userspace daemon
which provides persistent state for all rfkill devices. That is, if we
want to keep in-kernel handling of persistent state in the long run, we
have to justify it by addressing the corner cases which only the kernel
can solve.
rfkill-input is scheduled for removal in anticipation of rfkilld. If you
want to schedule removal of persistent rfkill devices in anticipation of
"rfkilld with persistence", that would seem just as reasonable :-p. I'm
sure Johannes would then accept the small change to the rfkill core to
fix this regression.
Alan
On Sat, 2009-11-28 at 10:41 -0200, Henrique de Moraes Holschuh wrote:
> What do you guys think? I will cook up a patch to implement the above, but
> if there are any objections to the idea, I'd like to hear it ASAP, as I do
> have a regression to fix :)
What part of that don't we do?
static int rfkill_resume(struct device *dev)
{
struct rfkill *rfkill = to_rfkill(dev);
bool cur;
if (!rfkill->persistent) {
cur = !!(rfkill->state & RFKILL_BLOCK_SW);
rfkill_set_block(rfkill, cur);
}
johannes
On Sat, 28 Nov 2009, Alan Jenkins wrote:
> Sorry for my naff patch description. The actual corner case is when
> the soft rfkill state changes between when we suspend and resume.
Ah, ok.
> "we really want to restore the hardware to the previous soft state"
>
> Context warning: this only affects "persistent" rfkill devices. We
> currently do this for everything except thinkpad-acpi and
> eeepc-laptop.
Yes. This is _only_ about persistent rfkill devices, i.e. thinkpads and
eeepc.
> Blame Marcel, the current choice was his suggestion :). I think his
> argument was that restoring the state could impose policy, and that
> this would be bad. I didn't resist too hard because his principle
> provides a marginal feature in eeepc-laptop.
Actually, that is a good argument, and I was not aware of it.
> [That said, I later noticed that it speeds up resume from s2ram.
> eeepc-laptop is 'special'; its WLDS method takes a whole second to
> run.]
Which is another good argument.
I can fix the regression in thinkpad-acpi either way, and I have just
found that I _will_ have to change code in thinkpad-acpi to fix it in
either case.
But I'd rather do it in the best way possible :)
If the current way (no resume for persistent devices) has real
advantages, I will just deal with it in the driver.
> To elaborate:
>
> The state in NV-ram may be changed deliberately. On eeepc-laptop,
> you can change it in the BIOS setup screen - and then resume from
> hibernation. Marcel suggests that overriding this change would be a
> policy decision, which userspace should be able to control. The
> simplest way to do so is to simply preserve the state (and emit an
> event to notify userspace of the change).
I see. that's the missing link. While thinkpads _can_ do that too, it
changes the *hard* kill line, and it (obviously) cannot be overriden at
all... When you disable radios in the ThinkPad BIOS, they _stay_
disabled... at least on the IBM models.
I was aware of the change-in-NVRAM possibility, but dismissed it since
we don't support booting another OS in the middle of a hibernation
cycle. The idea that the BIOS could be used to change NVRAM didn't
cross my mind.
> I thought thinkpad-acpi might also allow such changes. At least for
> some controls, the BIOS defaults to implementing them itself, right?
Yes, but for the radios, the BIOS goes the way of "disabled is
disabled", and doesn't let you change the NVRAM. That might have
changed on the newer Lenovo models, but I have not heard of any changes
yet.
> Even if this isn't true for rfkill on the thinkpad, it's a
> possibility we may encounter in future. Imagine a system where this
You told me EEEPC takes good advantage of the current way of doing
things, and I can certainly make it work properly in thinkpad-acpi as
well.
So, as far as I am concerned, you proved to me that there are strong
advantages for the current way of doing things, and until there is a
compelling need to have radios locked on resume, I think it is best to
leave things as is.
> - hibernate
> - boot resume kernel
> - user presses rfkill key -> BIOS toggles the rfkill state
> - load kernel from hibernation image
> - rfkill resume now unconditionally overrides the rfkill state
Yeah, ugly mess.
> rfkill-input is scheduled for removal in anticipation of rfkilld. If
> you want to schedule removal of persistent rfkill devices in
> anticipation of "rfkilld with persistence", that would seem just as
> reasonable :-p. I'm sure Johannes would then accept the small change
> to the rfkill core to fix this regression.
Nah, I think it is best to deal it in the driver, given the data you
just presented.
And I am strongly against foring users to use rfkilld-based persistence
in platforms that can do better (i.e. thinkpad-acpi and eeepc) :-)
--
"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
On Sat, 28 Nov 2009, Johannes Berg wrote:
> On Sat, 2009-11-28 at 10:41 -0200, Henrique de Moraes Holschuh wrote:
> > What do you guys think? I will cook up a patch to implement the above, but
> > if there are any objections to the idea, I'd like to hear it ASAP, as I do
> > have a regression to fix :)
>
> What part of that don't we do?
>
> static int rfkill_resume(struct device *dev)
> {
> struct rfkill *rfkill = to_rfkill(dev);
> bool cur;
>
> if (!rfkill->persistent) {
> cur = !!(rfkill->state & RFKILL_BLOCK_SW);
> rfkill_set_block(rfkill, cur);
> }
The issue I am reporting is _only_ about persistent devices, so it is the
if(!rfkill->persistent) that is the problem. It works perfectly for the
other devices.
--
"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
On Thu, 18 Jun 2009, Henrique de Moraes Holschuh wrote:
> > The setting of the "persistent" flag is also made more explicit using
> > a new rfkill_init_sw_state() function, instead of special-casing
> > rfkill_set_sw_state() when it is called before registration.
> >
> > Suspend is a bit of a corner case so we try to get away without adding
> > another hack to rfkill-input - it's going to be removed soon.
> > If the state does change over suspend, users will simply have to prod
> > rfkill-input twice in order to toggle the state.
Well, I acked it, but I didn't do my job properly and didn't notice the
above.
Suspend/resume is not much of a corner case at all: unless overriden by
some weird policy that doesn't even exist right now (the only one that comes
to mind is to have it always off upon resume in highly-critical
environments), we really want to restore the hardware to the previous soft
state, subject to any changes on any hard state that happened while the
machine was sleeping.
And any "weird policy" we might implement would have to come through the
core anyway. So, we really should be taking advantage of the fact that the
rfkill class will resume *after* the device, and let the core call the
backend (unconditionally, so it is also a way to make sure the
firmware/hardware is in sync with the core) to set the proper state after a
resume.
The backend will be able to update any hard states before the rfkill class
resume code runs, so this will always work fine. It also allows the
backend driver to ask the platform to resume with radios disabled, so that
if we _ever_ decide to change the core to have a different policy to what
should be done to radios on resume (e.g. leave them off and wait for
userspace to tell us what to do :) ), that will need no change to drivers
(and radios won't get turned on just to be turned off, etc).
It will also let us remove a few LOC from eeepc-laptop and avoid adding a
few LOC to thinkpad-acpi (which has a regression since 2.6.30 because failed
to notice I would have to handle resume).
What do you guys think? I will cook up a patch to implement the above, but
if there are any objections to the idea, I'd like to hear it ASAP, as I do
have a regression to fix :)
Note: eeepc-laptop and thinkpad-acpi are the only rfkill persistent devices
in-tree.
--
"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
On Sat, 28 Nov 2009, Johannes Berg wrote:
> On Sat, 2009-11-28 at 11:27 -0200, Henrique de Moraes Holschuh wrote:
> > > static int rfkill_resume(struct device *dev)
> > > {
> > > struct rfkill *rfkill = to_rfkill(dev);
> > > bool cur;
> > >
> > > if (!rfkill->persistent) {
> > > cur = !!(rfkill->state & RFKILL_BLOCK_SW);
> > > rfkill_set_block(rfkill, cur);
> > > }
> >
> > The issue I am reporting is _only_ about persistent devices, so it is the
> > if(!rfkill->persistent) that is the problem. It works perfectly for the
> > other devices.
>
> So then I don't understand -- if the device specifically said that it
> was persistent, what kind of help does it need, and why does it not just
> unset persistent if it needs it?
Never mind, Alan gave me various good reasons for the code to remain as is,
and I will fix it in the driver in another way.
--
"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