UBUNTU: SAUCE: dell-laptop: fire SMI when toggling hardware killswitch
 

On Wed, Jun 9, 2010 at 3:44 PM, Keng-Yu Lin <keng-yu.lin@canonical.com> wrote:
> SRU Justification:
>
> Impact:
> A type of Dell Inspiron Mini does not turn off bluetooth physically when
> pressing F2 (rfkill hotkey).
>
> Fix:
> Current kernel code expects hardware to handle the hard-rfkill switching
> spontaneously and only notify rfkill subsystem with the status change.
> This patch makes kernel to explicitly fire a SMI to switch on/off
> rfkill devices if BIOS reports that the hardware switch is not supported.
>
> This patch depends on the following sauce patch in lucid kernel so is not going upstream:
> * * * *UBUNTU: SAUCE: dell-laptop: Store the HW switch status internally rather than requerying every time
>
> Testcase:
> I tested the patch on a Dell Mini 10. It works smoothly.
>
> --
> From b17359e833677297d4d5daf78e4d9dc460e51285 Mon Sep 17 00:00:00 2001
> From: Keng-Yu Lin <keng-yu.lin@canonical.com>
> Date: Thu, 3 Jun 2010 10:31:56 +0800
> Subject: [PATCH] UBUNTU: SAUCE: dell-laptop: fire SMI when toggling hardware killswitch
>
> When BIOS reports hardware switch is not supported, it does not turn on/off all
> devices spontaneously. So it is neccessary to fire an SMI to switch the rfkill
> status explicitly when toggling the hardware killswitch.
>
> BugLink: http://bugs.launchpad.net/bugs/590607
>
> Signed-off-by: Keng-Yu Lin <keng-yu.lin@canonical.com>
> ---
> *drivers/platform/x86/dell-laptop.c | * *4 +++-
> *1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 15d96a0..e54ab12 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -29,6 +29,7 @@
> *#define WLAN_SWITCH_MASK 0
> *#define BT_SWITCH_MASK 1
> *#define WWAN_SWITCH_MASK 2
> +#define HW_SWITCH_SUPPORT 3
> *#define HW_SWITCH_MASK 16
>
> */* This structure will be modified by the firmware when we enter
> @@ -240,7 +241,7 @@ static int dell_rfkill_set(void *data, bool blocked)
> * * * *int disable = blocked ? 1 : 0;
> * * * *unsigned long radio = (unsigned long)data;
>
> - * * * if (!(hw_switch_status & BIT(radio-1)) || !(hw_switch_status & BIT(HW_SWITCH_MASK))) {
> + * * * if (!(hw_switch_status & BIT(radio-1)) || !(hw_switch_status & BIT(HW_SWITCH_MASK)) || !(hw_switch_status & BIT(HW_SWITCH_SUPPORT))) {

Coding style perspective, this is too long. You can either wrap it
or introduce a static inline function, e.g. hw_support_switch() to
clean up the conditions here.

> * * * * * * * *memset(&buffer, 0, sizeof(struct calling_interface_buffer));
> * * * * * * * *buffer.input[0] = (1 | (radio<<8) | (disable << 16));
> * * * * * * * *dell_send_request(&buffer, 17, 11);
> @@ -258,6 +259,7 @@ static void dell_rfkill_query(struct rfkill *rfkill, void *data)
> * * * *dell_send_request(&buffer, 17, 11);
> * * * *status = buffer.output[1];
>
> + * * * hw_switch_status |= (status & BIT(0)) << BIT(HW_SWITCH_SUPPORT);
> * * * *hw_switch_status |= (status & BIT(HW_SWITCH_MASK)) ^ BIT(HW_SWITCH_MASK);
>

Patch looks good to me, and we should get this upstreamed as well.

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

UBUNTU: SAUCE: dell-laptop: fire SMI when toggling hardware killswitch
 

2010/6/9 Stefan Bader <stefan.bader@canonical.com>:
> The patch is small (Eric's comment about the line length should be taken care
> of) but I wonder a bit about other systems. This adds setting a bit without
> being obvious whether that get restricted to hw that requires it. What happens
> on hw that does not require this special handling?
> So I definitely want that to be coming back from upstream instead of SRU it
> manually. Make *sure you submit it as a stable patch, then we will get it
> automatically.
>
> -Stefan
>
> On 06/09/2010 09:44 AM, Keng-Yu Lin wrote:
>> SRU Justification:
>>
>> Impact:
>> A type of Dell Inspiron Mini does not turn off bluetooth physically when
>> pressing F2 (rfkill hotkey).
>>
>> Fix:
>> Current kernel code expects hardware to handle the hard-rfkill switching
>> spontaneously and only notify rfkill subsystem with the status change.
>> This patch makes kernel to explicitly fire a SMI to switch on/off
>> rfkill devices if BIOS reports that the hardware switch is not supported.
>>
>> This patch depends on the following sauce patch in lucid kernel so is not going upstream:
>> * * * UBUNTU: SAUCE: dell-laptop: Store the HW switch status internally rather than requerying every time
>>
>> Testcase:
>> I tested the patch on a Dell Mini 10. It works smoothly.
>>
>> --
>> From b17359e833677297d4d5daf78e4d9dc460e51285 Mon Sep 17 00:00:00 2001
>> From: Keng-Yu Lin <keng-yu.lin@canonical.com>
>> Date: Thu, 3 Jun 2010 10:31:56 +0800
>> Subject: [PATCH] UBUNTU: SAUCE: dell-laptop: fire SMI when toggling hardware killswitch
>>
>> When BIOS reports hardware switch is not supported, it does not turn on/off all
>> devices spontaneously. So it is neccessary to fire an SMI to switch the rfkill
>> status explicitly when toggling the hardware killswitch.
>>
>> BugLink: http://bugs.launchpad.net/bugs/590607
>>
>> Signed-off-by: Keng-Yu Lin <keng-yu.lin@canonical.com>
>> ---
>> *drivers/platform/x86/dell-laptop.c | * *4 +++-
>> *1 files changed, 3 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
>> index 15d96a0..e54ab12 100644
>> --- a/drivers/platform/x86/dell-laptop.c
>> +++ b/drivers/platform/x86/dell-laptop.c
>> @@ -29,6 +29,7 @@
>> *#define WLAN_SWITCH_MASK 0
>> *#define BT_SWITCH_MASK 1
>> *#define WWAN_SWITCH_MASK 2
>> +#define HW_SWITCH_SUPPORT 3
>> *#define HW_SWITCH_MASK 16
>>
>> */* This structure will be modified by the firmware when we enter
>> @@ -240,7 +241,7 @@ static int dell_rfkill_set(void *data, bool blocked)
>> * * * int disable = blocked ? 1 : 0;
>> * * * unsigned long radio = (unsigned long)data;
>>
>> - * * if (!(hw_switch_status & BIT(radio-1)) || !(hw_switch_status & BIT(HW_SWITCH_MASK))) {
>> + * * if (!(hw_switch_status & BIT(radio-1)) || !(hw_switch_status & BIT(HW_SWITCH_MASK)) || !(hw_switch_status & BIT(HW_SWITCH_SUPPORT))) {
>> * * * * * * * memset(&buffer, 0, sizeof(struct calling_interface_buffer));
>> * * * * * * * buffer.input[0] = (1 | (radio<<8) | (disable << 16));
>> * * * * * * * dell_send_request(&buffer, 17, 11);
>> @@ -258,6 +259,7 @@ static void dell_rfkill_query(struct rfkill *rfkill, void *data)
>> * * * dell_send_request(&buffer, 17, 11);
>> * * * status = buffer.output[1];
>>
>> + * * hw_switch_status |= (status & BIT(0)) << BIT(HW_SWITCH_SUPPORT);
>> * * * hw_switch_status |= (status & BIT(HW_SWITCH_MASK)) ^ BIT(HW_SWITCH_MASK);
>>
>> * * * /* HW switch control not supported
>
>

Hi Stefan:
This patch is restricted to a BIOS reporting that hardware switch is
not supported (BIT 0 off of status variable). If hardware does control
the rfkill, BIT 0 should be on, as I observed on Dell inspiron N3010
in hand. Then this patch changes no original behavior.
There are several sauce patches for dell-laptop.c in lucid tree. The
code difference between lucid tree and stable tree makes it difficult
to fix in a simple patch.
I will try to write a new patch for upstream (.35 kernel), but it
should be implemented in different ways.

Eric:
Thanks for mentioning the line length. should be careful of that.

Regards,
-kengyu

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team

UBUNTU: SAUCE: dell-laptop: fire SMI when toggling hardware killswitch
 

2010/6/9 Chase Douglas <chase.douglas@canonical.com>:
> On Wed, 2010-06-09 at 15:08 +0200, Stefan Bader wrote:
>> On 06/09/2010 12:05 PM, Keng-Yü Lin wrote:
>> > * There are several sauce patches for dell-laptop.c in lucid tree. The
>> > code difference between lucid tree and stable tree makes it difficult
>> > to fix in a simple patch.
>>
>> Ok, sorry I missed that note about the SAUCE patch in the initial request. But
>> it would bring up the question to me, why the other patch has not gone upstream.
>> Was it tried but rejected? If so, why?
>> Is the same patch not needed for Maverick? Is the other patch in there? This
>> sounds a bit like being simpler to forget things on the way forward.
>
> I think way back in the early lucid development some patches were
> applied to our tree that did not come from upstream. Matthew Garrett,
> the upstream maintainer, took the patches and reworked them some.
> However, the damage is done now, so we can't converge the two without a
> wholesale replacement of parts of the lucid driver.
>
> That said, we need to be sure that anything we put into Lucid is also
> pushed upstream in a form that is applicable to their sources. In fact,
> it may make the most sense to push changes to this driver upstream
> first, and then propose a reworked version for lucid once it is
> accepted.
>
> -- Chase
>
>

dug back to the history found that several of these SAUCE patches
exist since karmic. much the same discussion two month ago while Chase
sent a patch on the same driver. Some explanation in that thread by
Mario Limonciello:

"As I recall, during karmic we pulled a patch that didn't make it
upstream in the form we have it, and a bunch of those other patches
layered on top of it. I didn't have any type of notification that a
different form had finally landed until Matthew started pulling some
of the patches we had in too."

I tested maverick kernel (2.6.32-2.2) on the machine and this bug is
there. This sauce patch here does not fit in maverick or upstream. I
think I will work another patch for it without depending on the
previous sauce patch code.

agreed that it's a better process to fix upstream first and then gets
backported somehow. But I encountered this issue while working on some
enablement for lucid. This is why the patch for lucid comes first.


Regards,
-kengyu

--
kernel-team mailing list
kernel-team@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/kernel-team