From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Subject: Re: [PATCH] ACPI / GED: unregister interrupts during shutdown Date: Tue, 5 Dec 2017 23:18:44 +0100 Message-ID: References: <1512507705-2411-1-git-send-email-okaya@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <1512507705-2411-1-git-send-email-okaya@codeaurora.org> Sender: linux-acpi-owner@vger.kernel.org To: Sinan Kaya Cc: ACPI Devel Maling List , Timur Tabi , "Rafael J. Wysocki" , linux-arm-msm@vger.kernel.org, "linux-arm-kernel@lists.infradead.org" , Linux Kernel Mailing List List-Id: linux-arm-msm@vger.kernel.org On Tue, Dec 5, 2017 at 10:01 PM, Sinan Kaya 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 > --- > 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? > * > * 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()? > + 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. > + } > +} > + > 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. > .probe = ged_probe, > .driver = { > .name = MODULE_NAME, > -- Overall, it looks like we should just unbind the driver from all devices on shutdown. Thanks, Rafael From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752210AbdLEWSt (ORCPT ); Tue, 5 Dec 2017 17:18:49 -0500 Received: from mail-ot0-f193.google.com ([74.125.82.193]:37659 "EHLO mail-ot0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751500AbdLEWSp (ORCPT ); Tue, 5 Dec 2017 17:18:45 -0500 X-Google-Smtp-Source: AGs4zMYQd28DbLssft7TWkxCYkU39+V57f3obtcDJcfmpubMpeSvsTjlq03RORSUabb+kIIrE3F+Jx175IbtSui5lxA= MIME-Version: 1.0 In-Reply-To: <1512507705-2411-1-git-send-email-okaya@codeaurora.org> References: <1512507705-2411-1-git-send-email-okaya@codeaurora.org> From: "Rafael J. Wysocki" Date: Tue, 5 Dec 2017 23:18:44 +0100 X-Google-Sender-Auth: OK1FjGSOHSQ1W3IAFeQaq9tsKAw Message-ID: Subject: Re: [PATCH] ACPI / GED: unregister interrupts during shutdown To: Sinan Kaya Cc: ACPI Devel Maling List , Timur Tabi , "Rafael J. Wysocki" , linux-arm-msm@vger.kernel.org, "linux-arm-kernel@lists.infradead.org" , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Dec 5, 2017 at 10:01 PM, Sinan Kaya 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 > --- > 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? > * > * 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()? > + 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. > + } > +} > + > 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. > .probe = ged_probe, > .driver = { > .name = MODULE_NAME, > -- Overall, it looks like we should just unbind the driver from all devices on shutdown. Thanks, Rafael From mboxrd@z Thu Jan 1 00:00:00 1970 From: rafael@kernel.org (Rafael J. Wysocki) Date: Tue, 5 Dec 2017 23:18:44 +0100 Subject: [PATCH] ACPI / GED: unregister interrupts during shutdown In-Reply-To: <1512507705-2411-1-git-send-email-okaya@codeaurora.org> References: <1512507705-2411-1-git-send-email-okaya@codeaurora.org> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Dec 5, 2017 at 10:01 PM, Sinan Kaya 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 > --- > 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? > * > * 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()? > + 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. > + } > +} > + > 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. > .probe = ged_probe, > .driver = { > .name = MODULE_NAME, > -- Overall, it looks like we should just unbind the driver from all devices on shutdown. Thanks, Rafael