From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751823AbdLFNYQ (ORCPT ); Wed, 6 Dec 2017 08:24:16 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:33836 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751208AbdLFNYN (ORCPT ); Wed, 6 Dec 2017 08:24:13 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 7F4136022C Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=okaya@codeaurora.org Subject: Re: [PATCH] ACPI / GED: unregister interrupts during shutdown To: "Rafael J. Wysocki" 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 References: <1512507705-2411-1-git-send-email-okaya@codeaurora.org> From: Sinan Kaya Message-ID: <0b661eec-f9c7-7818-ad09-f3d488adcefe@codeaurora.org> Date: Wed, 6 Dec 2017 08:24:09 -0500 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/5/2017 5:18 PM, Rafael J. Wysocki wrote: > 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? 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.