Re: Regression with dell-rbtn: radio killed on resume after suspend to RAM

[Andrei Borzenkov <arvidjaar@gmail.com>]

23.11.2015 18:14, Gabriele Mazzotta пишет:
> On 23/11/2015 15:50, Pali Rohár wrote:
>> On Sunday 22 November 2015 09:23:19 Andrei Borzenkov wrote:
>>> 21.11.2015 23:39, Andrei Borzenkov пишет:
>>>> 21.11.2015 22:08, Pali Rohár пишет:
>>>>> On Saturday 21 November 2015 19:57:18 Andrei Borzenkov wrote:
>>>>>> After installing 4.2 on Dell Latitude E5450 I found that wireless was
>>>>>> disabled every second resume from suspend to RAM. Blacklisting
>>>>>> dell-rbtn "fixed" it.
>>>>>>
>>>>>> This is probably the same as discussed in this Arch forum and related
>>>>>> bug report: https://bbs.archlinux.org/viewtopic.php?id=203404. I
>>>>>> myself hit it on Ubuntu.
>>>>>>
>>>>>> I am not familiar with other models, but E5450 does have DELLABCE and
>>>>>> so is using dell-rbtn driver.
>>>>>>
>>>>>> I am happy to test patches if need. Please let me know if formal bug
>>>>>> report is required.
>>>>>>
>>>>>> -andrei
>>>>>
>>>>> Hi Andrei!
>>>>>
>>>>> About five hours ago Gabriele sent patch which fixing this problem to
>>>>> LKML. You can find it on: https://lkml.org/lkml/2015/11/21/57
>>>>>
>>>>> Can you test that patch and confirm it fix also for you?
>>>>>
>>>>
>>>> Yes, it does.
>>>
>>> Unfortunately it was too early. Today after resume I again got disable
>>> radio, and also several later attempts to suspend/resume. Then last
>>> time I
>>> suddenly got radio working again after resume.
>>>
>>> Patch looks racy; there is no guarantee we get notification before
>>> resetting
>>> in suspend flag.
>>>
>>
>> Gabriele, any idea how to fix this?
>>
>
> I can't think of anything other than ignoring notifications that arrives
> within X seconds after the execution of the resume callback.
> Unfortunately, we have no idea on how to detect systems that misbehave
> and creating a blacklist is not a feasible solution.
>
> Something such as the following should work, but maybe it's not a nice
> solution.
>

I looked at what dell-rbtn does. My system (Latitude E5450) has RBTN of 
type TOGGLE. In this case both del-laptop hooks itself into i8042 and 
dell-rbtn delivers KEY_RFKILL event to input susbsystem. IOW dell-rbtn 
looks completely redundant in this configuration.

Can we detect that rfkill toggle is already avaiable via normal keyboard 
and not activate dell-rbtn in this case?

> ---
>   drivers/platform/x86/dell-rbtn.c | 36
> ++++++++++++++++++++++++++++++++++++
>   1 file changed, 36 insertions(+)
>
> diff --git a/drivers/platform/x86/dell-rbtn.c
> b/drivers/platform/x86/dell-rbtn.c
> index cd410e3..c03062d 100644
> --- a/drivers/platform/x86/dell-rbtn.c
> +++ b/drivers/platform/x86/dell-rbtn.c
> @@ -18,6 +18,8 @@
>   #include <linux/rfkill.h>
>   #include <linux/input.h>
>
> +#define EXTRA_NOTIFICATION_TIMEOUT     (2 * HZ);
> +
>   enum rbtn_type {
>          RBTN_UNKNOWN,
>          RBTN_TOGGLE,
> @@ -28,6 +30,8 @@ struct rbtn_data {
>          enum rbtn_type type;
>          struct rfkill *rfkill;
>          struct input_dev *input_dev;
> +       bool suspended;
> +       unsigned long ignore_window;
>   };
>
>
> @@ -220,9 +224,34 @@ 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;
> +       rbtn_data->ignore_window = jiffies + EXTRA_NOTIFICATION_TIMEOUT;
> +
> +       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,
> @@ -383,6 +412,13 @@ static int rbtn_remove(struct acpi_device *device)
>   static void rbtn_notify(struct acpi_device *device, u32 event)
>   {
>          struct rbtn_data *rbtn_data = device->driver_data;
> +       bool ignore_notification = rbtn_data->suspended ||
> +                       time_before(jiffies, rbtn_data->ignore_window);
> +
> +       if (ignore_notification) {
> +               dev_dbg(&device->dev, "ACPI notification ignored\n");
> +               return;
> +       }
>
>          if (event != 0x80) {
>                  dev_info(&device->dev, "Received unknown event (0x%x)\n",