Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Darren Hart]

On Sun, Oct 18, 2015 at 09:34:56AM +0000, bugzilla-daemon@bugzilla.kernel.org wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=106031
> 
> --- Comment #2 from Britt Yazel <bwyazel@gmail.com> ---
> Confirmed. Blacklisting the dell_rbtn module solves this issue

Pali,

Can you take a look at this bug report regarding a 4.2 regression for rfkill
after the introduction of the dell-rbtn driver please.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Pali Rohár]

On Wednesday 21 October 2015 10:57:24 Darren Hart wrote:
> On Sun, Oct 18, 2015 at 09:34:56AM +0000, bugzilla-daemon@bugzilla.kernel.org wrote:
> > https://bugzilla.kernel.org/show_bug.cgi?id=106031
> > 
> > --- Comment #2 from Britt Yazel <bwyazel@gmail.com> ---
> > Confirmed. Blacklisting the dell_rbtn module solves this issue
> 
> Pali,
> 
> Can you take a look at this bug report regarding a 4.2 regression for rfkill
> after the introduction of the dell-rbtn driver please.
> 

Hi! This looks strange. Driver dell_rbnt is just receiver of BIOS/ACPI
events. It receive hotkey or slide switch event and translate it into
either linux input keypress or rfkill block event. But dell_rbnt driver
itself cannot set or change wireless rfkill or airplane mode.

So my opinion is that BIOS/ACPI could send such event, dell_rbnt then
propagate it into userspace and some application process it and do
something...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Pali Rohár]

On Wednesday 21 October 2015 11:19:54 Pali Rohár wrote:
> On Wednesday 21 October 2015 10:57:24 Darren Hart wrote:
> > On Sun, Oct 18, 2015 at 09:34:56AM +0000, bugzilla-daemon@bugzilla.kernel.org wrote:
> > > https://bugzilla.kernel.org/show_bug.cgi?id=106031
> > > 
> > > --- Comment #2 from Britt Yazel <bwyazel@gmail.com> ---
> > > Confirmed. Blacklisting the dell_rbtn module solves this issue
> > 
> > Pali,
> > 
> > Can you take a look at this bug report regarding a 4.2 regression for rfkill
> > after the introduction of the dell-rbtn driver please.
> > 
> 
> Hi! This looks strange. Driver dell_rbnt is just receiver of BIOS/ACPI
> events. It receive hotkey or slide switch event and translate it into
> either linux input keypress or rfkill block event. But dell_rbnt driver
> itself cannot set or change wireless rfkill or airplane mode.
> 
> So my opinion is that BIOS/ACPI could send such event, dell_rbnt then
> propagate it into userspace and some application process it and do
> something...
> 

CCing Gabriele, owner of XPS machine too. Have you seen similar problems
with dell_rbtn as described in above bug report?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Darren Hart]

On Wed, Oct 21, 2015 at 01:00:59PM +0200, Pali Rohár wrote:
> On Wednesday 21 October 2015 11:19:54 Pali Rohár wrote:
> > On Wednesday 21 October 2015 10:57:24 Darren Hart wrote:
> > > On Sun, Oct 18, 2015 at 09:34:56AM +0000, bugzilla-daemon@bugzilla.kernel.org wrote:
> > > > https://bugzilla.kernel.org/show_bug.cgi?id=106031
> > > > 
> > > > --- Comment #2 from Britt Yazel <bwyazel@gmail.com> ---
> > > > Confirmed. Blacklisting the dell_rbtn module solves this issue
> > > 
> > > Pali,
> > > 
> > > Can you take a look at this bug report regarding a 4.2 regression for rfkill
> > > after the introduction of the dell-rbtn driver please.
> > > 
> > 
> > Hi! This looks strange. Driver dell_rbnt is just receiver of BIOS/ACPI
> > events. It receive hotkey or slide switch event and translate it into
> > either linux input keypress or rfkill block event. But dell_rbnt driver
> > itself cannot set or change wireless rfkill or airplane mode.
> > 
> > So my opinion is that BIOS/ACPI could send such event, dell_rbnt then
> > propagate it into userspace and some application process it and do
> > something...
> > 
> 
> CCing Gabriele, owner of XPS machine too. Have you seen similar problems
> with dell_rbtn as described in above bug report?

Here's some context I've gathered. I haven't drawn any conclusions from this,
but perhaps you will find it useful:

From what I can tell, the XPS 13 has a TOGGLE type rfkill button (Fn-PrtScrn):

http://www.trustedreviews.com/dell-xps-13-2015-photos-11

I see in the journal that systemd is working with two rfkill devices (8 and 9),
and that dell-wmi is emitting unknown key events (e00e). This key is of course
not in the dell_wmi_legacy_keymap, but it would land right after 0xe00d, labeled
as a BIOS error.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Gabriele Mazzotta]

2015-10-21 13:12 GMT+02:00 Darren Hart <dvhart@infradead.org>:
> On Wed, Oct 21, 2015 at 01:00:59PM +0200, Pali Rohár wrote:
>> On Wednesday 21 October 2015 11:19:54 Pali Rohár wrote:
>> > On Wednesday 21 October 2015 10:57:24 Darren Hart wrote:
>> > > On Sun, Oct 18, 2015 at 09:34:56AM +0000, bugzilla-daemon@bugzilla.kernel.org wrote:
>> > > > https://bugzilla.kernel.org/show_bug.cgi?id=106031
>> > > >
>> > > > --- Comment #2 from Britt Yazel <bwyazel@gmail.com> ---
>> > > > Confirmed. Blacklisting the dell_rbtn module solves this issue
>> > >
>> > > Pali,
>> > >
>> > > Can you take a look at this bug report regarding a 4.2 regression for rfkill
>> > > after the introduction of the dell-rbtn driver please.
>> > >
>> >
>> > Hi! This looks strange. Driver dell_rbnt is just receiver of BIOS/ACPI
>> > events. It receive hotkey or slide switch event and translate it into
>> > either linux input keypress or rfkill block event. But dell_rbnt driver
>> > itself cannot set or change wireless rfkill or airplane mode.
>> >
>> > So my opinion is that BIOS/ACPI could send such event, dell_rbnt then
>> > propagate it into userspace and some application process it and do
>> > something...
>> >
>>
>> CCing Gabriele, owner of XPS machine too. Have you seen similar problems
>> with dell_rbtn as described in above bug report?

I'll look into this ASAP. I haven't been using dell_rbtn not to disable
the hardware switch, but I didn't notice anything strange the last time
I tested it.

> Here's some context I've gathered. I haven't drawn any conclusions from this,
> but perhaps you will find it useful:
>
> From what I can tell, the XPS 13 has a TOGGLE type rfkill button (Fn-PrtScrn):
>
> http://www.trustedreviews.com/dell-xps-13-2015-photos-11
>
> I see in the journal that systemd is working with two rfkill devices (8 and 9),
> and that dell-wmi is emitting unknown key events (e00e). This key is of course
> not in the dell_wmi_legacy_keymap, but it would land right after 0xe00d, labeled
> as a BIOS error.

e00e is some battery related event that is sent when some ACPI methods
are executed. I know it's sent upon resume. I thought about adding it
to the ignore list, but then I completely forgot. I'm not sure it has
the same meaning on all the laptops, but I don't think it's something
we care about.

> --
> Darren Hart
> Intel Open Source Technology Center

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Gabriele Mazzotta]

2015-10-21 13:42 GMT+02:00 Gabriele Mazzotta <gabriele.mzt@gmail.com>:
> 2015-10-21 13:12 GMT+02:00 Darren Hart <dvhart@infradead.org>:
>> On Wed, Oct 21, 2015 at 01:00:59PM +0200, Pali Rohár wrote:
>>> On Wednesday 21 October 2015 11:19:54 Pali Rohár wrote:
>>> > On Wednesday 21 October 2015 10:57:24 Darren Hart wrote:
>>> > > On Sun, Oct 18, 2015 at 09:34:56AM +0000, bugzilla-daemon@bugzilla.kernel.org wrote:
>>> > > > https://bugzilla.kernel.org/show_bug.cgi?id=106031
>>> > > >
>>> > > > --- Comment #2 from Britt Yazel <bwyazel@gmail.com> ---
>>> > > > Confirmed. Blacklisting the dell_rbtn module solves this issue
>>> > >
>>> > > Pali,
>>> > >
>>> > > Can you take a look at this bug report regarding a 4.2 regression for rfkill
>>> > > after the introduction of the dell-rbtn driver please.
>>> > >
>>> >
>>> > Hi! This looks strange. Driver dell_rbnt is just receiver of BIOS/ACPI
>>> > events. It receive hotkey or slide switch event and translate it into
>>> > either linux input keypress or rfkill block event. But dell_rbnt driver
>>> > itself cannot set or change wireless rfkill or airplane mode.
>>> >
>>> > So my opinion is that BIOS/ACPI could send such event, dell_rbnt then
>>> > propagate it into userspace and some application process it and do
>>> > something...
>>> >
>>>
>>> CCing Gabriele, owner of XPS machine too. Have you seen similar problems
>>> with dell_rbtn as described in above bug report?
>
> I'll look into this ASAP. I haven't been using dell_rbtn not to disable
> the hardware switch, but I didn't notice anything strange the last time
> I tested it.

I went back to see the old thread about dell-rbtn and apparently I was
aware of this problem [1]. So yes, I confirm the bug.

We are assuming that RBTN receives notifications only when a button
function keys are pressed, but this is true only for some laptops, while
others, such as my XPS13, send a notification also on resume [2].
This makes things a bit more complicated.

CCed Alex since he might be interested in this.

[1] https://marc.info/?l=linux-kernel&m=141927583032180
[2] https://marc.info/?l=linux-kernel&m=141941243829713

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Darren Hart]

On Wed, Oct 21, 2015 at 08:53:36PM +0200, Gabriele Mazzotta wrote:
> 2015-10-21 13:42 GMT+02:00 Gabriele Mazzotta <gabriele.mzt@gmail.com>:
> > 2015-10-21 13:12 GMT+02:00 Darren Hart <dvhart@infradead.org>:
> >> On Wed, Oct 21, 2015 at 01:00:59PM +0200, Pali Rohár wrote:
> >>> On Wednesday 21 October 2015 11:19:54 Pali Rohár wrote:
> >>> > On Wednesday 21 October 2015 10:57:24 Darren Hart wrote:
> >>> > > On Sun, Oct 18, 2015 at 09:34:56AM +0000, bugzilla-daemon@bugzilla.kernel.org wrote:
> >>> > > > https://bugzilla.kernel.org/show_bug.cgi?id=106031
> >>> > > >
> >>> > > > --- Comment #2 from Britt Yazel <bwyazel@gmail.com> ---
> >>> > > > Confirmed. Blacklisting the dell_rbtn module solves this issue
> >>> > >
> >>> > > Pali,
> >>> > >
> >>> > > Can you take a look at this bug report regarding a 4.2 regression for rfkill
> >>> > > after the introduction of the dell-rbtn driver please.
> >>> > >
> >>> >
> >>> > Hi! This looks strange. Driver dell_rbnt is just receiver of BIOS/ACPI
> >>> > events. It receive hotkey or slide switch event and translate it into
> >>> > either linux input keypress or rfkill block event. But dell_rbnt driver
> >>> > itself cannot set or change wireless rfkill or airplane mode.
> >>> >
> >>> > So my opinion is that BIOS/ACPI could send such event, dell_rbnt then
> >>> > propagate it into userspace and some application process it and do
> >>> > something...
> >>> >
> >>>
> >>> CCing Gabriele, owner of XPS machine too. Have you seen similar problems
> >>> with dell_rbtn as described in above bug report?
> >
> > I'll look into this ASAP. I haven't been using dell_rbtn not to disable
> > the hardware switch, but I didn't notice anything strange the last time
> > I tested it.
> 
> I went back to see the old thread about dell-rbtn and apparently I was
> aware of this problem [1]. So yes, I confirm the bug.
> 
> We are assuming that RBTN receives notifications only when a button
> function keys are pressed, but this is true only for some laptops, while
> others, such as my XPS13, send a notification also on resume [2].
> This makes things a bit more complicated.
> 
> CCed Alex since he might be interested in this.
> 
> [1] https://marc.info/?l=linux-kernel&m=141927583032180
> [2] https://marc.info/?l=linux-kernel&m=141941243829713
> 

Am I correct in my understanding that there is no EC state associated with the
RFKILL for the TOGGLE type?

Is there any point in sending the event upon resume? If not, perhaps we could
set a flag on suspend and then ignore the event on resume and then clear the
flag. This would have to be only for the affected latops though, which is
certainly not ideal.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Gabriele Mazzotta]

2015-10-22 9:49 GMT+02:00 Darren Hart <dvhart@infradead.org>:
> On Wed, Oct 21, 2015 at 08:53:36PM +0200, Gabriele Mazzotta wrote:
>> 2015-10-21 13:42 GMT+02:00 Gabriele Mazzotta <gabriele.mzt@gmail.com>:
>> > 2015-10-21 13:12 GMT+02:00 Darren Hart <dvhart@infradead.org>:
>> >> On Wed, Oct 21, 2015 at 01:00:59PM +0200, Pali Rohár wrote:
>> >>> On Wednesday 21 October 2015 11:19:54 Pali Rohár wrote:
>> >>> > On Wednesday 21 October 2015 10:57:24 Darren Hart wrote:
>> >>> > > On Sun, Oct 18, 2015 at 09:34:56AM +0000, bugzilla-daemon@bugzilla.kernel.org wrote:
>> >>> > > > https://bugzilla.kernel.org/show_bug.cgi?id=106031
>> >>> > > >
>> >>> > > > --- Comment #2 from Britt Yazel <bwyazel@gmail.com> ---
>> >>> > > > Confirmed. Blacklisting the dell_rbtn module solves this issue
>> >>> > >
>> >>> > > Pali,
>> >>> > >
>> >>> > > Can you take a look at this bug report regarding a 4.2 regression for rfkill
>> >>> > > after the introduction of the dell-rbtn driver please.
>> >>> > >
>> >>> >
>> >>> > Hi! This looks strange. Driver dell_rbnt is just receiver of BIOS/ACPI
>> >>> > events. It receive hotkey or slide switch event and translate it into
>> >>> > either linux input keypress or rfkill block event. But dell_rbnt driver
>> >>> > itself cannot set or change wireless rfkill or airplane mode.
>> >>> >
>> >>> > So my opinion is that BIOS/ACPI could send such event, dell_rbnt then
>> >>> > propagate it into userspace and some application process it and do
>> >>> > something...
>> >>> >
>> >>>
>> >>> CCing Gabriele, owner of XPS machine too. Have you seen similar problems
>> >>> with dell_rbtn as described in above bug report?
>> >
>> > I'll look into this ASAP. I haven't been using dell_rbtn not to disable
>> > the hardware switch, but I didn't notice anything strange the last time
>> > I tested it.
>>
>> I went back to see the old thread about dell-rbtn and apparently I was
>> aware of this problem [1]. So yes, I confirm the bug.
>>
>> We are assuming that RBTN receives notifications only when a button
>> function keys are pressed, but this is true only for some laptops, while
>> others, such as my XPS13, send a notification also on resume [2].
>> This makes things a bit more complicated.
>>
>> CCed Alex since he might be interested in this.
>>
>> [1] https://marc.info/?l=linux-kernel&m=141927583032180
>> [2] https://marc.info/?l=linux-kernel&m=141941243829713
>>
>
> Am I correct in my understanding that there is no EC state associated with the
> RFKILL for the TOGGLE type?

As far as I know, yes.

> Is there any point in sending the event upon resume? If not, perhaps we could
> set a flag on suspend and then ignore the event on resume and then clear the
> flag. This would have to be only for the affected latops though, which is
> certainly not ideal.

The problem is that we don't know which are the affected laptops. I
suspect they are those that support both the methods to disable radio
devices (software and hardware), i.e. those with a working ARBT method,
such as mine.

So this solution wouldn't really work properly, unless we define some
time constraints, which makes the solution even worse.

I'll see if I can find a batter way to deal with this problem,
dell-laptop can detect the presence of an hardware switch.

> --
> Darren Hart
> Intel Open Source Technology Center

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Pali Rohár]

On Thursday 22 October 2015 10:26:47 Gabriele Mazzotta wrote:
> I'll see if I can find a batter way to deal with this problem,
> dell-laptop can detect the presence of an hardware switch.

dell-rbtn.ko has acpi method CRBT which returns if notebook has hw switch or
toggle key.

And rfkill implementation in dell-laptop.ko does not work correctly on
XPS machines. And because userspace implementation of smbios works fine,
I think that problem is in kernel driver rather in BIOS/firmware...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Gabriele Mazzotta]

On 22/10/2015 10:51, Pali Rohár wrote:
> On Thursday 22 October 2015 10:26:47 Gabriele Mazzotta wrote:
>> I'll see if I can find a batter way to deal with this problem,
>> dell-laptop can detect the presence of an hardware switch.
>
> dell-rbtn.ko has acpi method CRBT which returns if notebook has hw switch or
> toggle key.
>
> And rfkill implementation in dell-laptop.ko does not work correctly on
> XPS machines. And because userspace implementation of smbios works fine,
> I think that problem is in kernel driver rather in BIOS/firmware...

I was thinking about something such as the following, which should favor
the hardware slider when available. This should work if my assumption is
correct, that is the laptops with a working RBTN method are the ones
having problems.

diff --git a/drivers/platform/x86/dell-rbtn.c 
b/drivers/platform/x86/dell-rbtn.c
index cd410e3..cf3c11c 100644
--- a/drivers/platform/x86/dell-rbtn.c
+++ b/drivers/platform/x86/dell-rbtn.c
@@ -321,6 +321,7 @@ static int rbtn_add(struct acpi_device *device)
  	struct rbtn_data *rbtn_data;
  	enum rbtn_type type;
  	int ret = 0;
+	bool has_hardware_slider; /* get this with SMI */

  	type = rbtn_check(device);
  	if (type == RBTN_UNKNOWN) {
@@ -328,7 +329,7 @@ static int rbtn_add(struct acpi_device *device)
  		return -EINVAL;
  	}

-	ret = rbtn_acquire(device, true);
+	ret = rbtn_acquire(device, !has_hardware_slider);
  	if (ret < 0) {
  		dev_err(&device->dev, "Cannot enable device\n");
  		return ret;
@@ -343,7 +344,10 @@ static int rbtn_add(struct acpi_device *device)

  	switch (rbtn_data->type) {
  	case RBTN_TOGGLE:
-		ret = rbtn_input_init(rbtn_data);
+		if (has_hardware_slider)
+			ret = 0;
+		else
+			ret = rbtn_input_init(rbtn_data);
  		break;
  	case RBTN_SLIDER:
  		if (auto_remove_rfkill && rbtn_chain_head.head)

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Pali Rohár]

On Thursday 22 October 2015 12:44:08 Gabriele Mazzotta wrote:
> On 22/10/2015 10:51, Pali Rohár wrote:
> >On Thursday 22 October 2015 10:26:47 Gabriele Mazzotta wrote:
> >>I'll see if I can find a batter way to deal with this problem,
> >>dell-laptop can detect the presence of an hardware switch.
> >
> >dell-rbtn.ko has acpi method CRBT which returns if notebook has hw switch or
> >toggle key.
> >
> >And rfkill implementation in dell-laptop.ko does not work correctly on
> >XPS machines. And because userspace implementation of smbios works fine,
> >I think that problem is in kernel driver rather in BIOS/firmware...
> 
> I was thinking about something such as the following, which should favor
> the hardware slider when available. This should work if my assumption is
> correct, that is the laptops with a working RBTN method are the ones
> having problems.
> 

Hm... wait! There are machines with HW slider and type is RBTN_TOGGLE?

I thought that all machines with HW slider has type RBTN_SLIDER and
others have RBTN_TOGGLE.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Gabriele Mazzotta]

On 22/10/2015 12:50, Pali Rohár wrote:
> On Thursday 22 October 2015 12:44:08 Gabriele Mazzotta wrote:
>> On 22/10/2015 10:51, Pali Rohár wrote:
>>> On Thursday 22 October 2015 10:26:47 Gabriele Mazzotta wrote:
>>>> I'll see if I can find a batter way to deal with this problem,
>>>> dell-laptop can detect the presence of an hardware switch.
>>>
>>> dell-rbtn.ko has acpi method CRBT which returns if notebook has hw switch or
>>> toggle key.
>>>
>>> And rfkill implementation in dell-laptop.ko does not work correctly on
>>> XPS machines. And because userspace implementation of smbios works fine,
>>> I think that problem is in kernel driver rather in BIOS/firmware...
>>
>> I was thinking about something such as the following, which should favor
>> the hardware slider when available. This should work if my assumption is
>> correct, that is the laptops with a working RBTN method are the ones
>> having problems.
>>
>
> Hm... wait! There are machines with HW slider and type is RBTN_TOGGLE?
>
> I thought that all machines with HW slider has type RBTN_SLIDER and
> others have RBTN_TOGGLE.

Yes, this is the problem I've been talking about ever since the work
on this driver started. AFAIK we don't know how to detect these laptops
(my XPS13 is one of them).

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Darren Hart]

On Thu, Oct 22, 2015 at 12:54:33PM +0200, Gabriele Mazzotta wrote:
> On 22/10/2015 12:50, Pali Rohár wrote:
> >On Thursday 22 October 2015 12:44:08 Gabriele Mazzotta wrote:
> >>On 22/10/2015 10:51, Pali Rohár wrote:
> >>>On Thursday 22 October 2015 10:26:47 Gabriele Mazzotta wrote:
> >>>>I'll see if I can find a batter way to deal with this problem,
> >>>>dell-laptop can detect the presence of an hardware switch.
> >>>
> >>>dell-rbtn.ko has acpi method CRBT which returns if notebook has hw switch or
> >>>toggle key.
> >>>
> >>>And rfkill implementation in dell-laptop.ko does not work correctly on
> >>>XPS machines. And because userspace implementation of smbios works fine,
> >>>I think that problem is in kernel driver rather in BIOS/firmware...
> >>
> >>I was thinking about something such as the following, which should favor
> >>the hardware slider when available. This should work if my assumption is
> >>correct, that is the laptops with a working RBTN method are the ones
> >>having problems.
> >>
> >
> >Hm... wait! There are machines with HW slider and type is RBTN_TOGGLE?
> >
> >I thought that all machines with HW slider has type RBTN_SLIDER and
> >others have RBTN_TOGGLE.
> 
> Yes, this is the problem I've been talking about ever since the work
> on this driver started. AFAIK we don't know how to detect these laptops
> (my XPS13 is one of them).

Where is the radio hw slider on this machine?

This patch would effectively ignore Fn-F2 (radio toggle) key on this machine
then - correct? I don't think that is desirable either.

Seems to me the desired behavior would be to restore the radio state on resume.
Consider the following table:

SW: Switch state
SS: State at suspend
SR: State after resume
 0: WiFi Enabled
 1: WiFi Disabled

State SW SS SR
--------------
a      0  0  0
b      0  1  1   (switch is enabled, but toggle key disabled wifi)
c      1  0  N/A (invalid state)
d      1  1  1

State "a" is where we are failing currently I believe?

Do we know if DELRBTN and DELLABCE are always a TOGGLE or a SLIDER respectively?
I'm wondering if these should be separate drivers.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Gabriele Mazzotta]

On 22/10/2015 15:02, Darren Hart wrote:
> On Thu, Oct 22, 2015 at 12:54:33PM +0200, Gabriele Mazzotta wrote:
>> On 22/10/2015 12:50, Pali Rohár wrote:
>>> On Thursday 22 October 2015 12:44:08 Gabriele Mazzotta wrote:
>>>> On 22/10/2015 10:51, Pali Rohár wrote:
>>>>> On Thursday 22 October 2015 10:26:47 Gabriele Mazzotta wrote:
>>>>>> I'll see if I can find a batter way to deal with this problem,
>>>>>> dell-laptop can detect the presence of an hardware switch.
>>>>>
>>>>> dell-rbtn.ko has acpi method CRBT which returns if notebook has hw switch or
>>>>> toggle key.
>>>>>
>>>>> And rfkill implementation in dell-laptop.ko does not work correctly on
>>>>> XPS machines. And because userspace implementation of smbios works fine,
>>>>> I think that problem is in kernel driver rather in BIOS/firmware...
>>>>
>>>> I was thinking about something such as the following, which should favor
>>>> the hardware slider when available. This should work if my assumption is
>>>> correct, that is the laptops with a working RBTN method are the ones
>>>> having problems.
>>>>
>>>
>>> Hm... wait! There are machines with HW slider and type is RBTN_TOGGLE?
>>>
>>> I thought that all machines with HW slider has type RBTN_SLIDER and
>>> others have RBTN_TOGGLE.
>>
>> Yes, this is the problem I've been talking about ever since the work
>> on this driver started. AFAIK we don't know how to detect these laptops
>> (my XPS13 is one of them).
>
> Where is the radio hw slider on this machine?

It's the F2 key. Depending on the value passed to the RBTN, it acts as
hw slider or sw toggle. Currently dell-rbtn makes all the laptops with
both the hw slider and sw toggle use the sw toggle. In this way we are
sure that whenever CRBT returns either 1 or 0, we are using the sw
mechanism to disable the radios.

The problem is that the CRBT method of these laptops supporting both
the methods return 1 or 0 as the laptops that only have the sw toggle.
That's why we are calling RBTN to force the sw toggle.

> This patch would effectively ignore Fn-F2 (radio toggle) key on this machine
> then - correct? I don't think that is desirable either.

No, this patch would force the hw slider when it's available instead
of blindly forcing the sw toggle. When the hw slider is in use,
everything is handled by the BIOS, so userspace/kernel don't have to
do anything.

> Seems to me the desired behavior would be to restore the radio state on resume.
> Consider the following table:
>
> SW: Switch state
> SS: State at suspend
> SR: State after resume
>   0: WiFi Enabled
>   1: WiFi Disabled
>
> State SW SS SR
> --------------
> a      0  0  0
> b      0  1  1   (switch is enabled, but toggle key disabled wifi)
> c      1  0  N/A (invalid state)
> d      1  1  1
>
> State "a" is where we are failing currently I believe?

dell-rbtn is simply listening to the ACPI notifications sent to
DELLABCE. These notifications are sent by the BIOS, both when users
press the function key and when it wants. In my case, the BIOS sends a
notification on resume (dell-rbtn sees this notification and sends an
input event to userspace), but the BIOS of some other laptops don't do
this.

Basically, dell-rbtn makes some laptop toggles the state of WiFi on
resume, but not all of them.

In my case:
if WiFi is ON on suspend, it's turned OFF on resume.
if WiFi is OFF on suspend, it's turned ON on resume.

Reading what Alex wrote some time ago [1], I guessed that the laptops 
that are misbehaving are those that can choose between sw toggle and hw
toggle.

> Do we know if DELRBTN and DELLABCE are always a TOGGLE or a SLIDER respectively?
> I'm wondering if these should be separate drivers.

I'm reading the acpidumps Alex provided some time ago [2] (and mine):
* XPS13 9333: DELLABCE, by default (without calling RBTN) SLIDER.
* Latitude E5440: DELLABCE, SLIDER
* Inspiron 7447: DELLABCE, TOGGLE
* Pali's laptop: DELLRBTN, SLIDER (correct?).

[1] https://marc.info/?l=linux-kernel&m=141941243829713
[2] http://people.canonical.com/~alexhung/dell-acpidump/

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Pali Rohár]

On Thursday 22 October 2015 15:43:46 Gabriele Mazzotta wrote:
> It's the F2 key. Depending on the value passed to the RBTN, it acts as
> hw slider or sw toggle.

Ok, we have differences in terminology and everybody understood this
problem differently.


To correct this situation, first define about what we are talking about:

* HW slider: It is hardware slider switch which as two positions ON and
             OFF. Position exactly defines hardware state of wireless
             radio devices. Not possible (or should not) to remap.

* SW hotkey toggle button: It is button or key, act in same way as any
                           other key on keyboard, controlled by SW
                           (if all drivers are installed, etc).


Next I think that this hypothesis is truth:

* Every machine on which is binded ACPI dell-rbtn.ko driver has either
  HW slider or SW toggle. Not both!

Correct?


Then follows my expected behaviour for HW slider:

* State of HW slider position is exported by kernel as rfkill device.

* In any case HW slider position (ON/OFF) match hard rfkill state device
  (rfkill device state is correct also after resume, wake from hibernate).

* Immediately after user change position of HW slider, rfkill device
  reflect it.


And then expected behaviour for SW toggle button:

* Every time when user press it, kernel just send input event "wireless
  key pressed" to userspace.

* Kernel does not send event "wireless key pressed" when user did not
  pressed it (e.g. after resuming from suspend, waking from hibernate).

* Kernel should provide rfkill devices to "soft" block appropriate
  wireless cards.

* Make sure that BIOS/firmware in any case does not change radio rfkill
  state of any wireless card and wireless cards stay in same state as
  before (no enable/disable or changing hard/soft rfkill state).

If last sentence is not truth, then kernel must not send "wireless key
pressed" event to userspace and act like "no key was pressed".


Make this sense? Or are there any objections about this behaviour?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Gabriele Mazzotta]

On 22/10/2015 16:17, Pali Rohár wrote:
> On Thursday 22 October 2015 15:43:46 Gabriele Mazzotta wrote:
>> It's the F2 key. Depending on the value passed to the RBTN, it acts as
>> hw slider or sw toggle.
>
> Ok, we have differences in terminology and everybody understood this
> problem differently.
>
>
> To correct this situation, first define about what we are talking about:
>
> * HW slider: It is hardware slider switch which as two positions ON and
>               OFF. Position exactly defines hardware state of wireless
>               radio devices. Not possible (or should not) to remap.
>
> * SW hotkey toggle button: It is button or key, act in same way as any
>                             other key on keyboard, controlled by SW
>                             (if all drivers are installed, etc).

Sorry for the confusion, I know I've been using "HW slider" improperly,
but I thought it was clear. Although it's a button, the BIOS makes it
behave like an hardware slider when configured to behave as such
through RBTN. The state of radio devices is changed by the BIOS when
the function key is pressed, this state is never changed until the user
presses the function key again (at least if we ignore the fact that we
can perform some special SMI calls), this state is saved in the EC and
both userspace and the kernel don't receive any event other than rfkill
events (if we don't consider the WMI events ignored by dell-wmi).

I think this behavior is closer to the one of an hardware slider
rather than the one of a software hotkey, which I can still request
through the RBTN method.

> Next I think that this hypothesis is truth:
>
> * Every machine on which is binded ACPI dell-rbtn.ko driver has either
>    HW slider or SW toggle. Not both!
>
> Correct?

It depends on how you classify my laptop. The behavior changes
completely depending on the value passed to RBTN.

Obviously it can behave either as HW slider (again, not an actual
slider) or SW toggle, but I can choose between the two.

> Then follows my expected behaviour for HW slider:
>
> * State of HW slider position is exported by kernel as rfkill device.
>
> * In any case HW slider position (ON/OFF) match hard rfkill state device
>    (rfkill device state is correct also after resume, wake from hibernate).
>
> * Immediately after user change position of HW slider, rfkill device
>    reflect it.
>
>
> And then expected behaviour for SW toggle button:
>
> * Every time when user press it, kernel just send input event "wireless
>    key pressed" to userspace.
>
> * Kernel does not send event "wireless key pressed" when user did not
>    pressed it (e.g. after resuming from suspend, waking from hibernate).

Yes, but we don't know exactly when the user pressed the button. As I
said, the BIOS might send an event that triggers an input event even if
the user didn't press anything.

> * Kernel should provide rfkill devices to "soft" block appropriate
>    wireless cards.
>
> * Make sure that BIOS/firmware in any case does not change radio rfkill
>    state of any wireless card and wireless cards stay in same state as
>    before (no enable/disable or changing hard/soft rfkill state).

This is the problem. We can't differentiate notifications sent to
DELLABCE/DELLRBTN by the BIOS because it felt like it was right to do so
(e.g. after resuming) or because the user pressed the function key.

> If last sentence is not truth, then kernel must not send "wireless key
> pressed" event to userspace and act like "no key was pressed".
>
>
> Make this sense? Or are there any objections about this behaviour?
>

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Pali Rohár]

On Friday 23 October 2015 01:29:12 Gabriele Mazzotta wrote:
> On 22/10/2015 16:17, Pali Rohár wrote:
> >On Thursday 22 October 2015 15:43:46 Gabriele Mazzotta wrote:
> >>It's the F2 key. Depending on the value passed to the RBTN, it acts as
> >>hw slider or sw toggle.
> >
> >Ok, we have differences in terminology and everybody understood this
> >problem differently.
> >
> >
> >To correct this situation, first define about what we are talking about:
> >
> >* HW slider: It is hardware slider switch which as two positions ON and
> >              OFF. Position exactly defines hardware state of wireless
> >              radio devices. Not possible (or should not) to remap.
> >
> >* SW hotkey toggle button: It is button or key, act in same way as any
> >                            other key on keyboard, controlled by SW
> >                            (if all drivers are installed, etc).
> 
> Sorry for the confusion, I know I've been using "HW slider" improperly,
> but I thought it was clear. Although it's a button, the BIOS makes it
> behave like an hardware slider when configured to behave as such
> through RBTN. The state of radio devices is changed by the BIOS when
> the function key is pressed, this state is never changed until the user
> presses the function key again (at least if we ignore the fact that we
> can perform some special SMI calls), this state is saved in the EC and
> both userspace and the kernel don't receive any event other than rfkill
> events (if we don't consider the WMI events ignored by dell-wmi).
> 
> I think this behavior is closer to the one of an hardware slider
> rather than the one of a software hotkey, which I can still request
> through the RBTN method.
> 
> >Next I think that this hypothesis is truth:
> >
> >* Every machine on which is binded ACPI dell-rbtn.ko driver has either
> >   HW slider or SW toggle. Not both!
> >
> >Correct?
> 
> It depends on how you classify my laptop. The behavior changes
> completely depending on the value passed to RBTN.
> 
> Obviously it can behave either as HW slider (again, not an actual
> slider) or SW toggle, but I can choose between the two.
> 

If it has keyboard button and not switch with visible two positions, it
is "toggle button" (from HW perspective).

> >Then follows my expected behaviour for HW slider:
> >
> >* State of HW slider position is exported by kernel as rfkill device.
> >
> >* In any case HW slider position (ON/OFF) match hard rfkill state device
> >   (rfkill device state is correct also after resume, wake from hibernate).
> >
> >* Immediately after user change position of HW slider, rfkill device
> >   reflect it.
> >
> >
> >And then expected behaviour for SW toggle button:
> >
> >* Every time when user press it, kernel just send input event "wireless
> >   key pressed" to userspace.
> >
> >* Kernel does not send event "wireless key pressed" when user did not
> >   pressed it (e.g. after resuming from suspend, waking from hibernate).
> 
> Yes, but we don't know exactly when the user pressed the button. As I
> said, the BIOS might send an event that triggers an input event even if
> the user didn't press anything.
> 

Ok. This is strange if BIOS send event "changed" even if it was not
really changed.

Is BIOS sending this incorrect event only after waking from suspend? Or
it is also randomly at any time?

If we know that it send incorrectly only after suspend, we can drop this
event. But if it is completely randomly, we probably cannot do anything
(and just do not use driver in this case).

> >* Kernel should provide rfkill devices to "soft" block appropriate
> >   wireless cards.
> >
> >* Make sure that BIOS/firmware in any case does not change radio rfkill
> >   state of any wireless card and wireless cards stay in same state as
> >   before (no enable/disable or changing hard/soft rfkill state).
> 
> This is the problem. We can't differentiate notifications sent to
> DELLABCE/DELLRBTN by the BIOS because it felt like it was right to do so
> (e.g. after resuming) or because the user pressed the function key.
> 

In my opinion it is better to ignore user key press after resume, if it
fix our problem. Better as false-positive event.

> >If last sentence is not truth, then kernel must not send "wireless key
> >pressed" event to userspace and act like "no key was pressed".
> >
> >
> >Make this sense? Or are there any objections about this behaviour?
> >

So if you are OK with above my description (from previous email), then
we should fix dell-rbtn to act as it (if it is not implemented already
and correctly).

So my next question is: Does dell-rbtn behave like my description? If
not what are differences and what needs to be fixed?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Gabriele Mazzotta]

On 23/10/2015 11:00, Pali Rohár wrote:
> On Friday 23 October 2015 01:29:12 Gabriele Mazzotta wrote:
>> On 22/10/2015 16:17, Pali Rohár wrote:
>>> On Thursday 22 October 2015 15:43:46 Gabriele Mazzotta wrote:
>>>> It's the F2 key. Depending on the value passed to the RBTN, it acts as
>>>> hw slider or sw toggle.
>>>
>>> Ok, we have differences in terminology and everybody understood this
>>> problem differently.
>>>
>>>
>>> To correct this situation, first define about what we are talking about:
>>>
>>> * HW slider: It is hardware slider switch which as two positions ON and
>>>               OFF. Position exactly defines hardware state of wireless
>>>               radio devices. Not possible (or should not) to remap.
>>>
>>> * SW hotkey toggle button: It is button or key, act in same way as any
>>>                             other key on keyboard, controlled by SW
>>>                             (if all drivers are installed, etc).
>>
>> Sorry for the confusion, I know I've been using "HW slider" improperly,
>> but I thought it was clear. Although it's a button, the BIOS makes it
>> behave like an hardware slider when configured to behave as such
>> through RBTN. The state of radio devices is changed by the BIOS when
>> the function key is pressed, this state is never changed until the user
>> presses the function key again (at least if we ignore the fact that we
>> can perform some special SMI calls), this state is saved in the EC and
>> both userspace and the kernel don't receive any event other than rfkill
>> events (if we don't consider the WMI events ignored by dell-wmi).
>>
>> I think this behavior is closer to the one of an hardware slider
>> rather than the one of a software hotkey, which I can still request
>> through the RBTN method.
>>
>>> Next I think that this hypothesis is truth:
>>>
>>> * Every machine on which is binded ACPI dell-rbtn.ko driver has either
>>>    HW slider or SW toggle. Not both!
>>>
>>> Correct?
>>
>> It depends on how you classify my laptop. The behavior changes
>> completely depending on the value passed to RBTN.
>>
>> Obviously it can behave either as HW slider (again, not an actual
>> slider) or SW toggle, but I can choose between the two.
>>
>
> If it has keyboard button and not switch with visible two positions, it
> is "toggle button" (from HW perspective).

Yes, sorry for the confusion.

>>> Then follows my expected behaviour for HW slider:
>>>
>>> * State of HW slider position is exported by kernel as rfkill device.
>>>
>>> * In any case HW slider position (ON/OFF) match hard rfkill state device
>>>    (rfkill device state is correct also after resume, wake from hibernate).
>>>
>>> * Immediately after user change position of HW slider, rfkill device
>>>    reflect it.
>>>
>>>
>>> And then expected behaviour for SW toggle button:
>>>
>>> * Every time when user press it, kernel just send input event "wireless
>>>    key pressed" to userspace.
>>>
>>> * Kernel does not send event "wireless key pressed" when user did not
>>>    pressed it (e.g. after resuming from suspend, waking from hibernate).
>>
>> Yes, but we don't know exactly when the user pressed the button. As I
>> said, the BIOS might send an event that triggers an input event even if
>> the user didn't press anything.
>>
>
> Ok. This is strange if BIOS send event "changed" even if it was not
> really changed.
>
> Is BIOS sending this incorrect event only after waking from suspend? Or
> it is also randomly at any time?

I looked at the ACPI table. It happens only on resume (or when the user
presses the function key).

> If we know that it send incorrectly only after suspend, we can drop this
> event. But if it is completely randomly, we probably cannot do anything
> (and just do not use driver in this case).
>
>>> * Kernel should provide rfkill devices to "soft" block appropriate
>>>    wireless cards.
>>>
>>> * Make sure that BIOS/firmware in any case does not change radio rfkill
>>>    state of any wireless card and wireless cards stay in same state as
>>>    before (no enable/disable or changing hard/soft rfkill state).
>>
>> This is the problem. We can't differentiate notifications sent to
>> DELLABCE/DELLRBTN by the BIOS because it felt like it was right to do so
>> (e.g. after resuming) or because the user pressed the function key.
>>
>
> In my opinion it is better to ignore user key press after resume, if it
> fix our problem. Better as false-positive event.

The following appears to work really well. The notification arrives
before rbtn_resume() has been executed, so the extra event is ignored.

diff --git a/drivers/platform/x86/dell-rbtn.c 
b/drivers/platform/x86/dell-rbtn.c
index cd410e3..1d64b72 100644
--- a/drivers/platform/x86/dell-rbtn.c
+++ b/drivers/platform/x86/dell-rbtn.c
@@ -28,6 +28,7 @@ struct rbtn_data {
  	enum rbtn_type type;
  	struct rfkill *rfkill;
  	struct input_dev *input_dev;
+	bool suspended;
  };


@@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
  	{ "", 0 },
  };

+#ifdef CONFIG_PM_SLEEP
+static int rbtn_suspend(struct device *dev)
+{
+	struct acpi_device *device = to_acpi_device(dev);
+	struct rbtn_data *rbtn_data = acpi_driver_data(device);
+
+	rbtn_data->suspended = true;
+
+	return 0;
+}
+
+static int rbtn_resume(struct device *dev)
+{
+	struct acpi_device *device = to_acpi_device(dev);
+	struct rbtn_data *rbtn_data = acpi_driver_data(device);
+
+	rbtn_data->suspended = false;
+
+	return 0;
+}
+#endif
+static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
+
  static struct acpi_driver rbtn_driver = {
  	.name = "dell-rbtn",
  	.ids = rbtn_ids,
+	.drv.pm = &rbtn_pm_ops,
  	.ops = {
  		.add = rbtn_add,
  		.remove = rbtn_remove,
@@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, 
u32 event)
  {
  	struct rbtn_data *rbtn_data = device->driver_data;

+	if (rbtn_data->suspended)
+		return;
+
  	if (event != 0x80) {
  		dev_info(&device->dev, "Received unknown event (0x%x)\n",
  			 event);

>>> If last sentence is not truth, then kernel must not send "wireless key
>>> pressed" event to userspace and act like "no key was pressed".
>>>
>>>
>>> Make this sense? Or are there any objections about this behaviour?
>>>
>
> So if you are OK with above my description (from previous email), then
> we should fix dell-rbtn to act as it (if it is not implemented already
> and correctly).
>
> So my next question is: Does dell-rbtn behave like my description? If
> not what are differences and what needs to be fixed?

Everything works as expected. We only have to deal with this extra
notification.

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Pali Rohár]

On Friday 23 October 2015 11:47:25 Gabriele Mazzotta wrote:
> >In my opinion it is better to ignore user key press after resume, if it
> >fix our problem. Better as false-positive event.
> 
> The following appears to work really well. The notification arrives
> before rbtn_resume() has been executed, so the extra event is ignored.
> 
> diff --git a/drivers/platform/x86/dell-rbtn.c
> b/drivers/platform/x86/dell-rbtn.c
> index cd410e3..1d64b72 100644
> --- a/drivers/platform/x86/dell-rbtn.c
> +++ b/drivers/platform/x86/dell-rbtn.c
> @@ -28,6 +28,7 @@ struct rbtn_data {
>  	enum rbtn_type type;
>  	struct rfkill *rfkill;
>  	struct input_dev *input_dev;
> +	bool suspended;
>  };
> 
> 
> @@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
>  	{ "", 0 },
>  };
> 
> +#ifdef CONFIG_PM_SLEEP
> +static int rbtn_suspend(struct device *dev)
> +{
> +	struct acpi_device *device = to_acpi_device(dev);
> +	struct rbtn_data *rbtn_data = acpi_driver_data(device);
> +
> +	rbtn_data->suspended = true;
> +
> +	return 0;
> +}
> +
> +static int rbtn_resume(struct device *dev)
> +{
> +	struct acpi_device *device = to_acpi_device(dev);
> +	struct rbtn_data *rbtn_data = acpi_driver_data(device);
> +
> +	rbtn_data->suspended = false;
> +
> +	return 0;
> +}
> +#endif
> +static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
> +
>  static struct acpi_driver rbtn_driver = {
>  	.name = "dell-rbtn",
>  	.ids = rbtn_ids,
> +	.drv.pm = &rbtn_pm_ops,
>  	.ops = {
>  		.add = rbtn_add,
>  		.remove = rbtn_remove,
> @@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, u32
> event)
>  {
>  	struct rbtn_data *rbtn_data = device->driver_data;
> 
> +	if (rbtn_data->suspended)
> +		return;
> +
>  	if (event != 0x80) {
>  		dev_info(&device->dev, "Received unknown event (0x%x)\n",
>  			 event);
> 

Great, but is not there a better way to turn off .notify ACPI function
when that ACPI device is suspended?

Is not this ACPI device driver bug that it allows to call .notify method
even if device is suspended?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Gabriele Mazzotta]

On 23/10/2015 13:14, Pali Rohár wrote:
> On Friday 23 October 2015 11:47:25 Gabriele Mazzotta wrote:
>>> In my opinion it is better to ignore user key press after resume, if it
>>> fix our problem. Better as false-positive event.
>>
>> The following appears to work really well. The notification arrives
>> before rbtn_resume() has been executed, so the extra event is ignored.
>>
>> diff --git a/drivers/platform/x86/dell-rbtn.c
>> b/drivers/platform/x86/dell-rbtn.c
>> index cd410e3..1d64b72 100644
>> --- a/drivers/platform/x86/dell-rbtn.c
>> +++ b/drivers/platform/x86/dell-rbtn.c
>> @@ -28,6 +28,7 @@ struct rbtn_data {
>>   	enum rbtn_type type;
>>   	struct rfkill *rfkill;
>>   	struct input_dev *input_dev;
>> +	bool suspended;
>>   };
>>
>>
>> @@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
>>   	{ "", 0 },
>>   };
>>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int rbtn_suspend(struct device *dev)
>> +{
>> +	struct acpi_device *device = to_acpi_device(dev);
>> +	struct rbtn_data *rbtn_data = acpi_driver_data(device);
>> +
>> +	rbtn_data->suspended = true;
>> +
>> +	return 0;
>> +}
>> +
>> +static int rbtn_resume(struct device *dev)
>> +{
>> +	struct acpi_device *device = to_acpi_device(dev);
>> +	struct rbtn_data *rbtn_data = acpi_driver_data(device);
>> +
>> +	rbtn_data->suspended = false;
>> +
>> +	return 0;
>> +}
>> +#endif
>> +static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
>> +
>>   static struct acpi_driver rbtn_driver = {
>>   	.name = "dell-rbtn",
>>   	.ids = rbtn_ids,
>> +	.drv.pm = &rbtn_pm_ops,
>>   	.ops = {
>>   		.add = rbtn_add,
>>   		.remove = rbtn_remove,
>> @@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, u32
>> event)
>>   {
>>   	struct rbtn_data *rbtn_data = device->driver_data;
>>
>> +	if (rbtn_data->suspended)
>> +		return;
>> +
>>   	if (event != 0x80) {
>>   		dev_info(&device->dev, "Received unknown event (0x%x)\n",
>>   			 event);
>>
>
> Great, but is not there a better way to turn off .notify ACPI function
> when that ACPI device is suspended?
>
> Is not this ACPI device driver bug that it allows to call .notify method
> even if device is suspended?

I was surprised this worked, I was assuming that nothing could run
before the resume callback, but I was wrong. I think it makes sense to
treat ACPI devices in a special way, but I really don't know, we need
someone more knowledgeable to answer these questions. However, while I
was trying to figure things out, I stumbled upon the following:
e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend events").

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Darren Hart]

On Fri, Oct 23, 2015 at 08:03:19PM +0200, Gabriele Mazzotta wrote:
> On 23/10/2015 13:14, Pali Rohár wrote:
> >On Friday 23 October 2015 11:47:25 Gabriele Mazzotta wrote:
> >>>In my opinion it is better to ignore user key press after resume, if it
> >>>fix our problem. Better as false-positive event.
> >>
> >>The following appears to work really well. The notification arrives
> >>before rbtn_resume() has been executed, so the extra event is ignored.
> >>
> >>diff --git a/drivers/platform/x86/dell-rbtn.c
> >>b/drivers/platform/x86/dell-rbtn.c
> >>index cd410e3..1d64b72 100644
> >>--- a/drivers/platform/x86/dell-rbtn.c
> >>+++ b/drivers/platform/x86/dell-rbtn.c
> >>@@ -28,6 +28,7 @@ struct rbtn_data {
> >>  	enum rbtn_type type;
> >>  	struct rfkill *rfkill;
> >>  	struct input_dev *input_dev;
> >>+	bool suspended;
> >>  };
> >>
> >>
> >>@@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
> >>  	{ "", 0 },
> >>  };
> >>
> >>+#ifdef CONFIG_PM_SLEEP
> >>+static int rbtn_suspend(struct device *dev)
> >>+{
> >>+	struct acpi_device *device = to_acpi_device(dev);
> >>+	struct rbtn_data *rbtn_data = acpi_driver_data(device);
> >>+
> >>+	rbtn_data->suspended = true;
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static int rbtn_resume(struct device *dev)
> >>+{
> >>+	struct acpi_device *device = to_acpi_device(dev);
> >>+	struct rbtn_data *rbtn_data = acpi_driver_data(device);
> >>+
> >>+	rbtn_data->suspended = false;
> >>+
> >>+	return 0;
> >>+}
> >>+#endif
> >>+static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
> >>+
> >>  static struct acpi_driver rbtn_driver = {
> >>  	.name = "dell-rbtn",
> >>  	.ids = rbtn_ids,
> >>+	.drv.pm = &rbtn_pm_ops,
> >>  	.ops = {
> >>  		.add = rbtn_add,
> >>  		.remove = rbtn_remove,
> >>@@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, u32
> >>event)
> >>  {
> >>  	struct rbtn_data *rbtn_data = device->driver_data;
> >>
> >>+	if (rbtn_data->suspended)
> >>+		return;
> >>+
> >>  	if (event != 0x80) {
> >>  		dev_info(&device->dev, "Received unknown event (0x%x)\n",
> >>  			 event);
> >>
> >
> >Great, but is not there a better way to turn off .notify ACPI function
> >when that ACPI device is suspended?
> >
> >Is not this ACPI device driver bug that it allows to call .notify method
> >even if device is suspended?
> 
> I was surprised this worked, I was assuming that nothing could run
> before the resume callback, but I was wrong. I think it makes sense to
> treat ACPI devices in a special way, but I really don't know, we need
> someone more knowledgeable to answer these questions. However, while I
> was trying to figure things out, I stumbled upon the following:
> e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend events").
> 

My understanding here though is that laptops with a Fn+RBTN key which changes
the state of the radio in firmware should be handled via the rfkill interface
rather than the inpu interface - so while this patch may appear to work, it's
using the input interface to copy the rfkill interface/state.

The proper solution, if I'm understanding this correctly - and apologies if not,
some of this is new territory for me as well - would be for this device to be
detected as firmware controlled (what we refer to as a SLIDER in the code -
which needs to be renamed IMHO).

Let's sort out this point, then we can pull in Rafael to make sure this is how
to best deal with the spurious event on resume.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Pali Rohár]

On Monday 26 October 2015 23:38:13 Darren Hart wrote:
> My understanding here though is that laptops with a Fn+RBTN key which changes
> the state of the radio in firmware should be handled via the rfkill interface
> rather than the inpu interface - so while this patch may appear to work, it's
> using the input interface to copy the rfkill interface/state.
> 

Here is problem: ACPI device ABCE/RBTN (handled by dell-rbtn.ko) just
receive events. It cannot set or change wireless state.

For changing rfkill/wireless state is there Dell SMBIOS api and it is
implemented in dell-laptop.ko. But due to bugs in that module, it is
disabled for XPS machines.

And problem is that on machines without HW switch you do not know if
wifi switch is in ON or OFF mode. ACPI tell you just "key pressed". So
for this reason Alex decided to export that event via input layer,
because it is really just key press, not changing state.

> The proper solution, if I'm understanding this correctly - and apologies if not,
> some of this is new territory for me as well - would be for this device to be
> detected as firmware controlled (what we refer to as a SLIDER in the code -
> which needs to be renamed IMHO).
> 

I understood that if you blacklist dell-rbtn.ko on that XPS machine,
then firmware takes control and automatically turn ON/OFF wifi card.

> Let's sort out this point, then we can pull in Rafael to make sure this is how
> to best deal with the spurious event on resume.
> 

For me solution (=ignore events when ACPI device is suspended) sounds
good. I do not know if there is better way to implement it, maybe
general linux device model should provide function "am_i_suspended?"
instead tracking "suspend" state internally in each driver. But if such
support in linux device or acpi model is not implemented, I'm fine with
provided patch if it really works.

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Pali Rohár]

On Friday 23 October 2015 20:03:19 Gabriele Mazzotta wrote:
> On 23/10/2015 13:14, Pali Rohár wrote:
> >On Friday 23 October 2015 11:47:25 Gabriele Mazzotta wrote:
> >>>In my opinion it is better to ignore user key press after resume, if it
> >>>fix our problem. Better as false-positive event.
> >>
> >>The following appears to work really well. The notification arrives
> >>before rbtn_resume() has been executed, so the extra event is ignored.
> >>
> >>diff --git a/drivers/platform/x86/dell-rbtn.c
> >>b/drivers/platform/x86/dell-rbtn.c
> >>index cd410e3..1d64b72 100644
> >>--- a/drivers/platform/x86/dell-rbtn.c
> >>+++ b/drivers/platform/x86/dell-rbtn.c
> >>@@ -28,6 +28,7 @@ struct rbtn_data {
> >>  	enum rbtn_type type;
> >>  	struct rfkill *rfkill;
> >>  	struct input_dev *input_dev;
> >>+	bool suspended;
> >>  };
> >>
> >>
> >>@@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
> >>  	{ "", 0 },
> >>  };
> >>
> >>+#ifdef CONFIG_PM_SLEEP
> >>+static int rbtn_suspend(struct device *dev)
> >>+{
> >>+	struct acpi_device *device = to_acpi_device(dev);
> >>+	struct rbtn_data *rbtn_data = acpi_driver_data(device);
> >>+
> >>+	rbtn_data->suspended = true;
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static int rbtn_resume(struct device *dev)
> >>+{
> >>+	struct acpi_device *device = to_acpi_device(dev);
> >>+	struct rbtn_data *rbtn_data = acpi_driver_data(device);
> >>+
> >>+	rbtn_data->suspended = false;
> >>+
> >>+	return 0;
> >>+}
> >>+#endif
> >>+static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
> >>+
> >>  static struct acpi_driver rbtn_driver = {
> >>  	.name = "dell-rbtn",
> >>  	.ids = rbtn_ids,
> >>+	.drv.pm = &rbtn_pm_ops,
> >>  	.ops = {
> >>  		.add = rbtn_add,
> >>  		.remove = rbtn_remove,
> >>@@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, u32
> >>event)
> >>  {
> >>  	struct rbtn_data *rbtn_data = device->driver_data;
> >>
> >>+	if (rbtn_data->suspended)
> >>+		return;
> >>+
> >>  	if (event != 0x80) {
> >>  		dev_info(&device->dev, "Received unknown event (0x%x)\n",
> >>  			 event);
> >>
> >
> >Great, but is not there a better way to turn off .notify ACPI function
> >when that ACPI device is suspended?
> >
> >Is not this ACPI device driver bug that it allows to call .notify method
> >even if device is suspended?
> 
> I was surprised this worked, I was assuming that nothing could run
> before the resume callback, but I was wrong. I think it makes sense to
> treat ACPI devices in a special way, but I really don't know, we need
> someone more knowledgeable to answer these questions. However, while I
> was trying to figure things out, I stumbled upon the following:
> e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend events").

Gabriele, are you going to send this patch?

I think that patch should be OK as it drop events when device is in
suspend state (when it should not receive events)...

Darren, what do you think about it?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Darren Hart]

On Fri, Nov 20, 2015 at 03:44:25PM +0100, Pali Rohár wrote:
> On Friday 23 October 2015 20:03:19 Gabriele Mazzotta wrote:
> > On 23/10/2015 13:14, Pali Rohár wrote:
> > >On Friday 23 October 2015 11:47:25 Gabriele Mazzotta wrote:
> > >>>In my opinion it is better to ignore user key press after resume, if it
> > >>>fix our problem. Better as false-positive event.
> > >>
> > >>The following appears to work really well. The notification arrives
> > >>before rbtn_resume() has been executed, so the extra event is ignored.
> > >>
> > >>diff --git a/drivers/platform/x86/dell-rbtn.c
> > >>b/drivers/platform/x86/dell-rbtn.c
> > >>index cd410e3..1d64b72 100644
> > >>--- a/drivers/platform/x86/dell-rbtn.c
> > >>+++ b/drivers/platform/x86/dell-rbtn.c
> > >>@@ -28,6 +28,7 @@ struct rbtn_data {
> > >>  	enum rbtn_type type;
> > >>  	struct rfkill *rfkill;
> > >>  	struct input_dev *input_dev;
> > >>+	bool suspended;
> > >>  };
> > >>
> > >>
> > >>@@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
> > >>  	{ "", 0 },
> > >>  };
> > >>
> > >>+#ifdef CONFIG_PM_SLEEP
> > >>+static int rbtn_suspend(struct device *dev)
> > >>+{
> > >>+	struct acpi_device *device = to_acpi_device(dev);
> > >>+	struct rbtn_data *rbtn_data = acpi_driver_data(device);
> > >>+
> > >>+	rbtn_data->suspended = true;
> > >>+
> > >>+	return 0;
> > >>+}
> > >>+
> > >>+static int rbtn_resume(struct device *dev)
> > >>+{
> > >>+	struct acpi_device *device = to_acpi_device(dev);
> > >>+	struct rbtn_data *rbtn_data = acpi_driver_data(device);
> > >>+
> > >>+	rbtn_data->suspended = false;
> > >>+
> > >>+	return 0;
> > >>+}
> > >>+#endif
> > >>+static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
> > >>+
> > >>  static struct acpi_driver rbtn_driver = {
> > >>  	.name = "dell-rbtn",
> > >>  	.ids = rbtn_ids,
> > >>+	.drv.pm = &rbtn_pm_ops,
> > >>  	.ops = {
> > >>  		.add = rbtn_add,
> > >>  		.remove = rbtn_remove,
> > >>@@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, u32
> > >>event)
> > >>  {
> > >>  	struct rbtn_data *rbtn_data = device->driver_data;
> > >>
> > >>+	if (rbtn_data->suspended)
> > >>+		return;
> > >>+
> > >>  	if (event != 0x80) {
> > >>  		dev_info(&device->dev, "Received unknown event (0x%x)\n",
> > >>  			 event);
> > >>
> > >
> > >Great, but is not there a better way to turn off .notify ACPI function
> > >when that ACPI device is suspended?
> > >
> > >Is not this ACPI device driver bug that it allows to call .notify method
> > >even if device is suspended?
> > 
> > I was surprised this worked, I was assuming that nothing could run
> > before the resume callback, but I was wrong. I think it makes sense to
> > treat ACPI devices in a special way, but I really don't know, we need
> > someone more knowledgeable to answer these questions. However, while I
> > was trying to figure things out, I stumbled upon the following:
> > e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend events").
> 
> Gabriele, are you going to send this patch?
> 
> I think that patch should be OK as it drop events when device is in
> suspend state (when it should not receive events)...
> 
> Darren, what do you think about it?
> 

Sorry, this one has been difficult for me to track, but it's clearly an issue,
and new systems are experiencing it as well.

I'd like to get Rafael's opinion on disabling .notify ACPI function while
suspended.

+Rafael

Has Dell been involved here?

+Jared


-- 
Darren Hart
Intel Open Source Technology Center

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Rafael J. Wysocki]

On Friday, December 18, 2015 04:12:08 PM Darren Hart wrote:
> On Fri, Nov 20, 2015 at 03:44:25PM +0100, Pali Rohár wrote:
> > On Friday 23 October 2015 20:03:19 Gabriele Mazzotta wrote:
> > > On 23/10/2015 13:14, Pali Rohár wrote:
> > > >On Friday 23 October 2015 11:47:25 Gabriele Mazzotta wrote:
> > > >>>In my opinion it is better to ignore user key press after resume, if it
> > > >>>fix our problem. Better as false-positive event.
> > > >>
> > > >>The following appears to work really well. The notification arrives
> > > >>before rbtn_resume() has been executed, so the extra event is ignored.
> > > >>
> > > >>diff --git a/drivers/platform/x86/dell-rbtn.c
> > > >>b/drivers/platform/x86/dell-rbtn.c
> > > >>index cd410e3..1d64b72 100644
> > > >>--- a/drivers/platform/x86/dell-rbtn.c
> > > >>+++ b/drivers/platform/x86/dell-rbtn.c
> > > >>@@ -28,6 +28,7 @@ struct rbtn_data {
> > > >>  	enum rbtn_type type;
> > > >>  	struct rfkill *rfkill;
> > > >>  	struct input_dev *input_dev;
> > > >>+	bool suspended;
> > > >>  };
> > > >>
> > > >>
> > > >>@@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
> > > >>  	{ "", 0 },
> > > >>  };
> > > >>
> > > >>+#ifdef CONFIG_PM_SLEEP
> > > >>+static int rbtn_suspend(struct device *dev)
> > > >>+{
> > > >>+	struct acpi_device *device = to_acpi_device(dev);
> > > >>+	struct rbtn_data *rbtn_data = acpi_driver_data(device);
> > > >>+
> > > >>+	rbtn_data->suspended = true;
> > > >>+
> > > >>+	return 0;
> > > >>+}
> > > >>+
> > > >>+static int rbtn_resume(struct device *dev)
> > > >>+{
> > > >>+	struct acpi_device *device = to_acpi_device(dev);
> > > >>+	struct rbtn_data *rbtn_data = acpi_driver_data(device);
> > > >>+
> > > >>+	rbtn_data->suspended = false;
> > > >>+
> > > >>+	return 0;
> > > >>+}
> > > >>+#endif
> > > >>+static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
> > > >>+
> > > >>  static struct acpi_driver rbtn_driver = {
> > > >>  	.name = "dell-rbtn",
> > > >>  	.ids = rbtn_ids,
> > > >>+	.drv.pm = &rbtn_pm_ops,
> > > >>  	.ops = {
> > > >>  		.add = rbtn_add,
> > > >>  		.remove = rbtn_remove,
> > > >>@@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, u32
> > > >>event)
> > > >>  {
> > > >>  	struct rbtn_data *rbtn_data = device->driver_data;
> > > >>
> > > >>+	if (rbtn_data->suspended)
> > > >>+		return;
> > > >>+
> > > >>  	if (event != 0x80) {
> > > >>  		dev_info(&device->dev, "Received unknown event (0x%x)\n",
> > > >>  			 event);
> > > >>
> > > >
> > > >Great, but is not there a better way to turn off .notify ACPI function
> > > >when that ACPI device is suspended?
> > > >
> > > >Is not this ACPI device driver bug that it allows to call .notify method
> > > >even if device is suspended?
> > > 
> > > I was surprised this worked, I was assuming that nothing could run
> > > before the resume callback, but I was wrong. I think it makes sense to
> > > treat ACPI devices in a special way, but I really don't know, we need
> > > someone more knowledgeable to answer these questions. However, while I
> > > was trying to figure things out, I stumbled upon the following:
> > > e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend events").
> > 
> > Gabriele, are you going to send this patch?
> > 
> > I think that patch should be OK as it drop events when device is in
> > suspend state (when it should not receive events)...
> > 
> > Darren, what do you think about it?
> > 
> 
> Sorry, this one has been difficult for me to track, but it's clearly an issue,
> and new systems are experiencing it as well.
> 
> I'd like to get Rafael's opinion on disabling .notify ACPI function while
> suspended.
> 
> +Rafael

This by far wouldn't be enough, because drivers may install ACPI notify
handlers by themselves and those are hooked up directly into the ACPICA
code.

Besides, some drivers may actually want to receive those events while
the system is suspending or resuming, so I think this has to be addressed
on a per-driver basis.

> Has Dell been involved here?

Not that I know of.

Thanks,
Rafael

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Gabriele Mazzotta]

2015-12-20 17:21 GMT+01:00 Rafael J. Wysocki <rjw@rjwysocki.net>:
> On Friday, December 18, 2015 04:12:08 PM Darren Hart wrote:
>> On Fri, Nov 20, 2015 at 03:44:25PM +0100, Pali Rohár wrote:
>> > On Friday 23 October 2015 20:03:19 Gabriele Mazzotta wrote:
>> > > On 23/10/2015 13:14, Pali Rohár wrote:
>> > > >On Friday 23 October 2015 11:47:25 Gabriele Mazzotta wrote:
>> > > >>>In my opinion it is better to ignore user key press after resume, if it
>> > > >>>fix our problem. Better as false-positive event.
>> > > >>
>> > > >>The following appears to work really well. The notification arrives
>> > > >>before rbtn_resume() has been executed, so the extra event is ignored.
>> > > >>
>> > > >>diff --git a/drivers/platform/x86/dell-rbtn.c
>> > > >>b/drivers/platform/x86/dell-rbtn.c
>> > > >>index cd410e3..1d64b72 100644
>> > > >>--- a/drivers/platform/x86/dell-rbtn.c
>> > > >>+++ b/drivers/platform/x86/dell-rbtn.c
>> > > >>@@ -28,6 +28,7 @@ struct rbtn_data {
>> > > >>        enum rbtn_type type;
>> > > >>        struct rfkill *rfkill;
>> > > >>        struct input_dev *input_dev;
>> > > >>+       bool suspended;
>> > > >>  };
>> > > >>
>> > > >>
>> > > >>@@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
>> > > >>        { "", 0 },
>> > > >>  };
>> > > >>
>> > > >>+#ifdef CONFIG_PM_SLEEP
>> > > >>+static int rbtn_suspend(struct device *dev)
>> > > >>+{
>> > > >>+       struct acpi_device *device = to_acpi_device(dev);
>> > > >>+       struct rbtn_data *rbtn_data = acpi_driver_data(device);
>> > > >>+
>> > > >>+       rbtn_data->suspended = true;
>> > > >>+
>> > > >>+       return 0;
>> > > >>+}
>> > > >>+
>> > > >>+static int rbtn_resume(struct device *dev)
>> > > >>+{
>> > > >>+       struct acpi_device *device = to_acpi_device(dev);
>> > > >>+       struct rbtn_data *rbtn_data = acpi_driver_data(device);
>> > > >>+
>> > > >>+       rbtn_data->suspended = false;
>> > > >>+
>> > > >>+       return 0;
>> > > >>+}
>> > > >>+#endif
>> > > >>+static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
>> > > >>+
>> > > >>  static struct acpi_driver rbtn_driver = {
>> > > >>        .name = "dell-rbtn",
>> > > >>        .ids = rbtn_ids,
>> > > >>+       .drv.pm = &rbtn_pm_ops,
>> > > >>        .ops = {
>> > > >>                .add = rbtn_add,
>> > > >>                .remove = rbtn_remove,
>> > > >>@@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, u32
>> > > >>event)
>> > > >>  {
>> > > >>        struct rbtn_data *rbtn_data = device->driver_data;
>> > > >>
>> > > >>+       if (rbtn_data->suspended)
>> > > >>+               return;
>> > > >>+
>> > > >>        if (event != 0x80) {
>> > > >>                dev_info(&device->dev, "Received unknown event (0x%x)\n",
>> > > >>                         event);
>> > > >>
>> > > >
>> > > >Great, but is not there a better way to turn off .notify ACPI function
>> > > >when that ACPI device is suspended?
>> > > >
>> > > >Is not this ACPI device driver bug that it allows to call .notify method
>> > > >even if device is suspended?
>> > >
>> > > I was surprised this worked, I was assuming that nothing could run
>> > > before the resume callback, but I was wrong. I think it makes sense to
>> > > treat ACPI devices in a special way, but I really don't know, we need
>> > > someone more knowledgeable to answer these questions. However, while I
>> > > was trying to figure things out, I stumbled upon the following:
>> > > e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend events").
>> >
>> > Gabriele, are you going to send this patch?
>> >
>> > I think that patch should be OK as it drop events when device is in
>> > suspend state (when it should not receive events)...
>> >
>> > Darren, what do you think about it?
>> >
>>
>> Sorry, this one has been difficult for me to track, but it's clearly an issue,
>> and new systems are experiencing it as well.
>>
>> I'd like to get Rafael's opinion on disabling .notify ACPI function while
>> suspended.
>>
>> +Rafael
>
> This by far wouldn't be enough, because drivers may install ACPI notify
> handlers by themselves and those are hooked up directly into the ACPICA
> code.
>
> Besides, some drivers may actually want to receive those events while
> the system is suspending or resuming, so I think this has to be addressed
> on a per-driver basis.

Hi,

sorry, but I'm not sure I understood everything, so I'll try to
briefly describe the problem and its current solution.

Currently dell-rbtn listens for the notifications sent to an ACPI
device and for notification sends an input event to userspace.

The problem we have is that some BIOSes send an extra notification
on resume and therefore we send an extra input event.

What we want to do is to ignore this ACPI notification without
affecting the systems whose BIOS does not send this extra
notification. We know that not all of them send this notification.

What I noticed is that the extra notification is issued by the _WAK
method and always arrives before dell-rbtn has been resumed, so
what I did is to add a flag that is set by the suspend and resume
callbacks of the device driver.

What we were wondering is whether this would be enough or we
need to do something different, e.g. ignore all the notifications that
arrived X seconds after the execution of the resume callback.

Thanks,
Gabriele

>> Has Dell been involved here?
>
> Not that I know of.
>
> Thanks,
> Rafael
>

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Rafael J. Wysocki]

On Monday, December 21, 2015 04:34:58 PM Gabriele Mazzotta wrote:
> 2015-12-20 17:21 GMT+01:00 Rafael J. Wysocki <rjw@rjwysocki.net>:
> > On Friday, December 18, 2015 04:12:08 PM Darren Hart wrote:
> >> On Fri, Nov 20, 2015 at 03:44:25PM +0100, Pali Rohár wrote:
> >> > On Friday 23 October 2015 20:03:19 Gabriele Mazzotta wrote:
> >> > > On 23/10/2015 13:14, Pali Rohár wrote:
> >> > > >On Friday 23 October 2015 11:47:25 Gabriele Mazzotta wrote:
> >> > > >>>In my opinion it is better to ignore user key press after resume, if it
> >> > > >>>fix our problem. Better as false-positive event.
> >> > > >>
> >> > > >>The following appears to work really well. The notification arrives
> >> > > >>before rbtn_resume() has been executed, so the extra event is ignored.
> >> > > >>
> >> > > >>diff --git a/drivers/platform/x86/dell-rbtn.c
> >> > > >>b/drivers/platform/x86/dell-rbtn.c
> >> > > >>index cd410e3..1d64b72 100644
> >> > > >>--- a/drivers/platform/x86/dell-rbtn.c
> >> > > >>+++ b/drivers/platform/x86/dell-rbtn.c
> >> > > >>@@ -28,6 +28,7 @@ struct rbtn_data {
> >> > > >>        enum rbtn_type type;
> >> > > >>        struct rfkill *rfkill;
> >> > > >>        struct input_dev *input_dev;
> >> > > >>+       bool suspended;
> >> > > >>  };
> >> > > >>
> >> > > >>
> >> > > >>@@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
> >> > > >>        { "", 0 },
> >> > > >>  };
> >> > > >>
> >> > > >>+#ifdef CONFIG_PM_SLEEP
> >> > > >>+static int rbtn_suspend(struct device *dev)
> >> > > >>+{
> >> > > >>+       struct acpi_device *device = to_acpi_device(dev);
> >> > > >>+       struct rbtn_data *rbtn_data = acpi_driver_data(device);
> >> > > >>+
> >> > > >>+       rbtn_data->suspended = true;
> >> > > >>+
> >> > > >>+       return 0;
> >> > > >>+}
> >> > > >>+
> >> > > >>+static int rbtn_resume(struct device *dev)
> >> > > >>+{
> >> > > >>+       struct acpi_device *device = to_acpi_device(dev);
> >> > > >>+       struct rbtn_data *rbtn_data = acpi_driver_data(device);
> >> > > >>+
> >> > > >>+       rbtn_data->suspended = false;
> >> > > >>+
> >> > > >>+       return 0;
> >> > > >>+}
> >> > > >>+#endif
> >> > > >>+static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
> >> > > >>+
> >> > > >>  static struct acpi_driver rbtn_driver = {
> >> > > >>        .name = "dell-rbtn",
> >> > > >>        .ids = rbtn_ids,
> >> > > >>+       .drv.pm = &rbtn_pm_ops,
> >> > > >>        .ops = {
> >> > > >>                .add = rbtn_add,
> >> > > >>                .remove = rbtn_remove,
> >> > > >>@@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, u32
> >> > > >>event)
> >> > > >>  {
> >> > > >>        struct rbtn_data *rbtn_data = device->driver_data;
> >> > > >>
> >> > > >>+       if (rbtn_data->suspended)
> >> > > >>+               return;
> >> > > >>+
> >> > > >>        if (event != 0x80) {
> >> > > >>                dev_info(&device->dev, "Received unknown event (0x%x)\n",
> >> > > >>                         event);
> >> > > >>
> >> > > >
> >> > > >Great, but is not there a better way to turn off .notify ACPI function
> >> > > >when that ACPI device is suspended?
> >> > > >
> >> > > >Is not this ACPI device driver bug that it allows to call .notify method
> >> > > >even if device is suspended?
> >> > >
> >> > > I was surprised this worked, I was assuming that nothing could run
> >> > > before the resume callback, but I was wrong. I think it makes sense to
> >> > > treat ACPI devices in a special way, but I really don't know, we need
> >> > > someone more knowledgeable to answer these questions. However, while I
> >> > > was trying to figure things out, I stumbled upon the following:
> >> > > e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend events").
> >> >
> >> > Gabriele, are you going to send this patch?
> >> >
> >> > I think that patch should be OK as it drop events when device is in
> >> > suspend state (when it should not receive events)...
> >> >
> >> > Darren, what do you think about it?
> >> >
> >>
> >> Sorry, this one has been difficult for me to track, but it's clearly an issue,
> >> and new systems are experiencing it as well.
> >>
> >> I'd like to get Rafael's opinion on disabling .notify ACPI function while
> >> suspended.
> >>
> >> +Rafael
> >
> > This by far wouldn't be enough, because drivers may install ACPI notify
> > handlers by themselves and those are hooked up directly into the ACPICA
> > code.
> >
> > Besides, some drivers may actually want to receive those events while
> > the system is suspending or resuming, so I think this has to be addressed
> > on a per-driver basis.
> 
> Hi,
> 
> sorry, but I'm not sure I understood everything, so I'll try to
> briefly describe the problem and its current solution.
> 
> Currently dell-rbtn listens for the notifications sent to an ACPI
> device and for notification sends an input event to userspace.
> 
> The problem we have is that some BIOSes send an extra notification
> on resume and therefore we send an extra input event.
> 
> What we want to do is to ignore this ACPI notification without
> affecting the systems whose BIOS does not send this extra
> notification. We know that not all of them send this notification.
> 
> What I noticed is that the extra notification is issued by the _WAK
> method and always arrives before dell-rbtn has been resumed, so
> what I did is to add a flag that is set by the suspend and resume
> callbacks of the device driver.

ACPI notifications are delivered asynchronously to drivers, so you'd
need to flush kacpi_notify_wq in .resume().  That would make the driver
wait for things it really doesn't need to wait for, so it would not be
super-optimal.

> What we were wondering is whether this would be enough or we
> need to do something different, e.g. ignore all the notifications that
> arrived X seconds after the execution of the resume callback.

I'd try something like setting the flag from .suspend() and then adding
a work item to clear it to kacpi_notify_wq from .resume().  Then you'll
know that all of the pending notifications have been processed before
your flag is cleared.

That would require a new helper for adding work items to kacpi_notify_wq
from drivers, but it shouldn't be too difficult to create one.

Thanks,
Rafael

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Pali Rohár]

On Tuesday 22 December 2015 01:20:30 Rafael J. Wysocki wrote:
> On Monday, December 21, 2015 04:34:58 PM Gabriele Mazzotta wrote:
> > 2015-12-20 17:21 GMT+01:00 Rafael J. Wysocki <rjw@rjwysocki.net>:
> > > On Friday, December 18, 2015 04:12:08 PM Darren Hart wrote:
> > >> On Fri, Nov 20, 2015 at 03:44:25PM +0100, Pali Rohár wrote:
> > >> > On Friday 23 October 2015 20:03:19 Gabriele Mazzotta wrote:
> > >> > > On 23/10/2015 13:14, Pali Rohár wrote:
> > >> > > >On Friday 23 October 2015 11:47:25 Gabriele Mazzotta wrote:
> > >> > > >>>In my opinion it is better to ignore user key press after resume, if it
> > >> > > >>>fix our problem. Better as false-positive event.
> > >> > > >>
> > >> > > >>The following appears to work really well. The notification arrives
> > >> > > >>before rbtn_resume() has been executed, so the extra event is ignored.
> > >> > > >>
> > >> > > >>diff --git a/drivers/platform/x86/dell-rbtn.c
> > >> > > >>b/drivers/platform/x86/dell-rbtn.c
> > >> > > >>index cd410e3..1d64b72 100644
> > >> > > >>--- a/drivers/platform/x86/dell-rbtn.c
> > >> > > >>+++ b/drivers/platform/x86/dell-rbtn.c
> > >> > > >>@@ -28,6 +28,7 @@ struct rbtn_data {
> > >> > > >>        enum rbtn_type type;
> > >> > > >>        struct rfkill *rfkill;
> > >> > > >>        struct input_dev *input_dev;
> > >> > > >>+       bool suspended;
> > >> > > >>  };
> > >> > > >>
> > >> > > >>
> > >> > > >>@@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
> > >> > > >>        { "", 0 },
> > >> > > >>  };
> > >> > > >>
> > >> > > >>+#ifdef CONFIG_PM_SLEEP
> > >> > > >>+static int rbtn_suspend(struct device *dev)
> > >> > > >>+{
> > >> > > >>+       struct acpi_device *device = to_acpi_device(dev);
> > >> > > >>+       struct rbtn_data *rbtn_data = acpi_driver_data(device);
> > >> > > >>+
> > >> > > >>+       rbtn_data->suspended = true;
> > >> > > >>+
> > >> > > >>+       return 0;
> > >> > > >>+}
> > >> > > >>+
> > >> > > >>+static int rbtn_resume(struct device *dev)
> > >> > > >>+{
> > >> > > >>+       struct acpi_device *device = to_acpi_device(dev);
> > >> > > >>+       struct rbtn_data *rbtn_data = acpi_driver_data(device);
> > >> > > >>+
> > >> > > >>+       rbtn_data->suspended = false;
> > >> > > >>+
> > >> > > >>+       return 0;
> > >> > > >>+}
> > >> > > >>+#endif
> > >> > > >>+static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
> > >> > > >>+
> > >> > > >>  static struct acpi_driver rbtn_driver = {
> > >> > > >>        .name = "dell-rbtn",
> > >> > > >>        .ids = rbtn_ids,
> > >> > > >>+       .drv.pm = &rbtn_pm_ops,
> > >> > > >>        .ops = {
> > >> > > >>                .add = rbtn_add,
> > >> > > >>                .remove = rbtn_remove,
> > >> > > >>@@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, u32
> > >> > > >>event)
> > >> > > >>  {
> > >> > > >>        struct rbtn_data *rbtn_data = device->driver_data;
> > >> > > >>
> > >> > > >>+       if (rbtn_data->suspended)
> > >> > > >>+               return;
> > >> > > >>+
> > >> > > >>        if (event != 0x80) {
> > >> > > >>                dev_info(&device->dev, "Received unknown event (0x%x)\n",
> > >> > > >>                         event);
> > >> > > >>
> > >> > > >
> > >> > > >Great, but is not there a better way to turn off .notify ACPI function
> > >> > > >when that ACPI device is suspended?
> > >> > > >
> > >> > > >Is not this ACPI device driver bug that it allows to call .notify method
> > >> > > >even if device is suspended?
> > >> > >
> > >> > > I was surprised this worked, I was assuming that nothing could run
> > >> > > before the resume callback, but I was wrong. I think it makes sense to
> > >> > > treat ACPI devices in a special way, but I really don't know, we need
> > >> > > someone more knowledgeable to answer these questions. However, while I
> > >> > > was trying to figure things out, I stumbled upon the following:
> > >> > > e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend events").
> > >> >
> > >> > Gabriele, are you going to send this patch?
> > >> >
> > >> > I think that patch should be OK as it drop events when device is in
> > >> > suspend state (when it should not receive events)...
> > >> >
> > >> > Darren, what do you think about it?
> > >> >
> > >>
> > >> Sorry, this one has been difficult for me to track, but it's clearly an issue,
> > >> and new systems are experiencing it as well.
> > >>
> > >> I'd like to get Rafael's opinion on disabling .notify ACPI function while
> > >> suspended.
> > >>
> > >> +Rafael
> > >
> > > This by far wouldn't be enough, because drivers may install ACPI notify
> > > handlers by themselves and those are hooked up directly into the ACPICA
> > > code.
> > >
> > > Besides, some drivers may actually want to receive those events while
> > > the system is suspending or resuming, so I think this has to be addressed
> > > on a per-driver basis.
> > 
> > Hi,
> > 
> > sorry, but I'm not sure I understood everything, so I'll try to
> > briefly describe the problem and its current solution.
> > 
> > Currently dell-rbtn listens for the notifications sent to an ACPI
> > device and for notification sends an input event to userspace.
> > 
> > The problem we have is that some BIOSes send an extra notification
> > on resume and therefore we send an extra input event.
> > 
> > What we want to do is to ignore this ACPI notification without
> > affecting the systems whose BIOS does not send this extra
> > notification. We know that not all of them send this notification.
> > 
> > What I noticed is that the extra notification is issued by the _WAK
> > method and always arrives before dell-rbtn has been resumed, so
> > what I did is to add a flag that is set by the suspend and resume
> > callbacks of the device driver.
> 
> ACPI notifications are delivered asynchronously to drivers, so you'd
> need to flush kacpi_notify_wq in .resume().  That would make the driver
> wait for things it really doesn't need to wait for, so it would not be
> super-optimal.
> 
> > What we were wondering is whether this would be enough or we
> > need to do something different, e.g. ignore all the notifications that
> > arrived X seconds after the execution of the resume callback.
> 
> I'd try something like setting the flag from .suspend() and then adding
> a work item to clear it to kacpi_notify_wq from .resume().  Then you'll
> know that all of the pending notifications have been processed before
> your flag is cleared.
> 
> That would require a new helper for adding work items to kacpi_notify_wq
> from drivers, but it shouldn't be too difficult to create one.
> 
> Thanks,
> Rafael
> 

Hi all! Is there any progress or new version of this patch?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Pali Rohár]

On Thursday 07 January 2016 23:35:29 Pali Rohár wrote:
> On Tuesday 22 December 2015 01:20:30 Rafael J. Wysocki wrote:
> > On Monday, December 21, 2015 04:34:58 PM Gabriele Mazzotta wrote:
> > > 2015-12-20 17:21 GMT+01:00 Rafael J. Wysocki <rjw@rjwysocki.net>:
> > > > On Friday, December 18, 2015 04:12:08 PM Darren Hart wrote:
> > > >> On Fri, Nov 20, 2015 at 03:44:25PM +0100, Pali Rohár wrote:
> > > >> > On Friday 23 October 2015 20:03:19 Gabriele Mazzotta wrote:
> > > >> > > On 23/10/2015 13:14, Pali Rohár wrote:
> > > >> > > >On Friday 23 October 2015 11:47:25 Gabriele Mazzotta wrote:
> > > >> > > >>>In my opinion it is better to ignore user key press after resume, if it
> > > >> > > >>>fix our problem. Better as false-positive event.
> > > >> > > >>
> > > >> > > >>The following appears to work really well. The notification arrives
> > > >> > > >>before rbtn_resume() has been executed, so the extra event is ignored.
> > > >> > > >>
> > > >> > > >>diff --git a/drivers/platform/x86/dell-rbtn.c
> > > >> > > >>b/drivers/platform/x86/dell-rbtn.c
> > > >> > > >>index cd410e3..1d64b72 100644
> > > >> > > >>--- a/drivers/platform/x86/dell-rbtn.c
> > > >> > > >>+++ b/drivers/platform/x86/dell-rbtn.c
> > > >> > > >>@@ -28,6 +28,7 @@ struct rbtn_data {
> > > >> > > >>        enum rbtn_type type;
> > > >> > > >>        struct rfkill *rfkill;
> > > >> > > >>        struct input_dev *input_dev;
> > > >> > > >>+       bool suspended;
> > > >> > > >>  };
> > > >> > > >>
> > > >> > > >>
> > > >> > > >>@@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
> > > >> > > >>        { "", 0 },
> > > >> > > >>  };
> > > >> > > >>
> > > >> > > >>+#ifdef CONFIG_PM_SLEEP
> > > >> > > >>+static int rbtn_suspend(struct device *dev)
> > > >> > > >>+{
> > > >> > > >>+       struct acpi_device *device = to_acpi_device(dev);
> > > >> > > >>+       struct rbtn_data *rbtn_data = acpi_driver_data(device);
> > > >> > > >>+
> > > >> > > >>+       rbtn_data->suspended = true;
> > > >> > > >>+
> > > >> > > >>+       return 0;
> > > >> > > >>+}
> > > >> > > >>+
> > > >> > > >>+static int rbtn_resume(struct device *dev)
> > > >> > > >>+{
> > > >> > > >>+       struct acpi_device *device = to_acpi_device(dev);
> > > >> > > >>+       struct rbtn_data *rbtn_data = acpi_driver_data(device);
> > > >> > > >>+
> > > >> > > >>+       rbtn_data->suspended = false;
> > > >> > > >>+
> > > >> > > >>+       return 0;
> > > >> > > >>+}
> > > >> > > >>+#endif
> > > >> > > >>+static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
> > > >> > > >>+
> > > >> > > >>  static struct acpi_driver rbtn_driver = {
> > > >> > > >>        .name = "dell-rbtn",
> > > >> > > >>        .ids = rbtn_ids,
> > > >> > > >>+       .drv.pm = &rbtn_pm_ops,
> > > >> > > >>        .ops = {
> > > >> > > >>                .add = rbtn_add,
> > > >> > > >>                .remove = rbtn_remove,
> > > >> > > >>@@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, u32
> > > >> > > >>event)
> > > >> > > >>  {
> > > >> > > >>        struct rbtn_data *rbtn_data = device->driver_data;
> > > >> > > >>
> > > >> > > >>+       if (rbtn_data->suspended)
> > > >> > > >>+               return;
> > > >> > > >>+
> > > >> > > >>        if (event != 0x80) {
> > > >> > > >>                dev_info(&device->dev, "Received unknown event (0x%x)\n",
> > > >> > > >>                         event);
> > > >> > > >>
> > > >> > > >
> > > >> > > >Great, but is not there a better way to turn off .notify ACPI function
> > > >> > > >when that ACPI device is suspended?
> > > >> > > >
> > > >> > > >Is not this ACPI device driver bug that it allows to call .notify method
> > > >> > > >even if device is suspended?
> > > >> > >
> > > >> > > I was surprised this worked, I was assuming that nothing could run
> > > >> > > before the resume callback, but I was wrong. I think it makes sense to
> > > >> > > treat ACPI devices in a special way, but I really don't know, we need
> > > >> > > someone more knowledgeable to answer these questions. However, while I
> > > >> > > was trying to figure things out, I stumbled upon the following:
> > > >> > > e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend events").
> > > >> >
> > > >> > Gabriele, are you going to send this patch?
> > > >> >
> > > >> > I think that patch should be OK as it drop events when device is in
> > > >> > suspend state (when it should not receive events)...
> > > >> >
> > > >> > Darren, what do you think about it?
> > > >> >
> > > >>
> > > >> Sorry, this one has been difficult for me to track, but it's clearly an issue,
> > > >> and new systems are experiencing it as well.
> > > >>
> > > >> I'd like to get Rafael's opinion on disabling .notify ACPI function while
> > > >> suspended.
> > > >>
> > > >> +Rafael
> > > >
> > > > This by far wouldn't be enough, because drivers may install ACPI notify
> > > > handlers by themselves and those are hooked up directly into the ACPICA
> > > > code.
> > > >
> > > > Besides, some drivers may actually want to receive those events while
> > > > the system is suspending or resuming, so I think this has to be addressed
> > > > on a per-driver basis.
> > > 
> > > Hi,
> > > 
> > > sorry, but I'm not sure I understood everything, so I'll try to
> > > briefly describe the problem and its current solution.
> > > 
> > > Currently dell-rbtn listens for the notifications sent to an ACPI
> > > device and for notification sends an input event to userspace.
> > > 
> > > The problem we have is that some BIOSes send an extra notification
> > > on resume and therefore we send an extra input event.
> > > 
> > > What we want to do is to ignore this ACPI notification without
> > > affecting the systems whose BIOS does not send this extra
> > > notification. We know that not all of them send this notification.
> > > 
> > > What I noticed is that the extra notification is issued by the _WAK
> > > method and always arrives before dell-rbtn has been resumed, so
> > > what I did is to add a flag that is set by the suspend and resume
> > > callbacks of the device driver.
> > 
> > ACPI notifications are delivered asynchronously to drivers, so you'd
> > need to flush kacpi_notify_wq in .resume().  That would make the driver
> > wait for things it really doesn't need to wait for, so it would not be
> > super-optimal.
> > 
> > > What we were wondering is whether this would be enough or we
> > > need to do something different, e.g. ignore all the notifications that
> > > arrived X seconds after the execution of the resume callback.
> > 
> > I'd try something like setting the flag from .suspend() and then adding
> > a work item to clear it to kacpi_notify_wq from .resume().  Then you'll
> > know that all of the pending notifications have been processed before
> > your flag is cleared.
> > 
> > That would require a new helper for adding work items to kacpi_notify_wq
> > from drivers, but it shouldn't be too difficult to create one.
> > 
> > Thanks,
> > Rafael
> > 
> 
> Hi all! Is there any progress or new version of this patch?
> 

Gabriele, Darren, Alex... was this problem fixed? Or what is current state?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Gabriele Mazzotta]

2016-03-11 10:45 GMT+01:00 Pali Rohár <pali.rohar@gmail.com>:
> On Thursday 07 January 2016 23:35:29 Pali Rohár wrote:
>> On Tuesday 22 December 2015 01:20:30 Rafael J. Wysocki wrote:
>> > On Monday, December 21, 2015 04:34:58 PM Gabriele Mazzotta wrote:
>> > > 2015-12-20 17:21 GMT+01:00 Rafael J. Wysocki <rjw@rjwysocki.net>:
>> > > > On Friday, December 18, 2015 04:12:08 PM Darren Hart wrote:
>> > > >> On Fri, Nov 20, 2015 at 03:44:25PM +0100, Pali Rohár wrote:
>> > > >> > On Friday 23 October 2015 20:03:19 Gabriele Mazzotta wrote:
>> > > >> > > On 23/10/2015 13:14, Pali Rohár wrote:
>> > > >> > > >On Friday 23 October 2015 11:47:25 Gabriele Mazzotta wrote:
>> > > >> > > >>>In my opinion it is better to ignore user key press after resume, if it
>> > > >> > > >>>fix our problem. Better as false-positive event.
>> > > >> > > >>
>> > > >> > > >>The following appears to work really well. The notification arrives
>> > > >> > > >>before rbtn_resume() has been executed, so the extra event is ignored.
>> > > >> > > >>
>> > > >> > > >>diff --git a/drivers/platform/x86/dell-rbtn.c
>> > > >> > > >>b/drivers/platform/x86/dell-rbtn.c
>> > > >> > > >>index cd410e3..1d64b72 100644
>> > > >> > > >>--- a/drivers/platform/x86/dell-rbtn.c
>> > > >> > > >>+++ b/drivers/platform/x86/dell-rbtn.c
>> > > >> > > >>@@ -28,6 +28,7 @@ struct rbtn_data {
>> > > >> > > >>        enum rbtn_type type;
>> > > >> > > >>        struct rfkill *rfkill;
>> > > >> > > >>        struct input_dev *input_dev;
>> > > >> > > >>+       bool suspended;
>> > > >> > > >>  };
>> > > >> > > >>
>> > > >> > > >>
>> > > >> > > >>@@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
>> > > >> > > >>        { "", 0 },
>> > > >> > > >>  };
>> > > >> > > >>
>> > > >> > > >>+#ifdef CONFIG_PM_SLEEP
>> > > >> > > >>+static int rbtn_suspend(struct device *dev)
>> > > >> > > >>+{
>> > > >> > > >>+       struct acpi_device *device = to_acpi_device(dev);
>> > > >> > > >>+       struct rbtn_data *rbtn_data = acpi_driver_data(device);
>> > > >> > > >>+
>> > > >> > > >>+       rbtn_data->suspended = true;
>> > > >> > > >>+
>> > > >> > > >>+       return 0;
>> > > >> > > >>+}
>> > > >> > > >>+
>> > > >> > > >>+static int rbtn_resume(struct device *dev)
>> > > >> > > >>+{
>> > > >> > > >>+       struct acpi_device *device = to_acpi_device(dev);
>> > > >> > > >>+       struct rbtn_data *rbtn_data = acpi_driver_data(device);
>> > > >> > > >>+
>> > > >> > > >>+       rbtn_data->suspended = false;
>> > > >> > > >>+
>> > > >> > > >>+       return 0;
>> > > >> > > >>+}
>> > > >> > > >>+#endif
>> > > >> > > >>+static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
>> > > >> > > >>+
>> > > >> > > >>  static struct acpi_driver rbtn_driver = {
>> > > >> > > >>        .name = "dell-rbtn",
>> > > >> > > >>        .ids = rbtn_ids,
>> > > >> > > >>+       .drv.pm = &rbtn_pm_ops,
>> > > >> > > >>        .ops = {
>> > > >> > > >>                .add = rbtn_add,
>> > > >> > > >>                .remove = rbtn_remove,
>> > > >> > > >>@@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, u32
>> > > >> > > >>event)
>> > > >> > > >>  {
>> > > >> > > >>        struct rbtn_data *rbtn_data = device->driver_data;
>> > > >> > > >>
>> > > >> > > >>+       if (rbtn_data->suspended)
>> > > >> > > >>+               return;
>> > > >> > > >>+
>> > > >> > > >>        if (event != 0x80) {
>> > > >> > > >>                dev_info(&device->dev, "Received unknown event (0x%x)\n",
>> > > >> > > >>                         event);
>> > > >> > > >>
>> > > >> > > >
>> > > >> > > >Great, but is not there a better way to turn off .notify ACPI function
>> > > >> > > >when that ACPI device is suspended?
>> > > >> > > >
>> > > >> > > >Is not this ACPI device driver bug that it allows to call .notify method
>> > > >> > > >even if device is suspended?
>> > > >> > >
>> > > >> > > I was surprised this worked, I was assuming that nothing could run
>> > > >> > > before the resume callback, but I was wrong. I think it makes sense to
>> > > >> > > treat ACPI devices in a special way, but I really don't know, we need
>> > > >> > > someone more knowledgeable to answer these questions. However, while I
>> > > >> > > was trying to figure things out, I stumbled upon the following:
>> > > >> > > e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend events").
>> > > >> >
>> > > >> > Gabriele, are you going to send this patch?
>> > > >> >
>> > > >> > I think that patch should be OK as it drop events when device is in
>> > > >> > suspend state (when it should not receive events)...
>> > > >> >
>> > > >> > Darren, what do you think about it?
>> > > >> >
>> > > >>
>> > > >> Sorry, this one has been difficult for me to track, but it's clearly an issue,
>> > > >> and new systems are experiencing it as well.
>> > > >>
>> > > >> I'd like to get Rafael's opinion on disabling .notify ACPI function while
>> > > >> suspended.
>> > > >>
>> > > >> +Rafael
>> > > >
>> > > > This by far wouldn't be enough, because drivers may install ACPI notify
>> > > > handlers by themselves and those are hooked up directly into the ACPICA
>> > > > code.
>> > > >
>> > > > Besides, some drivers may actually want to receive those events while
>> > > > the system is suspending or resuming, so I think this has to be addressed
>> > > > on a per-driver basis.
>> > >
>> > > Hi,
>> > >
>> > > sorry, but I'm not sure I understood everything, so I'll try to
>> > > briefly describe the problem and its current solution.
>> > >
>> > > Currently dell-rbtn listens for the notifications sent to an ACPI
>> > > device and for notification sends an input event to userspace.
>> > >
>> > > The problem we have is that some BIOSes send an extra notification
>> > > on resume and therefore we send an extra input event.
>> > >
>> > > What we want to do is to ignore this ACPI notification without
>> > > affecting the systems whose BIOS does not send this extra
>> > > notification. We know that not all of them send this notification.
>> > >
>> > > What I noticed is that the extra notification is issued by the _WAK
>> > > method and always arrives before dell-rbtn has been resumed, so
>> > > what I did is to add a flag that is set by the suspend and resume
>> > > callbacks of the device driver.
>> >
>> > ACPI notifications are delivered asynchronously to drivers, so you'd
>> > need to flush kacpi_notify_wq in .resume().  That would make the driver
>> > wait for things it really doesn't need to wait for, so it would not be
>> > super-optimal.
>> >
>> > > What we were wondering is whether this would be enough or we
>> > > need to do something different, e.g. ignore all the notifications that
>> > > arrived X seconds after the execution of the resume callback.
>> >
>> > I'd try something like setting the flag from .suspend() and then adding
>> > a work item to clear it to kacpi_notify_wq from .resume().  Then you'll
>> > know that all of the pending notifications have been processed before
>> > your flag is cleared.
>> >
>> > That would require a new helper for adding work items to kacpi_notify_wq
>> > from drivers, but it shouldn't be too difficult to create one.
>> >
>> > Thanks,
>> > Rafael
>> >
>>
>> Hi all! Is there any progress or new version of this patch?
>>
>
> Gabriele, Darren, Alex... was this problem fixed? Or what is current state?

As far as I know, there was no progress. I'm now going to try what
Rafael suggested and see what I can do.

> --
> Pali Rohár
> pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Pali Rohár]

On Saturday 12 March 2016 00:30:37 Gabriele Mazzotta wrote:
> 2016-03-11 10:45 GMT+01:00 Pali Rohár <pali.rohar@gmail.com>:
> > Gabriele, Darren, Alex... was this problem fixed? Or what is current state?
> 
> As far as I know, there was no progress. I'm now going to try what
> Rafael suggested and see what I can do.

Ok, thanks for info. If you have some results, let us know as we can
finally fix this problem...

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Alex Hung]

On Mon, Dec 21, 2015 at 11:34 PM, Gabriele Mazzotta
<gabriele.mzt@gmail.com> wrote:
> 2015-12-20 17:21 GMT+01:00 Rafael J. Wysocki <rjw@rjwysocki.net>:
>> On Friday, December 18, 2015 04:12:08 PM Darren Hart wrote:
>>> On Fri, Nov 20, 2015 at 03:44:25PM +0100, Pali Rohár wrote:
>>> > On Friday 23 October 2015 20:03:19 Gabriele Mazzotta wrote:
>>> > > On 23/10/2015 13:14, Pali Rohár wrote:
>>> > > >On Friday 23 October 2015 11:47:25 Gabriele Mazzotta wrote:
>>> > > >>>In my opinion it is better to ignore user key press after resume, if it
>>> > > >>>fix our problem. Better as false-positive event.
>>> > > >>
>>> > > >>The following appears to work really well. The notification arrives
>>> > > >>before rbtn_resume() has been executed, so the extra event is ignored.
>>> > > >>
>>> > > >>diff --git a/drivers/platform/x86/dell-rbtn.c
>>> > > >>b/drivers/platform/x86/dell-rbtn.c
>>> > > >>index cd410e3..1d64b72 100644
>>> > > >>--- a/drivers/platform/x86/dell-rbtn.c
>>> > > >>+++ b/drivers/platform/x86/dell-rbtn.c
>>> > > >>@@ -28,6 +28,7 @@ struct rbtn_data {
>>> > > >>        enum rbtn_type type;
>>> > > >>        struct rfkill *rfkill;
>>> > > >>        struct input_dev *input_dev;
>>> > > >>+       bool suspended;
>>> > > >>  };
>>> > > >>
>>> > > >>
>>> > > >>@@ -220,9 +221,33 @@ static const struct acpi_device_id rbtn_ids[] = {
>>> > > >>        { "", 0 },
>>> > > >>  };
>>> > > >>
>>> > > >>+#ifdef CONFIG_PM_SLEEP
>>> > > >>+static int rbtn_suspend(struct device *dev)
>>> > > >>+{
>>> > > >>+       struct acpi_device *device = to_acpi_device(dev);
>>> > > >>+       struct rbtn_data *rbtn_data = acpi_driver_data(device);
>>> > > >>+
>>> > > >>+       rbtn_data->suspended = true;
>>> > > >>+
>>> > > >>+       return 0;
>>> > > >>+}
>>> > > >>+
>>> > > >>+static int rbtn_resume(struct device *dev)
>>> > > >>+{
>>> > > >>+       struct acpi_device *device = to_acpi_device(dev);
>>> > > >>+       struct rbtn_data *rbtn_data = acpi_driver_data(device);
>>> > > >>+
>>> > > >>+       rbtn_data->suspended = false;
>>> > > >>+
>>> > > >>+       return 0;
>>> > > >>+}
>>> > > >>+#endif
>>> > > >>+static SIMPLE_DEV_PM_OPS(rbtn_pm_ops, rbtn_suspend, rbtn_resume);
>>> > > >>+
>>> > > >>  static struct acpi_driver rbtn_driver = {
>>> > > >>        .name = "dell-rbtn",
>>> > > >>        .ids = rbtn_ids,
>>> > > >>+       .drv.pm = &rbtn_pm_ops,
>>> > > >>        .ops = {
>>> > > >>                .add = rbtn_add,
>>> > > >>                .remove = rbtn_remove,
>>> > > >>@@ -384,6 +409,9 @@ static void rbtn_notify(struct acpi_device *device, u32
>>> > > >>event)
>>> > > >>  {
>>> > > >>        struct rbtn_data *rbtn_data = device->driver_data;
>>> > > >>
>>> > > >>+       if (rbtn_data->suspended)
>>> > > >>+               return;
>>> > > >>+
>>> > > >>        if (event != 0x80) {
>>> > > >>                dev_info(&device->dev, "Received unknown event (0x%x)\n",
>>> > > >>                         event);
>>> > > >>
>>> > > >
>>> > > >Great, but is not there a better way to turn off .notify ACPI function
>>> > > >when that ACPI device is suspended?
>>> > > >
>>> > > >Is not this ACPI device driver bug that it allows to call .notify method
>>> > > >even if device is suspended?
>>> > >
>>> > > I was surprised this worked, I was assuming that nothing could run
>>> > > before the resume callback, but I was wrong. I think it makes sense to
>>> > > treat ACPI devices in a special way, but I really don't know, we need
>>> > > someone more knowledgeable to answer these questions. However, while I
>>> > > was trying to figure things out, I stumbled upon the following:
>>> > > e71eeb2a6bcc ("ACPI / button: Do not propagate wakeup-from-suspend events").
>>> >
>>> > Gabriele, are you going to send this patch?
>>> >
>>> > I think that patch should be OK as it drop events when device is in
>>> > suspend state (when it should not receive events)...
>>> >
>>> > Darren, what do you think about it?
>>> >
>>>
>>> Sorry, this one has been difficult for me to track, but it's clearly an issue,
>>> and new systems are experiencing it as well.
>>>
>>> I'd like to get Rafael's opinion on disabling .notify ACPI function while
>>> suspended.
>>>
>>> +Rafael
>>
>> This by far wouldn't be enough, because drivers may install ACPI notify
>> handlers by themselves and those are hooked up directly into the ACPICA
>> code.
>>
>> Besides, some drivers may actually want to receive those events while
>> the system is suspending or resuming, so I think this has to be addressed
>> on a per-driver basis.
>
> Hi,
>
> sorry, but I'm not sure I understood everything, so I'll try to
> briefly describe the problem and its current solution.
>
> Currently dell-rbtn listens for the notifications sent to an ACPI
> device and for notification sends an input event to userspace.
>
> The problem we have is that some BIOSes send an extra notification
> on resume and therefore we send an extra input event.
>
> What we want to do is to ignore this ACPI notification without
> affecting the systems whose BIOS does not send this extra
> notification. We know that not all of them send this notification.
>
> What I noticed is that the extra notification is issued by the _WAK
> method and always arrives before dell-rbtn has been resumed, so
> what I did is to add a flag that is set by the suspend and resume
> callbacks of the device driver.

Sorry I screw up my mail filter and I wasn't aware of this thread until now.

BIOS sends this additional ACPI event for the systems with hardware
switch so a driver can update its state; therefore this is done only
once and therefore ignoring the ACPI event sent to dell rbtn once
after resume is sufficient.

I actually tried a solution similar to Gabriele's patch above (one
with rbtn_suspend and rbtn_resume) a while ago and it works fine.

If there is a conclusion and there is a patch to be tested, I am happy
to test it on wider range (I should be able to find 5+ Dell systems
that runs on dell-rbtn).

>
> What we were wondering is whether this would be enough or we
> need to do something different, e.g. ignore all the notifications that
> arrived X seconds after the execution of the resume callback.
>
> Thanks,
> Gabriele
>
>>> Has Dell been involved here?
>>
>> Not that I know of.
>>
>> Thanks,
>> Rafael
>>



-- 
Cheers,
Alex Hung

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Darren Hart]

On Wed, Oct 21, 2015 at 01:12:43PM +0200, Darren Hart wrote:
> From what I can tell, the XPS 13 has a TOGGLE type rfkill button (Fn-PrtScrn):
> 
> http://www.trustedreviews.com/dell-xps-13-2015-photos-11

The reporter states his radio toggle is on F2, so it is not the above machine,
probably this one:

http://www.ibtimes.co.uk/dell-xps-13-review-ultrabook-341773

In any case, his logs confirm we are seeing a TOGGLE type which is using the
input mechanism.

-- 
Darren Hart
Intel Open Source Technology Center

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Pali Rohár]

On Thursday 22 October 2015 10:17:25 Darren Hart wrote:
> On Wed, Oct 21, 2015 at 01:12:43PM +0200, Darren Hart wrote:
> > From what I can tell, the XPS 13 has a TOGGLE type rfkill button (Fn-PrtScrn):
> > 
> > http://www.trustedreviews.com/dell-xps-13-2015-photos-11
> 
> The reporter states his radio toggle is on F2, so it is not the above machine,
> probably this one:
> 
> http://www.ibtimes.co.uk/dell-xps-13-review-ultrabook-341773
> 
> In any case, his logs confirm we are seeing a TOGGLE type which is using the
> input mechanism.
> 

So my assumption that turning rfkill on (airplane mode) is done by
userspace (and not by kernel itself) is correct.

Now we need to know if we can drop that event input key after resuming
from suspend on all dell machines which has toggle key.

Alex, do you know more? Can you help us?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Darren Hart]

On Thu, Oct 22, 2015 at 10:27:22AM +0200, Pali Rohár wrote:
> On Thursday 22 October 2015 10:17:25 Darren Hart wrote:
> > On Wed, Oct 21, 2015 at 01:12:43PM +0200, Darren Hart wrote:
> > > From what I can tell, the XPS 13 has a TOGGLE type rfkill button (Fn-PrtScrn):
> > > 
> > > http://www.trustedreviews.com/dell-xps-13-2015-photos-11
> > 
> > The reporter states his radio toggle is on F2, so it is not the above machine,
> > probably this one:
> > 
> > http://www.ibtimes.co.uk/dell-xps-13-review-ultrabook-341773
> > 
> > In any case, his logs confirm we are seeing a TOGGLE type which is using the
> > input mechanism.
> > 
> 
> So my assumption that turning rfkill on (airplane mode) is done by
> userspace (and not by kernel itself) is correct.

Agreed.

> 
> Now we need to know if we can drop that event input key after resuming
> from suspend on all dell machines which has toggle key.
> 

Well, as Gabriele points out, it seems it is dependent on the system, as some
with the TOGGLE emit the event on resume, and others do not.

We could of course do this on a case by case DMI match, but I'd really like to
avoid that if at all possible.

> Alex, do you know more? Can you help us?
> 
> -- 
> Pali Rohár
> pali.rohar@gmail.com
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Gabriele Mazzotta]

2015-10-22 10:17 GMT+02:00 Darren Hart <dvhart@infradead.org>:
> On Wed, Oct 21, 2015 at 01:12:43PM +0200, Darren Hart wrote:
>> From what I can tell, the XPS 13 has a TOGGLE type rfkill button (Fn-PrtScrn):
>>
>> http://www.trustedreviews.com/dell-xps-13-2015-photos-11
>
> The reporter states his radio toggle is on F2, so it is not the above machine,
> probably this one:
>
> http://www.ibtimes.co.uk/dell-xps-13-review-ultrabook-341773
>
> In any case, his logs confirm we are seeing a TOGGLE type which is using the
> input mechanism.

According to the dmesg it's an XPS13 9333, which is the one I own.

> --
> Darren Hart
> Intel Open Source Technology Center

Re: [Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[Darren Hart]

On Thu, Oct 22, 2015 at 10:28:21AM +0200, Gabriele Mazzotta wrote:
> 2015-10-22 10:17 GMT+02:00 Darren Hart <dvhart@infradead.org>:
> > On Wed, Oct 21, 2015 at 01:12:43PM +0200, Darren Hart wrote:
> >> From what I can tell, the XPS 13 has a TOGGLE type rfkill button (Fn-PrtScrn):
> >>
> >> http://www.trustedreviews.com/dell-xps-13-2015-photos-11
> >
> > The reporter states his radio toggle is on F2, so it is not the above machine,
> > probably this one:
> >
> > http://www.ibtimes.co.uk/dell-xps-13-review-ultrabook-341773
> >
> > In any case, his logs confirm we are seeing a TOGGLE type which is using the
> > input mechanism.
> 
> According to the dmesg it's an XPS13 9333, which is the one I own.

Oh excellent! That should help considerably!

-- 
Darren Hart
Intel Open Source Technology Center

[Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=106031

Emmanuel Grumbach (emmanuel.grumbach@intel.com) changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |CODE_FIX

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.

[Bug 106031] Regression in 4.2.x: in airplane mode each time I open my laptop lid

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=106031

Emmanuel Grumbach (emmanuel.grumbach@intel.com) changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching the assignee of the bug.