John, Ivo,
This small patchset contains two fixes to issues in the suspend/resume
handling of the rfkill class core.
It needs to go to 2.6.28. These patches are based on 2.6.28-rc3.
Chances are they will *clash* with what is in wireless-testing, so I will
send a rebased patchset for wireless-testing with these patches on the
bottom of the stack (to make merging easier with mainline).
The patches are also needed on stable, but won't apply automatically. I
don't think I will bother with 2.6.26 since the suspend/resume restore of
radio state there is not going to work well anyway, unless I backport a bit
more stuff... but 2.6.27 deserves to get 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
The rfkill class API requires that the driver connected to a class
call rfkill_force_state() on resume to update the real state of the
rfkill controller, OR that it provides a get_state() hook.
This means there is potentially a hidden call in the resume code flow
that changes rfkill->state (i.e. rfkill_force_state()), so the
previous state of the transmitter was being lost.
The simplest and most future-proof way to fix this is to explicitly
store the pre-sleep state on the rfkill structure, and restore from
that on resume.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: Alan Jenkins <[email protected]>
---
include/linux/rfkill.h | 1 +
net/rfkill/rfkill.c | 7 ++++++-
2 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index 4cd64b0..f376a93 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -108,6 +108,7 @@ struct rfkill {
struct device dev;
struct list_head node;
+ enum rfkill_state state_for_resume;
};
#define to_rfkill(d) container_of(d, struct rfkill, dev)
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index f949a48..caee717 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -511,10 +511,15 @@ static void rfkill_release(struct device *dev)
#ifdef CONFIG_PM
static int rfkill_suspend(struct device *dev, pm_message_t state)
{
+ struct rfkill *rfkill = to_rfkill(dev);
+
/* mark class device as suspended */
if (dev->power.power_state.event != state.event)
dev->power.power_state = state;
+ /* store state for the resume handler */
+ rfkill->state_for_resume = rfkill->state;
+
return 0;
}
@@ -528,7 +533,7 @@ static int rfkill_resume(struct device *dev)
dev->power.power_state.event = PM_EVENT_ON;
/* restore radio state AND notify everybody */
- rfkill_toggle_radio(rfkill, rfkill->state, 1);
+ rfkill_toggle_radio(rfkill, rfkill->state_for_resume, 1);
mutex_unlock(&rfkill->mutex);
}
--
1.5.6.5
We "optimize" away the get_state() hook call on rfkill_toggle_radio
when doing a forced state change. This means the resume path is not
calling get_state() as it should.
Call it manually on the resume handler, as we don't want to mess with
the EPO path by removing the optimization. This has the added benefit
of making it explicit that rfkill->state could have been modified
before we hit the rfkill_toggle_radio() call in the class resume
handler.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Cc: Ivo van Doorn <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: Alan Jenkins <[email protected]>
---
net/rfkill/rfkill.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index caee717..7a82a35 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -526,6 +526,7 @@ static int rfkill_suspend(struct device *dev, pm_message_t state)
static int rfkill_resume(struct device *dev)
{
struct rfkill *rfkill = to_rfkill(dev);
+ enum rfkill_state newstate;
if (dev->power.power_state.event != PM_EVENT_ON) {
mutex_lock(&rfkill->mutex);
@@ -533,6 +534,9 @@ static int rfkill_resume(struct device *dev)
dev->power.power_state.event = PM_EVENT_ON;
/* restore radio state AND notify everybody */
+ if (rfkill->get_state &&
+ !rfkill->get_state(rfkill->data, &newstate))
+ rfkill->state = newstate;
rfkill_toggle_radio(rfkill, rfkill->state_for_resume, 1);
mutex_unlock(&rfkill->mutex);
--
1.5.6.5
On Monday 03 November 2008, Henrique de Moraes Holschuh wrote:
> On Mon, 03 Nov 2008, Ivo van Doorn wrote:
> > On Monday 03 November 2008, Henrique de Moraes Holschuh wrote:
> > > This small patchset contains two fixes to issues in the suspend/resume
> > > handling of the rfkill class core.
> > >
> > > It needs to go to 2.6.28. These patches are based on 2.6.28-rc3.
> >
> > Are they real regressions or normal bugfixes?
>
> I am not sure if they regress anything, but I am pretty sure there is no
> bugzilla entry about them yet. I have added two more interested parties to
> the CC.
>
> So, if there is a strong feeling this would best be held until the next
> merge window, I can certainly respin the patches to on top of
> wireless-testing...
Well I'll give my ACK to the 2 patches, but I'll let john decide if they
are "regression" enough for 2.6.28. ;)
Ivo
On Thu, Nov 20, 2008 at 5:31 PM, Matthew Garrett <[email protected]> wrote:
> On Thu, Nov 20, 2008 at 05:27:38PM +0100, drago01 wrote:
>> On Wed, Nov 12, 2008 at 10:15 PM, John W. Linville
>> <[email protected]> wrote:
>> > So I keep looking at these patches, and I'm not sure about them.
>> > It seems that they restore the rfkill state after resume to what it
>> > was before the suspend.
>>
>> Wouldn't this break hw kill switches?
>> If I move the switch while the system is suspended it will ignore this
>> event but the hw will still not work.
>
> Whichever driver is responsible for listening to the hardware events
> should handle that in its own resume method. The rfkill state
> restoration code will only be relevant for restoring state set by
> sofware.
>
>> Well the above scenario would be just broken.
>> Unless the driver rereads the real hw state and updates the rfkill
>> state on resume.
>
> Quite. How else are you going to know if a switch has moved in the
> intervening time?
Yeah that makes sense, please ignore my comment ;)
On Monday 03 November 2008, Henrique de Moraes Holschuh wrote:
> We "optimize" away the get_state() hook call on rfkill_toggle_radio
> when doing a forced state change. This means the resume path is not
> calling get_state() as it should.
>
> Call it manually on the resume handler, as we don't want to mess with
> the EPO path by removing the optimization. This has the added benefit
> of making it explicit that rfkill->state could have been modified
> before we hit the rfkill_toggle_radio() call in the class resume
> handler.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Ivo van Doorn <[email protected]>
> Cc: Matthew Garrett <[email protected]>
> Cc: Alan Jenkins <[email protected]>
Acked-by: Ivo van Doorn <[email protected]>
> ---
> net/rfkill/rfkill.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index caee717..7a82a35 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -526,6 +526,7 @@ static int rfkill_suspend(struct device *dev, pm_message_t state)
> static int rfkill_resume(struct device *dev)
> {
> struct rfkill *rfkill = to_rfkill(dev);
> + enum rfkill_state newstate;
>
> if (dev->power.power_state.event != PM_EVENT_ON) {
> mutex_lock(&rfkill->mutex);
> @@ -533,6 +534,9 @@ static int rfkill_resume(struct device *dev)
> dev->power.power_state.event = PM_EVENT_ON;
>
> /* restore radio state AND notify everybody */
> + if (rfkill->get_state &&
> + !rfkill->get_state(rfkill->data, &newstate))
> + rfkill->state = newstate;
> rfkill_toggle_radio(rfkill, rfkill->state_for_resume, 1);
>
> mutex_unlock(&rfkill->mutex);
On Mon, 03 Nov 2008, Ivo van Doorn wrote:
> On Monday 03 November 2008, Henrique de Moraes Holschuh wrote:
> > This small patchset contains two fixes to issues in the suspend/resume
> > handling of the rfkill class core.
> >
> > It needs to go to 2.6.28. These patches are based on 2.6.28-rc3.
>
> Are they real regressions or normal bugfixes?
I am not sure if they regress anything, but I am pretty sure there is no
bugzilla entry about them yet. I have added two more interested parties to
the CC.
So, if there is a strong feeling this would best be held until the next
merge window, I can certainly respin the patches to on top of
wireless-testing...
--
"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
We "optimize" away the get_state() hook call on rfkill_toggle_radio
when doing a forced state change. This means the resume path is not
calling get_state() as it should.
Call it manually on the resume handler, as we don't want to mess with
the EPO path by removing the optimization. This has the added benefit
of making it explicit that rfkill->state could have been modified
before we hit the rfkill_toggle_radio() call in the class resume
handler.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Acked-by: Ivo van Doorn <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: Alan Jenkins <[email protected]>
---
net/rfkill/rfkill.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 4113454..ff3af0e 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -580,6 +580,7 @@ static int rfkill_suspend(struct device *dev, pm_message_t state)
static int rfkill_resume(struct device *dev)
{
struct rfkill *rfkill = to_rfkill(dev);
+ enum rfkill_state newstate;
if (dev->power.power_state.event != PM_EVENT_ON) {
mutex_lock(&rfkill->mutex);
@@ -587,6 +588,15 @@ static int rfkill_resume(struct device *dev)
dev->power.power_state.event = PM_EVENT_ON;
/*
+ * rfkill->state could have been modified before we got
+ * called, and won't be updated by rfkill_toggle_radio()
+ * in force mode. Sync it FIRST.
+ */
+ if (rfkill->get_state &&
+ !rfkill->get_state(rfkill->data, &newstate))
+ rfkill->state = newstate;
+
+ /*
* If we are under EPO, kick transmitter offline,
* otherwise restore to pre-suspend state.
*
--
1.5.6.5
On Thu, Nov 20, 2008 at 05:27:38PM +0100, drago01 wrote:
> On Wed, Nov 12, 2008 at 10:15 PM, John W. Linville
> <[email protected]> wrote:
> > So I keep looking at these patches, and I'm not sure about them.
> > It seems that they restore the rfkill state after resume to what it
> > was before the suspend.
>
> Wouldn't this break hw kill switches?
> If I move the switch while the system is suspended it will ignore this
> event but the hw will still not work.
Whichever driver is responsible for listening to the hardware events
should handle that in its own resume method. The rfkill state
restoration code will only be relevant for restoring state set by
sofware.
> Well the above scenario would be just broken.
> Unless the driver rereads the real hw state and updates the rfkill
> state on resume.
Quite. How else are you going to know if a switch has moved in the
intervening time?
--
Matthew Garrett | [email protected]
On Monday 03 November 2008, Henrique de Moraes Holschuh wrote:
> The rfkill class API requires that the driver connected to a class
> call rfkill_force_state() on resume to update the real state of the
> rfkill controller, OR that it provides a get_state() hook.
>
> This means there is potentially a hidden call in the resume code flow
> that changes rfkill->state (i.e. rfkill_force_state()), so the
> previous state of the transmitter was being lost.
>
> The simplest and most future-proof way to fix this is to explicitly
> store the pre-sleep state on the rfkill structure, and restore from
> that on resume.
>
> Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
> Cc: Ivo van Doorn <[email protected]>
> Cc: Matthew Garrett <[email protected]>
> Cc: Alan Jenkins <[email protected]>
Acked-by: Ivo van Doorn <[email protected]>
> ---
> include/linux/rfkill.h | 1 +
> net/rfkill/rfkill.c | 7 ++++++-
> 2 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
> index 4cd64b0..f376a93 100644
> --- a/include/linux/rfkill.h
> +++ b/include/linux/rfkill.h
> @@ -108,6 +108,7 @@ struct rfkill {
>
> struct device dev;
> struct list_head node;
> + enum rfkill_state state_for_resume;
> };
> #define to_rfkill(d) container_of(d, struct rfkill, dev)
>
> diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
> index f949a48..caee717 100644
> --- a/net/rfkill/rfkill.c
> +++ b/net/rfkill/rfkill.c
> @@ -511,10 +511,15 @@ static void rfkill_release(struct device *dev)
> #ifdef CONFIG_PM
> static int rfkill_suspend(struct device *dev, pm_message_t state)
> {
> + struct rfkill *rfkill = to_rfkill(dev);
> +
> /* mark class device as suspended */
> if (dev->power.power_state.event != state.event)
> dev->power.power_state = state;
>
> + /* store state for the resume handler */
> + rfkill->state_for_resume = rfkill->state;
> +
> return 0;
> }
>
> @@ -528,7 +533,7 @@ static int rfkill_resume(struct device *dev)
> dev->power.power_state.event = PM_EVENT_ON;
>
> /* restore radio state AND notify everybody */
> - rfkill_toggle_radio(rfkill, rfkill->state, 1);
> + rfkill_toggle_radio(rfkill, rfkill->state_for_resume, 1);
>
> mutex_unlock(&rfkill->mutex);
> }
On Monday 03 November 2008, Henrique de Moraes Holschuh wrote:
>
> John, Ivo,
>
> This small patchset contains two fixes to issues in the suspend/resume
> handling of the rfkill class core.
>
> It needs to go to 2.6.28. These patches are based on 2.6.28-rc3.
Are they real regressions or normal bugfixes?
> Chances are they will *clash* with what is in wireless-testing, so I will
> send a rebased patchset for wireless-testing with these patches on the
> bottom of the stack (to make merging easier with mainline).
>
> The patches are also needed on stable, but won't apply automatically. I
> don't think I will bother with 2.6.26 since the suspend/resume restore of
> radio state there is not going to work well anyway, unless I backport a bit
> more stuff... but 2.6.27 deserves to get them.
>
The rfkill class API requires that the driver connected to a class
call rfkill_force_state() on resume to update the real state of the
rfkill controller, OR that it provides a get_state() hook.
This means there is potentially a hidden call in the resume code flow
that changes rfkill->state (i.e. rfkill_force_state()), so the
previous state of the transmitter was being lost.
The simplest and most future-proof way to fix this is to explicitly
store the pre-sleep state on the rfkill structure, and restore from
that on resume.
Signed-off-by: Henrique de Moraes Holschuh <[email protected]>
Acked-by: Ivo van Doorn <[email protected]>
Cc: Matthew Garrett <[email protected]>
Cc: Alan Jenkins <[email protected]>
---
include/linux/rfkill.h | 1 +
net/rfkill/rfkill.c | 7 ++++++-
2 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/include/linux/rfkill.h b/include/linux/rfkill.h
index 4cd64b0..f376a93 100644
--- a/include/linux/rfkill.h
+++ b/include/linux/rfkill.h
@@ -108,6 +108,7 @@ struct rfkill {
struct device dev;
struct list_head node;
+ enum rfkill_state state_for_resume;
};
#define to_rfkill(d) container_of(d, struct rfkill, dev)
diff --git a/net/rfkill/rfkill.c b/net/rfkill/rfkill.c
index 69f3a3b..4113454 100644
--- a/net/rfkill/rfkill.c
+++ b/net/rfkill/rfkill.c
@@ -565,10 +565,15 @@ static void rfkill_release(struct device *dev)
#ifdef CONFIG_PM
static int rfkill_suspend(struct device *dev, pm_message_t state)
{
+ struct rfkill *rfkill = to_rfkill(dev);
+
/* mark class device as suspended */
if (dev->power.power_state.event != state.event)
dev->power.power_state = state;
+ /* store state for the resume handler */
+ rfkill->state_for_resume = rfkill->state;
+
return 0;
}
@@ -590,7 +595,7 @@ static int rfkill_resume(struct device *dev)
rfkill_toggle_radio(rfkill,
rfkill_epo_lock_active ?
RFKILL_STATE_SOFT_BLOCKED :
- rfkill->state,
+ rfkill->state_for_resume,
1);
mutex_unlock(&rfkill->mutex);
--
1.5.6.5
On Wed, 19 Nov 2008, John W. Linville wrote:
> I'm sorry, but I would prefer not to bother with this for 2.6.28.
> Could you send versions intended for wireless-testing/2.6.29?
Sure, I can do that.
Which tree/branch you want them based on? wireless-testing master? Linus
mainline?
--
"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 Wed, 12 Nov 2008, John W. Linville wrote:
> Still, I don't see how they qualify for 2.6.28. How serious do you
> think this potential problem really is?
It is not very grave, but it IS annoying... The only reason I don't
outright call it a regression, is that it was so broken before, it is just
broken in a different way without these patches.
--
"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, Nov 20, 2008 at 08:51:07AM -0200, Henrique de Moraes Holschuh wrote:
> On Wed, 19 Nov 2008, John W. Linville wrote:
> > I'm sorry, but I would prefer not to bother with this for 2.6.28.
> > Could you send versions intended for wireless-testing/2.6.29?
>
> Sure, I can do that.
>
> Which tree/branch you want them based on? wireless-testing master? Linus
> mainline?
wireless-testing, please.
Thanks!
John
--
John W. Linville Linux should be at the core
[email protected] of your literate lifestyle.
On Mon, Nov 03, 2008 at 06:20:40PM +0100, Ivo van Doorn wrote:
> On Monday 03 November 2008, Henrique de Moraes Holschuh wrote:
> > On Mon, 03 Nov 2008, Ivo van Doorn wrote:
> > > On Monday 03 November 2008, Henrique de Moraes Holschuh wrote:
> > > > This small patchset contains two fixes to issues in the suspend/resume
> > > > handling of the rfkill class core.
> > > >
> > > > It needs to go to 2.6.28. These patches are based on 2.6.28-rc3.
> > >
> > > Are they real regressions or normal bugfixes?
> >
> > I am not sure if they regress anything, but I am pretty sure there is no
> > bugzilla entry about them yet. I have added two more interested parties to
> > the CC.
> >
> > So, if there is a strong feeling this would best be held until the next
> > merge window, I can certainly respin the patches to on top of
> > wireless-testing...
>
> Well I'll give my ACK to the 2 patches, but I'll let john decide if they
> are "regression" enough for 2.6.28. ;)
So I keep looking at these patches, and I'm not sure about them.
It seems that they restore the rfkill state after resume to what it
was before the suspend. What I am unsure about is whether or not
that is the appropriate thing to do. I suppose it makes sense under
the rule of least surprise.
I guess the safer alternative would be to always resume with rfkill
enabled, but that is probably undesirable in the most common case.
So, I have no objection to these patches.
Still, I don't see how they qualify for 2.6.28. How serious do you
think this potential problem really is?
John
--
John W. Linville Linux should be at the core
[email protected] of your literate lifestyle.
On Thu, Nov 13, 2008 at 10:16:24PM -0200, Henrique de Moraes Holschuh wrote:
> On Wed, 12 Nov 2008, John W. Linville wrote:
> > Still, I don't see how they qualify for 2.6.28. How serious do you
> > think this potential problem really is?
>
> It is not very grave, but it IS annoying... The only reason I don't
> outright call it a regression, is that it was so broken before, it is just
> broken in a different way without these patches.
I'm sorry, but I would prefer not to bother with this for 2.6.28.
Could you send versions intended for wireless-testing/2.6.29?
Thanks,
John
--
John W. Linville Linux should be at the core
[email protected] of your literate lifestyle.
On Wed, Nov 12, 2008 at 10:15 PM, John W. Linville
<[email protected]> wrote:
> On Mon, Nov 03, 2008 at 06:20:40PM +0100, Ivo van Doorn wrote:
>> On Monday 03 November 2008, Henrique de Moraes Holschuh wrote:
>> > On Mon, 03 Nov 2008, Ivo van Doorn wrote:
>> > > On Monday 03 November 2008, Henrique de Moraes Holschuh wrote:
>> > > > This small patchset contains two fixes to issues in the suspend/resume
>> > > > handling of the rfkill class core.
>> > > >
>> > > > It needs to go to 2.6.28. These patches are based on 2.6.28-rc3.
>> > >
>> > > Are they real regressions or normal bugfixes?
>> >
>> > I am not sure if they regress anything, but I am pretty sure there is no
>> > bugzilla entry about them yet. I have added two more interested parties to
>> > the CC.
>> >
>> > So, if there is a strong feeling this would best be held until the next
>> > merge window, I can certainly respin the patches to on top of
>> > wireless-testing...
>>
>> Well I'll give my ACK to the 2 patches, but I'll let john decide if they
>> are "regression" enough for 2.6.28. ;)
>
> So I keep looking at these patches, and I'm not sure about them.
> It seems that they restore the rfkill state after resume to what it
> was before the suspend.
Wouldn't this break hw kill switches?
If I move the switch while the system is suspended it will ignore this
event but the hw will still not work.
> What I am unsure about is whether or not
> that is the appropriate thing to do. I suppose it makes sense under
> the rule of least surprise.
Well the above scenario would be just broken.
Unless the driver rereads the real hw state and updates the rfkill
state on resume.