Re: [PATCH] ACPI / GED: unregister interrupts during shutdown

[Sinan Kaya <okaya@codeaurora.org>]

On 12/5/2017 5:18 PM, Rafael J. Wysocki wrote:
> On Tue, Dec 5, 2017 at 10:01 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> Some GED interrupts could be pending by the time we are doing a reboot.
>>
>> Even though GED driver uses devm_request_irq() to register the interrupt
>> handler, the handler is not being freed on time during a shutdown since
>> the driver is missing a shutdown callback.
>>
>> If the ACPI handler is no longer available, this causes an interrupt
>> storm and delays shutdown.
>>
>> Register a shutdown callback and manually free the interrupts.
>>
>> Signed-off-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>>  drivers/acpi/evged.c | 36 +++++++++++++++++++++++++++++++++---
>>  1 file changed, 33 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/evged.c b/drivers/acpi/evged.c
>> index 46f0603..dde50ea 100644
>> --- a/drivers/acpi/evged.c
>> +++ b/drivers/acpi/evged.c
>> @@ -1,7 +1,7 @@
>>  /*
>>   * Generic Event Device for ACPI.
>>   *
>> - * Copyright (c) 2016, The Linux Foundation. All rights reserved.
>> + * Copyright (c) 2016-2017, The Linux Foundation. All rights reserved.
> 
> Why are you changing this?

Some legal BS that we are trying to figure out internally. This slipped through
the cracks. I was going to remove it before posting.

> 
>>   *
>>   * This program is free software; you can redistribute it and/or modify
>>   * it under the terms of the GNU General Public License version 2 and
>> @@ -49,6 +49,11 @@
>>
>>  #define MODULE_NAME    "acpi-ged"
>>
>> +struct acpi_ged_device {
>> +       struct device *dev;
>> +       struct list_head event_list;
>> +};
>> +
>>  struct acpi_ged_event {
>>         struct list_head node;
>>         struct device *dev;
>> @@ -76,7 +81,8 @@ static acpi_status acpi_ged_request_interrupt(struct acpi_resource *ares,
>>         unsigned int irq;
>>         unsigned int gsi;
>>         unsigned int irqflags = IRQF_ONESHOT;
>> -       struct device *dev = context;
>> +       struct acpi_ged_device *geddev = context;
>> +       struct device *dev = geddev->dev;
>>         acpi_handle handle = ACPI_HANDLE(dev);
>>         acpi_handle evt_handle;
>>         struct resource r;
>> @@ -122,29 +128,53 @@ static acpi_status acpi_ged_request_interrupt(struct acpi_resource *ares,
>>                 return AE_ERROR;
>>         }
>>
>> +       dev_info(dev, "GED listening GSI %u @ IRQ %u\n", gsi, irq);
> 
> Why dev_info()?

I can change it to dev_dbg.

> 
>> +       list_add_tail(&event->node, &geddev->event_list);
>>         return AE_OK;
>>  }
>>
>>  static int ged_probe(struct platform_device *pdev)
>>  {
>> +       struct acpi_ged_device *geddev;
>>         acpi_status acpi_ret;
>>
>> +       geddev = devm_kzalloc(&pdev->dev, sizeof(*geddev), GFP_KERNEL);
>> +       if (!geddev)
>> +               return -ENOMEM;
>> +
>> +       geddev->dev = &pdev->dev;
>> +       INIT_LIST_HEAD(&geddev->event_list);
>>         acpi_ret = acpi_walk_resources(ACPI_HANDLE(&pdev->dev), "_CRS",
>> -                                      acpi_ged_request_interrupt, &pdev->dev);
>> +                                      acpi_ged_request_interrupt, geddev);
>>         if (ACPI_FAILURE(acpi_ret)) {
>>                 dev_err(&pdev->dev, "unable to parse the _CRS record\n");
>>                 return -EINVAL;
>>         }
>> +       platform_set_drvdata(pdev, geddev);
>>
>>         return 0;
>>  }
>>
>> +static void ged_remove(struct platform_device *pdev)
>> +{
>> +       struct acpi_ged_device *geddev = platform_get_drvdata(pdev);
>> +       struct acpi_ged_event *event, *next;
>> +
>> +       list_for_each_entry_safe(event, next, &geddev->event_list, node) {
>> +               devm_free_irq(geddev->dev, event->irq, event);
>> +               list_del(&event->node);
>> +               dev_info(geddev->dev, "GED releasing GSI %u @ IRQ %u\n",
>> +                        event->gsi, event->irq);
> 
> dev_dbg() and that if you really need it.

will change to dev_dbg

> 
>> +       }
>> +}
>> +
>>  static const struct acpi_device_id ged_acpi_ids[] = {
>>         {"ACPI0013"},
>>         {},
>>  };
>>
>>  static struct platform_driver ged_driver = {
>> +       .shutdown = ged_remove,
> 
> That ged_remove really should be called ged_shutdown.  It is confusing
> as is, because there is a ->remove callback in the structure too.

I'll rename as shutdown.

> 
>>         .probe = ged_probe,
>>         .driver = {
>>                 .name = MODULE_NAME,
>> --
> 
> Overall, it looks like we should just unbind the driver from all
> devices on shutdown.

I see that shutdown is getting called on all GED instances. That should
take care of it, right?

> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.