Thinkpad X200s

Thinkpad X200s

[Ian Molton]

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

Hi folks,

I've been investigating a problem with rfkill on the X200s from Lenovo.
The problem exists in debians 2.6.30-2 kernel and persists right up to -git

I've found that the WWAN card does not behave as expected on suspend /
resume.

The problem seems to be the rfkill layer getting out of sync with ACPIs
idea of the hardware state.

If the laptop is suspended with WWAN enabled, it will wake up with it
disabled, requiring a toggle of the physical WWAN switch or a prod in
/sys to get it going again.

I wrote a little patch that "cures" this issue (attached), however it is
far from perfect.

For example, even with this patch, when the side switch is used, it
overrides the soft setting for the devices completely, so if for example
the WWAN was off, then toggling the switch changes its rfkill state from
0 -> 2, and then from 2 -> 1

It *appears* from the code that it should be possible to get the device
to power up in the state it was last in, but I cant make mine do it.

I've also noticed that on recent kernels, I get an extra rfkillswitch
for hci0 (bluetooth) in addition to the ACPI one. This vanishes if power
is toggled using the ACPI rfkill softswitch, however bluetooth continues
to work (when enabled).

Has anyone else experienced these issues?

-Ian

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1991 bytes --]

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index d93108d..4896af5 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -4055,14 +4055,37 @@ enum {
 };
 
 #define TPACPI_RFK_WWAN_SW_NAME		"tpacpi_wwan_sw"
+static int laststat;
 
 static void wan_suspend(pm_message_t state)
 {
+	int status;
+	acpi_evalf(hkey_handle, &status, "GWAN", "d");
+	laststat = status;
+        printk("s%d stat.\n", status);
 	/* Try to make sure radio will resume powered off */
 	if (!acpi_evalf(NULL, NULL, "\\WGSV", "qvd",
 		   TP_ACPI_WGSV_PWR_OFF_ON_RESUME))
 		vdbg_printk(TPACPI_DBG_RFKILL,
 			"WWAN power down on resume request failed\n");
+	printk("sus\n");
+	acpi_evalf(hkey_handle, &status, "GWAN", "d");
+        printk("s%d stat.\n", status);
+}
+
+static int wan_resume(pm_message_t state)
+{
+	int status;
+	printk("res\n");
+	acpi_evalf(hkey_handle, &status, "GWAN", "d");
+	printk("r%d stat.\n", status);
+        printk("l%d stat.\n", laststat);
+	if(laststat & TP_ACPI_WANCARD_RADIOSSW) {
+	acpi_evalf(hkey_handle, NULL, "SWAN", "vd", TP_ACPI_WANCARD_RADIOSSW);
+	acpi_evalf(hkey_handle, &status, "GWAN", "d");
+	printk("r%d stat.\n", status);
+	tpacpi_rfk_update_swstate_all();
+	}
 }
 
 static int wan_get_status(void)
@@ -4078,6 +4101,7 @@ static int wan_get_status(void)
 	if (!acpi_evalf(hkey_handle, &status, "GWAN", "d"))
 		return -EIO;
 
+	printk("gs: %d\n", status);
 	return ((status & TP_ACPI_WANCARD_RADIOSSW) != 0) ?
 			TPACPI_RFK_RADIO_ON : TPACPI_RFK_RADIO_OFF;
 }
@@ -4103,6 +4127,7 @@ static int wan_set_status(enum tpacpi_rfkill_state state)
 	else
 		status = 0;
 
+	printk("ss: %d\n", status);
 	if (!acpi_evalf(hkey_handle, NULL, "SWAN", "vd", status))
 		return -EIO;
 
@@ -4239,6 +4264,7 @@ static struct ibm_struct wan_driver_data = {
 	.read = wan_read,
 	.write = wan_write,
 	.exit = wan_exit,
+	.resume = wan_resume,
 	.suspend = wan_suspend,
 	.shutdown = wan_shutdown,
 };

Re: Thinkpad X200s

[Henrique de Moraes Holschuh]

On Fri, 27 Nov 2009, Ian Molton wrote:
> The problem seems to be the rfkill layer getting out of sync with ACPIs
> idea of the hardware state.

Hmm... did rfkill drop the suspend/resume support perchance?

> If the laptop is suspended with WWAN enabled, it will wake up with it
> disabled, requiring a toggle of the physical WWAN switch or a prod in
> /sys to get it going again.

Yes.  rfkill is suposed to do proper suspend/resume in the class level,
which would call back thinkpad-acpi and request the radio to be turned on.

> I wrote a little patch that "cures" this issue (attached), however it is
> far from perfect.

Indeed, and it is NAK'd.  Let's fix it properly :-)

> For example, even with this patch, when the side switch is used, it
> overrides the soft setting for the devices completely, so if for example
> the WWAN was off, then toggling the switch changes its rfkill state from
> 0 -> 2, and then from 2 -> 1

...

> It *appears* from the code that it should be possible to get the device
> to power up in the state it was last in, but I cant make mine do it.

Depends only on rfkill, and whatever is handling EV_SW SW_RFKILL_ALL in your
Linux distribution.  Sometime ago, you could give rfkill-input a parameter
to change its mode to do just what you want.  I don't know if that's still
possible.

So, at least for this part of the issue, you have to turn your eyes to
rfkill and userspace.

> I've also noticed that on recent kernels, I get an extra rfkillswitch
> for hci0 (bluetooth) in addition to the ACPI one. This vanishes if power
> is toggled using the ACPI rfkill softswitch, however bluetooth continues
> to work (when enabled).

Yes.  The thinkpad-acpi rfkill switch powers down the bluetooth hardware,
thus it disappears.

> +static int wan_resume(pm_message_t state)
> +{
> +	int status;
> +	printk("res\n");
> +	acpi_evalf(hkey_handle, &status, "GWAN", "d");
> +	printk("r%d stat.\n", status);
> +        printk("l%d stat.\n", laststat);
> +	if(laststat & TP_ACPI_WANCARD_RADIOSSW) {
> +	acpi_evalf(hkey_handle, NULL, "SWAN", "vd", TP_ACPI_WANCARD_RADIOSSW);
> +	acpi_evalf(hkey_handle, &status, "GWAN", "d");
> +	printk("r%d stat.\n", status);
> +	tpacpi_rfk_update_swstate_all();
> +	}

This is the right idea, done wrong.  I will fix it properly.  Just let me
check if it needs fixing in thinkpad-acpi or in rfkill, because if it is in
rfkill, they might have broken other drivers.

-- 
  "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: Thinkpad X200s

[Henrique de Moraes Holschuh]

On Fri, 27 Nov 2009, Henrique de Moraes Holschuh wrote:
> On Fri, 27 Nov 2009, Ian Molton wrote:
> > The problem seems to be the rfkill layer getting out of sync with ACPIs
> > idea of the hardware state.
> 
> Hmm... did rfkill drop the suspend/resume support perchance?

Yes, for persistent devices, which are thinkpad-acpi and eeepc-laptop.
I even ack'd the change that caused this regression, because I didn't
pay enough attention to the last lines of the patch. ARGH!

This can be worked-around by thinkpad-acpi with a few lines of extra
code, or fixed in rfkill removing two lines of code, which would allow
us to remove more LOC from eeepc-laptop as well, as they'd become
redundant.

It is a regression from 2.6.30, so it will have to go to stable anyway.
This means it is best to waste a few days checking with the others if it
would be better fixed in rfkill or thinkpad-acpi, and then get it in
2.6.32 and 2.6.31 through stable.

I will handle it, either 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

Re: Thinkpad X200s

[Ian Molton]

Henrique de Moraes Holschuh wrote:

> It is a regression from 2.6.30, so it will have to go to stable anyway.
> This means it is best to waste a few days checking with the others if it
> would be better fixed in rfkill or thinkpad-acpi, and then get it in
> 2.6.32 and 2.6.31 through stable.
> 
> I will handle it, either way.

Cheers - I'll keep an eye on this thread so I can test your patch ASAP! :-)

-Ian

Re: Thinkpad X200s

[Henrique de Moraes Holschuh]

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

On Sat, 28 Nov 2009, Ian Molton wrote:
> Henrique de Moraes Holschuh wrote:
> > It is a regression from 2.6.30, so it will have to go to stable anyway.
> > This means it is best to waste a few days checking with the others if it
> > would be better fixed in rfkill or thinkpad-acpi, and then get it in
> > 2.6.32 and 2.6.31 through stable.
> > 
> > I will handle it, either way.
> 
> Cheers - I'll keep an eye on this thread so I can test your patch ASAP! :-)

And I will _need_ your help.  Here is a tentative patch.  I can use a much
heavier hand than this, but if the firmware cooperates, this one should do
it (and it will be faster, to boot).

Please check if WWAN, bluetooth and if at all possible, UWB are doing the
right thing:

remain enabled if you suspend with them enabled.
remain disabled if you suspend with they disabled.

And in the case of WWAN and bluetooth, whether they retain their state
(enabled or disabled) across reboots and shutdown.

If you happen to find out that UWB also retains state across
reboot/shutdown, I'd like to know about it.

-- 
  "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

[-- Attachment #2: fix-tpacpi-rfk-resume.patch --]
[-- Type: text/x-diff, Size: 3264 bytes --]

commit 03dd8d5357d8fe2d91bff4ccdecd7e26f73e92e9
Author: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
Date:   Sat Nov 28 00:31:47 2009 -0200

    thinkpad-acpi: preserve rfkill state across suspend/resume
    
    Since the rfkill rework in 2.6.31, the driver is always resuming with
    the radios all in blocked state.  We used to resume with radios
    blocked, and then depend on the rfkill core to re-enable them.
    
    This doesn't work with the new rfkill core and thinkpad-acpi rfkill
    code for various reasons.
    
    Change thinkpad-acpi to ask the firmware to resume with the radios
    in the last state when possible, and preserve that state in the
    driver itself when that's not possible.
    
    This fixes a regression from 2.6.30.
    
    Reported-by: Ian Molton <ian.molton@collabora.co.uk>
    Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
    Cc: stable@kernel.org

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 915e054..4abde23 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -3877,15 +3877,6 @@ enum {
 
 #define TPACPI_RFK_BLUETOOTH_SW_NAME	"tpacpi_bluetooth_sw"
 
-static void bluetooth_suspend(pm_message_t state)
-{
-	/* Try to make sure radio will resume powered off */
-	if (!acpi_evalf(NULL, NULL, "\\BLTH", "vd",
-		   TP_ACPI_BLTH_PWR_OFF_ON_RESUME))
-		vdbg_printk(TPACPI_DBG_RFKILL,
-			"bluetooth power down on resume request failed\n");
-}
-
 static int bluetooth_get_status(void)
 {
 	int status;
@@ -3919,10 +3910,9 @@ static int bluetooth_set_status(enum tpacpi_rfkill_state state)
 #endif
 
 	/* We make sure to keep TP_ACPI_BLUETOOTH_RESUMECTRL off */
+	status = TP_ACPI_BLUETOOTH_RESUMECTRL;
 	if (state == TPACPI_RFK_RADIO_ON)
-		status = TP_ACPI_BLUETOOTH_RADIOSSW;
-	else
-		status = 0;
+		status |= TP_ACPI_BLUETOOTH_RADIOSSW;
 
 	if (!acpi_evalf(hkey_handle, NULL, "SBDC", "vd", status))
 		return -EIO;
@@ -4061,7 +4051,6 @@ static struct ibm_struct bluetooth_driver_data = {
 	.read = bluetooth_read,
 	.write = bluetooth_write,
 	.exit = bluetooth_exit,
-	.suspend = bluetooth_suspend,
 	.shutdown = bluetooth_shutdown,
 };
 
@@ -4079,15 +4068,6 @@ enum {
 
 #define TPACPI_RFK_WWAN_SW_NAME		"tpacpi_wwan_sw"
 
-static void wan_suspend(pm_message_t state)
-{
-	/* Try to make sure radio will resume powered off */
-	if (!acpi_evalf(NULL, NULL, "\\WGSV", "qvd",
-		   TP_ACPI_WGSV_PWR_OFF_ON_RESUME))
-		vdbg_printk(TPACPI_DBG_RFKILL,
-			"WWAN power down on resume request failed\n");
-}
-
 static int wan_get_status(void)
 {
 	int status;
@@ -4120,11 +4100,10 @@ static int wan_set_status(enum tpacpi_rfkill_state state)
 	}
 #endif
 
-	/* We make sure to keep TP_ACPI_WANCARD_RESUMECTRL off */
+	/* We make sure to set TP_ACPI_WANCARD_RESUMECTRL */
+	status = TP_ACPI_WANCARD_RESUMECTRL;
 	if (state == TPACPI_RFK_RADIO_ON)
-		status = TP_ACPI_WANCARD_RADIOSSW;
-	else
-		status = 0;
+		status |= TP_ACPI_WANCARD_RADIOSSW;
 
 	if (!acpi_evalf(hkey_handle, NULL, "SWAN", "vd", status))
 		return -EIO;
@@ -4262,7 +4241,6 @@ static struct ibm_struct wan_driver_data = {
 	.read = wan_read,
 	.write = wan_write,
 	.exit = wan_exit,
-	.suspend = wan_suspend,
 	.shutdown = wan_shutdown,
 };
 

Re: Thinkpad X200s

[Ian Molton]

Henrique de Moraes Holschuh wrote:

>> Cheers - I'll keep an eye on this thread so I can test your patch ASAP! :-)
> 
> And I will _need_ your help.  Here is a tentative patch.  I can use a much
> heavier hand than this, but if the firmware cooperates, this one should do
> it (and it will be faster, to boot).
> 
> Please check if WWAN, bluetooth and if at all possible, UWB are doing the
> right thing:
> 
> remain enabled if you suspend with them enabled.

Yes, and:

> remain disabled if you suspend with they disabled.

Yes   (bluetooth and WWAN)

*BUT* If I toggle the hard RF killswitch, they all forget themselves and
reset to 'on' rather than their last known state. IOW, they are
disabled, then unconditionally re-enabled.

> And in the case of WWAN and bluetooth, whether they retain their state
> (enabled or disabled) across reboots and shutdown.

Yes - although GNOME interferes with that later...

> If you happen to find out that UWB also retains state across
> reboot/shutdown, I'd like to know about it.

I know nothing about the X200s UWB - what module would I need to test?

Hope this helps,

-Ian

Re: Thinkpad X200s

[Henrique de Moraes Holschuh]

On Mon, 30 Nov 2009, Ian Molton wrote:
> Henrique de Moraes Holschuh wrote:
> > remain disabled if you suspend with they disabled.
> 
> Yes   (bluetooth and WWAN)
> 
> *BUT* If I toggle the hard RF killswitch, they all forget themselves and
> reset to 'on' rather than their last known state. IOW, they are
> disabled, then unconditionally re-enabled.

This might well be userspace or the rfkill core, if the EC queues any
events and deliver them when the thinkad wakes up, they would enable the
radios in the default rfkill config.

Unfortunately, that's not something I can test on my T43.  Still, even
if it is the firmware doing that, I think we could live with it.

That said, please play with the master_switch_mode parameter of the
rfkill module.  I personally cannot tolerate anything but
master_switch_mode=1, and I dare think it might solve your problem :)

I think I can consider this patch approved and add your tested-by?

> > If you happen to find out that UWB also retains state across
> > reboot/shutdown, I'd like to know about it.
> 
> I know nothing about the X200s UWB - what module would I need to test?

I don't know if your x200s has an UWB radio.  Does thinkpad-acpi tells
you it exists?  There is a CONFIG_UWB option which enables UWB and WUSB
support.

-- 
  "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: Thinkpad X200s

[Ian Molton]

Henrique de Moraes Holschuh wrote:

> That said, please play with the master_switch_mode parameter of the
> rfkill module.  I personally cannot tolerate anything but
> master_switch_mode=1, and I dare think it might solve your problem :)

Sadly not. It looks like it should, and I've instrumented the rfkill
module to be sure the parameter is set, which it is. Debug log (WWAN)
follows. Bluetooth behaves the same way as WWAN - powers up after
killswitch toggle, regardless of its state prior to that.

# cat /sys/module/rfkill/parameters/master_switch_mode
1

prior to sleep:

# find /sys | grep rfkill..name | xargs cat
tpacpi_bluetooth_sw
tpacpi_wwan_sw
hci0
phy0

# find /sys | grep rfkill..state | xargs cat
1
1
1
1

Turn off WWAN vis /sys:

# echo 0 > /sys/devices/platform/thinkpad_acpi/rfkill/rfkill1/state

Verify it is off:

# find /sys | grep rfkill..state | xargs cat
1
0
1
1

Flip rfkill (hard) switch to off:

corwyn:/home/collabora# find /sys | grep rfkill..state | xargs cat
2
2
2

note: hci0 goes missing when the hard switch is used. (presumably usb
power loss)

Flip rfkill (hard) switch back on again:

corwyn:/home/collabora# find /sys | grep rfkill..state | xargs cat
1
1
1
1

> I think I can consider this patch approved and add your tested-by?

You can certainly add my tested-by for the sleep/resume path patch.

Tested-by: Ian Molton <ian.molton@collabora.co.uk>

> Does thinkpad-acpi tells

acpi -V -s does not list any wireless devices at all. dmesg has a few
mentions of thinkpad-acpi but only the rfkillswitch as far as wireless
devices.

Thanks for your quick response too :-)

-Ian