[PATCH] ACPI / GED: unregister interrupts during shutdown

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Sinan Kaya]

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.
  *
  * 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);
+	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);
+	}
+}
+
 static const struct acpi_device_id ged_acpi_ids[] = {
 	{"ACPI0013"},
 	{},
 };
 
 static struct platform_driver ged_driver = {
+	.shutdown = ged_remove,
 	.probe = ged_probe,
 	.driver = {
 		.name = MODULE_NAME,
-- 
1.9.1

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Sinan Kaya]

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.
  *
  * 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);
+	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);
+	}
+}
+
 static const struct acpi_device_id ged_acpi_ids[] = {
 	{"ACPI0013"},
 	{},
 };
 
 static struct platform_driver ged_driver = {
+	.shutdown = ged_remove,
 	.probe = ged_probe,
 	.driver = {
 		.name = MODULE_NAME,
-- 
1.9.1

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

[Rafael J. Wysocki]

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?

>   *
>   * 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

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Rafael J. Wysocki]

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?

>   *
>   * 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

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

[Rafael J. Wysocki]

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?

>   *
>   * 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

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

[Sinan Kaya]

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.

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Sinan Kaya]

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 at 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.

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

[Sinan Kaya]

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.

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

[Rafael J. Wysocki]

On Wed, Dec 6, 2017 at 2:24 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> 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.

[cut]

>
>>
>>>         .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?

Yes, it should, so I'm not sure why you need the list in the first place.

Also it looks like something along the lines of devres_release_all()
should be sufficient.

Thanks,
Rafael

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Rafael J. Wysocki]

On Wed, Dec 6, 2017 at 2:24 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> 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.

[cut]

>
>>
>>>         .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?

Yes, it should, so I'm not sure why you need the list in the first place.

Also it looks like something along the lines of devres_release_all()
should be sufficient.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Wed, Dec 6, 2017 at 2:24 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> 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.

[cut]

>
>>
>>>         .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?

Yes, it should, so I'm not sure why you need the list in the first place.

Also it looks like something along the lines of devres_release_all()
should be sufficient.

Thanks,
Rafael

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

[Sinan Kaya]

On 12/6/2017 8:37 AM, Rafael J. Wysocki wrote:
> On Wed, Dec 6, 2017 at 2:24 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> 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.
> 
> [cut]
> 
>>
>>>
>>>>         .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?
> 
> Yes, it should, so I'm not sure why you need the list in the first place.
> 
> Also it looks like something along the lines of devres_release_all()
> should be sufficient.

Good suggestion, let me test this.

> 
> 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.

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Sinan Kaya]

On 12/6/2017 8:37 AM, Rafael J. Wysocki wrote:
> On Wed, Dec 6, 2017 at 2:24 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> 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.
> 
> [cut]
> 
>>
>>>
>>>>         .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?
> 
> Yes, it should, so I'm not sure why you need the list in the first place.
> 
> Also it looks like something along the lines of devres_release_all()
> should be sufficient.

Good suggestion, let me test this.

> 
> Thanks,
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo at 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.

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

[Sinan Kaya]

On 12/6/2017 8:37 AM, Rafael J. Wysocki wrote:
> On Wed, Dec 6, 2017 at 2:24 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> 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.
> 
> [cut]
> 
>>
>>>
>>>>         .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?
> 
> Yes, it should, so I'm not sure why you need the list in the first place.
> 
> Also it looks like something along the lines of devres_release_all()
> should be sufficient.

Good suggestion, let me test this.

> 
> 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.

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

[Sinan Kaya]

On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>> Yes, it should, so I'm not sure why you need the list in the first place.
>>
>> Also it looks like something along the lines of devres_release_all()
>> should be sufficient.
> Good suggestion, let me test this.
> 

I tried to pull the code into GED but the API is not public. I also looked
at how it is used. I was afraid to mess up with the internals of base.c by
calling devres_release_all() externally first and by the driver framework
next. I moved away from this approach.

I just fixed the function rename and changed the dev_info() to dev_dbg().

-- 
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.

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Sinan Kaya]

On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>> Yes, it should, so I'm not sure why you need the list in the first place.
>>
>> Also it looks like something along the lines of devres_release_all()
>> should be sufficient.
> Good suggestion, let me test this.
> 

I tried to pull the code into GED but the API is not public. I also looked
at how it is used. I was afraid to mess up with the internals of base.c by
calling devres_release_all() externally first and by the driver framework
next. I moved away from this approach.

I just fixed the function rename and changed the dev_info() to dev_dbg().

-- 
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.

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

[Sinan Kaya]

On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>> Yes, it should, so I'm not sure why you need the list in the first place.
>>
>> Also it looks like something along the lines of devres_release_all()
>> should be sufficient.
> Good suggestion, let me test this.
> 

I tried to pull the code into GED but the API is not public. I also looked
at how it is used. I was afraid to mess up with the internals of base.c by
calling devres_release_all() externally first and by the driver framework
next. I moved away from this approach.

I just fixed the function rename and changed the dev_info() to dev_dbg().

-- 
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.

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

[Rafael J. Wysocki]

On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>>> Yes, it should, so I'm not sure why you need the list in the first place.
>>>
>>> Also it looks like something along the lines of devres_release_all()
>>> should be sufficient.
>> Good suggestion, let me test this.
>>
>
> I tried to pull the code into GED but the API is not public. I also looked
> at how it is used. I was afraid to mess up with the internals of base.c by
> calling devres_release_all() externally first and by the driver framework
> next. I moved away from this approach.

Are you sure it is called by the core in the shutdown path?

> I just fixed the function rename and changed the dev_info() to dev_dbg().

And why do you need the list?

ged_shutdown() will be called for every device for which ged_probe()
has been called.

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Rafael J. Wysocki]

On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>>> Yes, it should, so I'm not sure why you need the list in the first place.
>>>
>>> Also it looks like something along the lines of devres_release_all()
>>> should be sufficient.
>> Good suggestion, let me test this.
>>
>
> I tried to pull the code into GED but the API is not public. I also looked
> at how it is used. I was afraid to mess up with the internals of base.c by
> calling devres_release_all() externally first and by the driver framework
> next. I moved away from this approach.

Are you sure it is called by the core in the shutdown path?

> I just fixed the function rename and changed the dev_info() to dev_dbg().

And why do you need the list?

ged_shutdown() will be called for every device for which ged_probe()
has been called.

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

[Rafael J. Wysocki]

On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>>> Yes, it should, so I'm not sure why you need the list in the first place.
>>>
>>> Also it looks like something along the lines of devres_release_all()
>>> should be sufficient.
>> Good suggestion, let me test this.
>>
>
> I tried to pull the code into GED but the API is not public. I also looked
> at how it is used. I was afraid to mess up with the internals of base.c by
> calling devres_release_all() externally first and by the driver framework
> next. I moved away from this approach.

Are you sure it is called by the core in the shutdown path?

> I just fixed the function rename and changed the dev_info() to dev_dbg().

And why do you need the list?

ged_shutdown() will be called for every device for which ged_probe()
has been called.

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

[Sinan Kaya]

On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>>>> Yes, it should, so I'm not sure why you need the list in the first place.
>>>>
>>>> Also it looks like something along the lines of devres_release_all()
>>>> should be sufficient.
>>> Good suggestion, let me test this.
>>>
>>
>> I tried to pull the code into GED but the API is not public. I also looked
>> at how it is used. I was afraid to mess up with the internals of base.c by
>> calling devres_release_all() externally first and by the driver framework
>> next. I moved away from this approach.
> 
> Are you sure it is called by the core in the shutdown path?

Sorry, I was thinking about a general case not the shutdown path. If we open
this API and have device drivers call it from arbitrary places; then we could
be opening a new can of worms that show up during device driver removal.

If you feel confident (I'm not), I can pursue this approach too.

> 
>> I just fixed the function rename and changed the dev_info() to dev_dbg().
> 
> And why do you need the list?
> 
> ged_shutdown() will be called for every device for which ged_probe()
> has been called.

The problem I had here is that =the last argument is the context pointer when I
call devm_free_irq().

If I don't pass the same context I used during IRQ registration, then the IRQ
free API fails. That's why, I needed to keep the event pointer around until the
IRQ free is called.


-- 
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.

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Sinan Kaya]

On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>>>> Yes, it should, so I'm not sure why you need the list in the first place.
>>>>
>>>> Also it looks like something along the lines of devres_release_all()
>>>> should be sufficient.
>>> Good suggestion, let me test this.
>>>
>>
>> I tried to pull the code into GED but the API is not public. I also looked
>> at how it is used. I was afraid to mess up with the internals of base.c by
>> calling devres_release_all() externally first and by the driver framework
>> next. I moved away from this approach.
> 
> Are you sure it is called by the core in the shutdown path?

Sorry, I was thinking about a general case not the shutdown path. If we open
this API and have device drivers call it from arbitrary places; then we could
be opening a new can of worms that show up during device driver removal.

If you feel confident (I'm not), I can pursue this approach too.

> 
>> I just fixed the function rename and changed the dev_info() to dev_dbg().
> 
> And why do you need the list?
> 
> ged_shutdown() will be called for every device for which ged_probe()
> has been called.

The problem I had here is that =the last argument is the context pointer when I
call devm_free_irq().

If I don't pass the same context I used during IRQ registration, then the IRQ
free API fails. That's why, I needed to keep the event pointer around until the
IRQ free is called.


-- 
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.

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

[Sinan Kaya]

On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>>>> Yes, it should, so I'm not sure why you need the list in the first place.
>>>>
>>>> Also it looks like something along the lines of devres_release_all()
>>>> should be sufficient.
>>> Good suggestion, let me test this.
>>>
>>
>> I tried to pull the code into GED but the API is not public. I also looked
>> at how it is used. I was afraid to mess up with the internals of base.c by
>> calling devres_release_all() externally first and by the driver framework
>> next. I moved away from this approach.
> 
> Are you sure it is called by the core in the shutdown path?

Sorry, I was thinking about a general case not the shutdown path. If we open
this API and have device drivers call it from arbitrary places; then we could
be opening a new can of worms that show up during device driver removal.

If you feel confident (I'm not), I can pursue this approach too.

> 
>> I just fixed the function rename and changed the dev_info() to dev_dbg().
> 
> And why do you need the list?
> 
> ged_shutdown() will be called for every device for which ged_probe()
> has been called.

The problem I had here is that =the last argument is the context pointer when I
call devm_free_irq().

If I don't pass the same context I used during IRQ registration, then the IRQ
free API fails. That's why, I needed to keep the event pointer around until the
IRQ free is called.


-- 
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.

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

[Rafael J. Wysocki]

On Wed, Dec 6, 2017 at 5:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
>> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>>>>> Yes, it should, so I'm not sure why you need the list in the first place.
>>>>>
>>>>> Also it looks like something along the lines of devres_release_all()
>>>>> should be sufficient.
>>>> Good suggestion, let me test this.
>>>>
>>>
>>> I tried to pull the code into GED but the API is not public. I also looked
>>> at how it is used. I was afraid to mess up with the internals of base.c by
>>> calling devres_release_all() externally first and by the driver framework
>>> next. I moved away from this approach.
>>
>> Are you sure it is called by the core in the shutdown path?
>
> Sorry, I was thinking about a general case not the shutdown path. If we open
> this API and have device drivers call it from arbitrary places; then we could
> be opening a new can of worms that show up during device driver removal.
>
> If you feel confident (I'm not), I can pursue this approach too.
>
>>
>>> I just fixed the function rename and changed the dev_info() to dev_dbg().
>>
>> And why do you need the list?
>>
>> ged_shutdown() will be called for every device for which ged_probe()
>> has been called.
>
> The problem I had here is that =the last argument is the context pointer when I
> call devm_free_irq().
>
> If I don't pass the same context I used during IRQ registration, then the IRQ
> free API fails. That's why, I needed to keep the event pointer around until the
> IRQ free is called.

OK

Anyway, it looks like something is missing in the core.

You shouldn't really need to do all that dance in a driver.

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Rafael J. Wysocki]

On Wed, Dec 6, 2017 at 5:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
>> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>>>>> Yes, it should, so I'm not sure why you need the list in the first place.
>>>>>
>>>>> Also it looks like something along the lines of devres_release_all()
>>>>> should be sufficient.
>>>> Good suggestion, let me test this.
>>>>
>>>
>>> I tried to pull the code into GED but the API is not public. I also looked
>>> at how it is used. I was afraid to mess up with the internals of base.c by
>>> calling devres_release_all() externally first and by the driver framework
>>> next. I moved away from this approach.
>>
>> Are you sure it is called by the core in the shutdown path?
>
> Sorry, I was thinking about a general case not the shutdown path. If we open
> this API and have device drivers call it from arbitrary places; then we could
> be opening a new can of worms that show up during device driver removal.
>
> If you feel confident (I'm not), I can pursue this approach too.
>
>>
>>> I just fixed the function rename and changed the dev_info() to dev_dbg().
>>
>> And why do you need the list?
>>
>> ged_shutdown() will be called for every device for which ged_probe()
>> has been called.
>
> The problem I had here is that =the last argument is the context pointer when I
> call devm_free_irq().
>
> If I don't pass the same context I used during IRQ registration, then the IRQ
> free API fails. That's why, I needed to keep the event pointer around until the
> IRQ free is called.

OK

Anyway, it looks like something is missing in the core.

You shouldn't really need to do all that dance in a driver.

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

[Rafael J. Wysocki]

On Wed, Dec 6, 2017 at 5:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
>> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>>>>> Yes, it should, so I'm not sure why you need the list in the first place.
>>>>>
>>>>> Also it looks like something along the lines of devres_release_all()
>>>>> should be sufficient.
>>>> Good suggestion, let me test this.
>>>>
>>>
>>> I tried to pull the code into GED but the API is not public. I also looked
>>> at how it is used. I was afraid to mess up with the internals of base.c by
>>> calling devres_release_all() externally first and by the driver framework
>>> next. I moved away from this approach.
>>
>> Are you sure it is called by the core in the shutdown path?
>
> Sorry, I was thinking about a general case not the shutdown path. If we open
> this API and have device drivers call it from arbitrary places; then we could
> be opening a new can of worms that show up during device driver removal.
>
> If you feel confident (I'm not), I can pursue this approach too.
>
>>
>>> I just fixed the function rename and changed the dev_info() to dev_dbg().
>>
>> And why do you need the list?
>>
>> ged_shutdown() will be called for every device for which ged_probe()
>> has been called.
>
> The problem I had here is that =the last argument is the context pointer when I
> call devm_free_irq().
>
> If I don't pass the same context I used during IRQ registration, then the IRQ
> free API fails. That's why, I needed to keep the event pointer around until the
> IRQ free is called.

OK

Anyway, it looks like something is missing in the core.

You shouldn't really need to do all that dance in a driver.

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

[Rafael J. Wysocki]

Hi Greg,

On Wed, Dec 6, 2017 at 6:16 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Dec 6, 2017 at 5:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
>>> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>>>>>> Yes, it should, so I'm not sure why you need the list in the first place.
>>>>>>
>>>>>> Also it looks like something along the lines of devres_release_all()
>>>>>> should be sufficient.
>>>>> Good suggestion, let me test this.
>>>>>
>>>>
>>>> I tried to pull the code into GED but the API is not public. I also looked
>>>> at how it is used. I was afraid to mess up with the internals of base.c by
>>>> calling devres_release_all() externally first and by the driver framework
>>>> next. I moved away from this approach.
>>>
>>> Are you sure it is called by the core in the shutdown path?
>>
>> Sorry, I was thinking about a general case not the shutdown path. If we open
>> this API and have device drivers call it from arbitrary places; then we could
>> be opening a new can of worms that show up during device driver removal.

[cut]

>
> OK
>
> Anyway, it looks like something is missing in the core.
>
> You shouldn't really need to do all that dance in a driver.

We have a problem with the ACPI GED driver which essentially is a
platform driver requesting a number of interrupts and handling them by
dispatching a specific AML method.

It uses devm_request_threaded_irq() to request the interrupts, so it
doesn't need a ->remove() callback, but it turns out to need a
->shutdown() one to free all of these interrupts at the shutdown time.

While we can add a ->shutdown() callback to it, that essentially needs
to duplicate devres_release_all() somewhat.

Any suggestions what to do with that?

Thanks,
Rafael

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Rafael J. Wysocki]

Hi Greg,

On Wed, Dec 6, 2017 at 6:16 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Dec 6, 2017 at 5:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
>>> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>>>>>> Yes, it should, so I'm not sure why you need the list in the first place.
>>>>>>
>>>>>> Also it looks like something along the lines of devres_release_all()
>>>>>> should be sufficient.
>>>>> Good suggestion, let me test this.
>>>>>
>>>>
>>>> I tried to pull the code into GED but the API is not public. I also looked
>>>> at how it is used. I was afraid to mess up with the internals of base.c by
>>>> calling devres_release_all() externally first and by the driver framework
>>>> next. I moved away from this approach.
>>>
>>> Are you sure it is called by the core in the shutdown path?
>>
>> Sorry, I was thinking about a general case not the shutdown path. If we open
>> this API and have device drivers call it from arbitrary places; then we could
>> be opening a new can of worms that show up during device driver removal.

[cut]

>
> OK
>
> Anyway, it looks like something is missing in the core.
>
> You shouldn't really need to do all that dance in a driver.

We have a problem with the ACPI GED driver which essentially is a
platform driver requesting a number of interrupts and handling them by
dispatching a specific AML method.

It uses devm_request_threaded_irq() to request the interrupts, so it
doesn't need a ->remove() callback, but it turns out to need a
->shutdown() one to free all of these interrupts at the shutdown time.

While we can add a ->shutdown() callback to it, that essentially needs
to duplicate devres_release_all() somewhat.

Any suggestions what to do with that?

Thanks,
Rafael

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

[Rafael J. Wysocki]

Hi Greg,

On Wed, Dec 6, 2017 at 6:16 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Dec 6, 2017 at 5:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>> On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
>>> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>>>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
>>>>>> Yes, it should, so I'm not sure why you need the list in the first place.
>>>>>>
>>>>>> Also it looks like something along the lines of devres_release_all()
>>>>>> should be sufficient.
>>>>> Good suggestion, let me test this.
>>>>>
>>>>
>>>> I tried to pull the code into GED but the API is not public. I also looked
>>>> at how it is used. I was afraid to mess up with the internals of base.c by
>>>> calling devres_release_all() externally first and by the driver framework
>>>> next. I moved away from this approach.
>>>
>>> Are you sure it is called by the core in the shutdown path?
>>
>> Sorry, I was thinking about a general case not the shutdown path. If we open
>> this API and have device drivers call it from arbitrary places; then we could
>> be opening a new can of worms that show up during device driver removal.

[cut]

>
> OK
>
> Anyway, it looks like something is missing in the core.
>
> You shouldn't really need to do all that dance in a driver.

We have a problem with the ACPI GED driver which essentially is a
platform driver requesting a number of interrupts and handling them by
dispatching a specific AML method.

It uses devm_request_threaded_irq() to request the interrupts, so it
doesn't need a ->remove() callback, but it turns out to need a
->shutdown() one to free all of these interrupts at the shutdown time.

While we can add a ->shutdown() callback to it, that essentially needs
to duplicate devres_release_all() somewhat.

Any suggestions what to do with that?

Thanks,
Rafael

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

[Greg Kroah-Hartman]

On Thu, Dec 07, 2017 at 12:19:25AM +0100, Rafael J. Wysocki wrote:
> Hi Greg,
> 
> On Wed, Dec 6, 2017 at 6:16 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Dec 6, 2017 at 5:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> >> On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
> >>> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> >>>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
> >>>>>> Yes, it should, so I'm not sure why you need the list in the first place.
> >>>>>>
> >>>>>> Also it looks like something along the lines of devres_release_all()
> >>>>>> should be sufficient.
> >>>>> Good suggestion, let me test this.
> >>>>>
> >>>>
> >>>> I tried to pull the code into GED but the API is not public. I also looked
> >>>> at how it is used. I was afraid to mess up with the internals of base.c by
> >>>> calling devres_release_all() externally first and by the driver framework
> >>>> next. I moved away from this approach.
> >>>
> >>> Are you sure it is called by the core in the shutdown path?
> >>
> >> Sorry, I was thinking about a general case not the shutdown path. If we open
> >> this API and have device drivers call it from arbitrary places; then we could
> >> be opening a new can of worms that show up during device driver removal.
> 
> [cut]
> 
> >
> > OK
> >
> > Anyway, it looks like something is missing in the core.
> >
> > You shouldn't really need to do all that dance in a driver.
> 
> We have a problem with the ACPI GED driver which essentially is a
> platform driver requesting a number of interrupts and handling them by
> dispatching a specific AML method.
> 
> It uses devm_request_threaded_irq() to request the interrupts, so it
> doesn't need a ->remove() callback, but it turns out to need a
> ->shutdown() one to free all of these interrupts at the shutdown time.
> 
> While we can add a ->shutdown() callback to it, that essentially needs
> to duplicate devres_release_all() somewhat.
> 
> Any suggestions what to do with that?

Just don't use devm_request_threaded_irq()?  :)

Seriously, those are just "helper" functions if your code happens to
follow the pattern they provide, if not, then don't use them, it's not
that hard to provide the correct code to unwind things properly by "open
coding" this logic as needed.

The devm_*irq() functions are known for not being able to be used all of
the time for lots of shutdown and cleanup issues, this isn't the first
time it has happened, which is why we are very careful when taking
"cleanup" patches that use those functions.

thanks,

greg k-h

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Greg Kroah-Hartman]

On Thu, Dec 07, 2017 at 12:19:25AM +0100, Rafael J. Wysocki wrote:
> Hi Greg,
> 
> On Wed, Dec 6, 2017 at 6:16 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Dec 6, 2017 at 5:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> >> On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
> >>> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> >>>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
> >>>>>> Yes, it should, so I'm not sure why you need the list in the first place.
> >>>>>>
> >>>>>> Also it looks like something along the lines of devres_release_all()
> >>>>>> should be sufficient.
> >>>>> Good suggestion, let me test this.
> >>>>>
> >>>>
> >>>> I tried to pull the code into GED but the API is not public. I also looked
> >>>> at how it is used. I was afraid to mess up with the internals of base.c by
> >>>> calling devres_release_all() externally first and by the driver framework
> >>>> next. I moved away from this approach.
> >>>
> >>> Are you sure it is called by the core in the shutdown path?
> >>
> >> Sorry, I was thinking about a general case not the shutdown path. If we open
> >> this API and have device drivers call it from arbitrary places; then we could
> >> be opening a new can of worms that show up during device driver removal.
> 
> [cut]
> 
> >
> > OK
> >
> > Anyway, it looks like something is missing in the core.
> >
> > You shouldn't really need to do all that dance in a driver.
> 
> We have a problem with the ACPI GED driver which essentially is a
> platform driver requesting a number of interrupts and handling them by
> dispatching a specific AML method.
> 
> It uses devm_request_threaded_irq() to request the interrupts, so it
> doesn't need a ->remove() callback, but it turns out to need a
> ->shutdown() one to free all of these interrupts at the shutdown time.
> 
> While we can add a ->shutdown() callback to it, that essentially needs
> to duplicate devres_release_all() somewhat.
> 
> Any suggestions what to do with that?

Just don't use devm_request_threaded_irq()?  :)

Seriously, those are just "helper" functions if your code happens to
follow the pattern they provide, if not, then don't use them, it's not
that hard to provide the correct code to unwind things properly by "open
coding" this logic as needed.

The devm_*irq() functions are known for not being able to be used all of
the time for lots of shutdown and cleanup issues, this isn't the first
time it has happened, which is why we are very careful when taking
"cleanup" patches that use those functions.

thanks,

greg k-h

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

[Greg Kroah-Hartman]

On Thu, Dec 07, 2017 at 12:19:25AM +0100, Rafael J. Wysocki wrote:
> Hi Greg,
> 
> On Wed, Dec 6, 2017 at 6:16 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Dec 6, 2017 at 5:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> >> On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
> >>> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> >>>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
> >>>>>> Yes, it should, so I'm not sure why you need the list in the first place.
> >>>>>>
> >>>>>> Also it looks like something along the lines of devres_release_all()
> >>>>>> should be sufficient.
> >>>>> Good suggestion, let me test this.
> >>>>>
> >>>>
> >>>> I tried to pull the code into GED but the API is not public. I also looked
> >>>> at how it is used. I was afraid to mess up with the internals of base.c by
> >>>> calling devres_release_all() externally first and by the driver framework
> >>>> next. I moved away from this approach.
> >>>
> >>> Are you sure it is called by the core in the shutdown path?
> >>
> >> Sorry, I was thinking about a general case not the shutdown path. If we open
> >> this API and have device drivers call it from arbitrary places; then we could
> >> be opening a new can of worms that show up during device driver removal.
> 
> [cut]
> 
> >
> > OK
> >
> > Anyway, it looks like something is missing in the core.
> >
> > You shouldn't really need to do all that dance in a driver.
> 
> We have a problem with the ACPI GED driver which essentially is a
> platform driver requesting a number of interrupts and handling them by
> dispatching a specific AML method.
> 
> It uses devm_request_threaded_irq() to request the interrupts, so it
> doesn't need a ->remove() callback, but it turns out to need a
> ->shutdown() one to free all of these interrupts at the shutdown time.
> 
> While we can add a ->shutdown() callback to it, that essentially needs
> to duplicate devres_release_all() somewhat.
> 
> Any suggestions what to do with that?

Just don't use devm_request_threaded_irq()?  :)

Seriously, those are just "helper" functions if your code happens to
follow the pattern they provide, if not, then don't use them, it's not
that hard to provide the correct code to unwind things properly by "open
coding" this logic as needed.

The devm_*irq() functions are known for not being able to be used all of
the time for lots of shutdown and cleanup issues, this isn't the first
time it has happened, which is why we are very careful when taking
"cleanup" patches that use those functions.

thanks,

greg k-h

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

[Rafael J. Wysocki]

On Thursday, December 7, 2017 9:29:58 AM CET Greg Kroah-Hartman wrote:
> On Thu, Dec 07, 2017 at 12:19:25AM +0100, Rafael J. Wysocki wrote:
> > Hi Greg,
> > 
> > On Wed, Dec 6, 2017 at 6:16 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > On Wed, Dec 6, 2017 at 5:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> > >> On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
> > >>> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> > >>>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
> > >>>>>> Yes, it should, so I'm not sure why you need the list in the first place.
> > >>>>>>
> > >>>>>> Also it looks like something along the lines of devres_release_all()
> > >>>>>> should be sufficient.
> > >>>>> Good suggestion, let me test this.
> > >>>>>
> > >>>>
> > >>>> I tried to pull the code into GED but the API is not public. I also looked
> > >>>> at how it is used. I was afraid to mess up with the internals of base.c by
> > >>>> calling devres_release_all() externally first and by the driver framework
> > >>>> next. I moved away from this approach.
> > >>>
> > >>> Are you sure it is called by the core in the shutdown path?
> > >>
> > >> Sorry, I was thinking about a general case not the shutdown path. If we open
> > >> this API and have device drivers call it from arbitrary places; then we could
> > >> be opening a new can of worms that show up during device driver removal.
> > 
> > [cut]
> > 
> > >
> > > OK
> > >
> > > Anyway, it looks like something is missing in the core.
> > >
> > > You shouldn't really need to do all that dance in a driver.
> > 
> > We have a problem with the ACPI GED driver which essentially is a
> > platform driver requesting a number of interrupts and handling them by
> > dispatching a specific AML method.
> > 
> > It uses devm_request_threaded_irq() to request the interrupts, so it
> > doesn't need a ->remove() callback, but it turns out to need a
> > ->shutdown() one to free all of these interrupts at the shutdown time.
> > 
> > While we can add a ->shutdown() callback to it, that essentially needs
> > to duplicate devres_release_all() somewhat.
> > 
> > Any suggestions what to do with that?
> 
> Just don't use devm_request_threaded_irq()?  :)
> 
> Seriously, those are just "helper" functions if your code happens to
> follow the pattern they provide, if not, then don't use them, it's not
> that hard to provide the correct code to unwind things properly by "open
> coding" this logic as needed.
> 
> The devm_*irq() functions are known for not being able to be used all of
> the time for lots of shutdown and cleanup issues, this isn't the first
> time it has happened, which is why we are very careful when taking
> "cleanup" patches that use those functions.

I see, thanks for the clarification.

OK, we'll need to rework the driver somewhat, then.

Thanks,
Rafael

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Rafael J. Wysocki]

On Thursday, December 7, 2017 9:29:58 AM CET Greg Kroah-Hartman wrote:
> On Thu, Dec 07, 2017 at 12:19:25AM +0100, Rafael J. Wysocki wrote:
> > Hi Greg,
> > 
> > On Wed, Dec 6, 2017 at 6:16 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > On Wed, Dec 6, 2017 at 5:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> > >> On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
> > >>> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> > >>>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
> > >>>>>> Yes, it should, so I'm not sure why you need the list in the first place.
> > >>>>>>
> > >>>>>> Also it looks like something along the lines of devres_release_all()
> > >>>>>> should be sufficient.
> > >>>>> Good suggestion, let me test this.
> > >>>>>
> > >>>>
> > >>>> I tried to pull the code into GED but the API is not public. I also looked
> > >>>> at how it is used. I was afraid to mess up with the internals of base.c by
> > >>>> calling devres_release_all() externally first and by the driver framework
> > >>>> next. I moved away from this approach.
> > >>>
> > >>> Are you sure it is called by the core in the shutdown path?
> > >>
> > >> Sorry, I was thinking about a general case not the shutdown path. If we open
> > >> this API and have device drivers call it from arbitrary places; then we could
> > >> be opening a new can of worms that show up during device driver removal.
> > 
> > [cut]
> > 
> > >
> > > OK
> > >
> > > Anyway, it looks like something is missing in the core.
> > >
> > > You shouldn't really need to do all that dance in a driver.
> > 
> > We have a problem with the ACPI GED driver which essentially is a
> > platform driver requesting a number of interrupts and handling them by
> > dispatching a specific AML method.
> > 
> > It uses devm_request_threaded_irq() to request the interrupts, so it
> > doesn't need a ->remove() callback, but it turns out to need a
> > ->shutdown() one to free all of these interrupts at the shutdown time.
> > 
> > While we can add a ->shutdown() callback to it, that essentially needs
> > to duplicate devres_release_all() somewhat.
> > 
> > Any suggestions what to do with that?
> 
> Just don't use devm_request_threaded_irq()?  :)
> 
> Seriously, those are just "helper" functions if your code happens to
> follow the pattern they provide, if not, then don't use them, it's not
> that hard to provide the correct code to unwind things properly by "open
> coding" this logic as needed.
> 
> The devm_*irq() functions are known for not being able to be used all of
> the time for lots of shutdown and cleanup issues, this isn't the first
> time it has happened, which is why we are very careful when taking
> "cleanup" patches that use those functions.

I see, thanks for the clarification.

OK, we'll need to rework the driver somewhat, then.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Thursday, December 7, 2017 9:29:58 AM CET Greg Kroah-Hartman wrote:
> On Thu, Dec 07, 2017 at 12:19:25AM +0100, Rafael J. Wysocki wrote:
> > Hi Greg,
> > 
> > On Wed, Dec 6, 2017 at 6:16 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > On Wed, Dec 6, 2017 at 5:55 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> > >> On 12/6/2017 11:41 AM, Rafael J. Wysocki wrote:
> > >>> On Wed, Dec 6, 2017 at 5:11 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> > >>>> On 12/6/2017 9:57 AM, Sinan Kaya wrote:
> > >>>>>> Yes, it should, so I'm not sure why you need the list in the first place.
> > >>>>>>
> > >>>>>> Also it looks like something along the lines of devres_release_all()
> > >>>>>> should be sufficient.
> > >>>>> Good suggestion, let me test this.
> > >>>>>
> > >>>>
> > >>>> I tried to pull the code into GED but the API is not public. I also looked
> > >>>> at how it is used. I was afraid to mess up with the internals of base.c by
> > >>>> calling devres_release_all() externally first and by the driver framework
> > >>>> next. I moved away from this approach.
> > >>>
> > >>> Are you sure it is called by the core in the shutdown path?
> > >>
> > >> Sorry, I was thinking about a general case not the shutdown path. If we open
> > >> this API and have device drivers call it from arbitrary places; then we could
> > >> be opening a new can of worms that show up during device driver removal.
> > 
> > [cut]
> > 
> > >
> > > OK
> > >
> > > Anyway, it looks like something is missing in the core.
> > >
> > > You shouldn't really need to do all that dance in a driver.
> > 
> > We have a problem with the ACPI GED driver which essentially is a
> > platform driver requesting a number of interrupts and handling them by
> > dispatching a specific AML method.
> > 
> > It uses devm_request_threaded_irq() to request the interrupts, so it
> > doesn't need a ->remove() callback, but it turns out to need a
> > ->shutdown() one to free all of these interrupts at the shutdown time.
> > 
> > While we can add a ->shutdown() callback to it, that essentially needs
> > to duplicate devres_release_all() somewhat.
> > 
> > Any suggestions what to do with that?
> 
> Just don't use devm_request_threaded_irq()?  :)
> 
> Seriously, those are just "helper" functions if your code happens to
> follow the pattern they provide, if not, then don't use them, it's not
> that hard to provide the correct code to unwind things properly by "open
> coding" this logic as needed.
> 
> The devm_*irq() functions are known for not being able to be used all of
> the time for lots of shutdown and cleanup issues, this isn't the first
> time it has happened, which is why we are very careful when taking
> "cleanup" patches that use those functions.

I see, thanks for the clarification.

OK, we'll need to rework the driver somewhat, then.

Thanks,
Rafael

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

[Sinan Kaya]

Rafael,

On 12/7/2017 8:00 AM, Rafael J. Wysocki wrote:
>> Just don't use devm_request_threaded_irq()?  :)
>>
>> Seriously, those are just "helper" functions if your code happens to
>> follow the pattern they provide, if not, then don't use them, it's not
>> that hard to provide the correct code to unwind things properly by "open
>> coding" this logic as needed.
>>
>> The devm_*irq() functions are known for not being able to be used all of
>> the time for lots of shutdown and cleanup issues, this isn't the first
>> time it has happened, which is why we are very careful when taking
>> "cleanup" patches that use those functions.
> I see, thanks for the clarification.
> 
> OK, we'll need to rework the driver somewhat, then.

Even if we got rid of devm_*irq() functions, I see that the free_irq() function
requires dev_id argument.

	 * There can be multiple actions per IRQ descriptor, find the right
	 * one based on the dev_id:

I still need to keep track of the dev_ids attached to request_irq() functions. 

My take away from the discussion is:
1. don't use devm family of functions for IRQ registration/free
2. still keep track of the events
3. call free_irq on shutdown.

Do you have something else on your mind?

Sinan

-- 
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.

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Sinan Kaya]

Rafael,

On 12/7/2017 8:00 AM, Rafael J. Wysocki wrote:
>> Just don't use devm_request_threaded_irq()?  :)
>>
>> Seriously, those are just "helper" functions if your code happens to
>> follow the pattern they provide, if not, then don't use them, it's not
>> that hard to provide the correct code to unwind things properly by "open
>> coding" this logic as needed.
>>
>> The devm_*irq() functions are known for not being able to be used all of
>> the time for lots of shutdown and cleanup issues, this isn't the first
>> time it has happened, which is why we are very careful when taking
>> "cleanup" patches that use those functions.
> I see, thanks for the clarification.
> 
> OK, we'll need to rework the driver somewhat, then.

Even if we got rid of devm_*irq() functions, I see that the free_irq() function
requires dev_id argument.

	 * There can be multiple actions per IRQ descriptor, find the right
	 * one based on the dev_id:

I still need to keep track of the dev_ids attached to request_irq() functions. 

My take away from the discussion is:
1. don't use devm family of functions for IRQ registration/free
2. still keep track of the events
3. call free_irq on shutdown.

Do you have something else on your mind?

Sinan

-- 
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.

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

[Sinan Kaya]

Rafael,

On 12/7/2017 8:00 AM, Rafael J. Wysocki wrote:
>> Just don't use devm_request_threaded_irq()?  :)
>>
>> Seriously, those are just "helper" functions if your code happens to
>> follow the pattern they provide, if not, then don't use them, it's not
>> that hard to provide the correct code to unwind things properly by "open
>> coding" this logic as needed.
>>
>> The devm_*irq() functions are known for not being able to be used all of
>> the time for lots of shutdown and cleanup issues, this isn't the first
>> time it has happened, which is why we are very careful when taking
>> "cleanup" patches that use those functions.
> I see, thanks for the clarification.
> 
> OK, we'll need to rework the driver somewhat, then.

Even if we got rid of devm_*irq() functions, I see that the free_irq() function
requires dev_id argument.

	 * There can be multiple actions per IRQ descriptor, find the right
	 * one based on the dev_id:

I still need to keep track of the dev_ids attached to request_irq() functions. 

My take away from the discussion is:
1. don't use devm family of functions for IRQ registration/free
2. still keep track of the events
3. call free_irq on shutdown.

Do you have something else on your mind?

Sinan

-- 
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.

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

[Rafael J. Wysocki]

On Thu, Dec 7, 2017 at 3:52 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> Rafael,
>
> On 12/7/2017 8:00 AM, Rafael J. Wysocki wrote:
>>> Just don't use devm_request_threaded_irq()?  :)
>>>
>>> Seriously, those are just "helper" functions if your code happens to
>>> follow the pattern they provide, if not, then don't use them, it's not
>>> that hard to provide the correct code to unwind things properly by "open
>>> coding" this logic as needed.
>>>
>>> The devm_*irq() functions are known for not being able to be used all of
>>> the time for lots of shutdown and cleanup issues, this isn't the first
>>> time it has happened, which is why we are very careful when taking
>>> "cleanup" patches that use those functions.
>> I see, thanks for the clarification.
>>
>> OK, we'll need to rework the driver somewhat, then.
>
> Even if we got rid of devm_*irq() functions, I see that the free_irq() function
> requires dev_id argument.
>
>          * There can be multiple actions per IRQ descriptor, find the right
>          * one based on the dev_id:
>
> I still need to keep track of the dev_ids attached to request_irq() functions.

Right.

> My take away from the discussion is:
> 1. don't use devm family of functions for IRQ registration/free
> 2. still keep track of the events
> 3. call free_irq on shutdown.

Yes.  And on remove too.

Thanks,
Rafael

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Rafael J. Wysocki]

On Thu, Dec 7, 2017 at 3:52 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> Rafael,
>
> On 12/7/2017 8:00 AM, Rafael J. Wysocki wrote:
>>> Just don't use devm_request_threaded_irq()?  :)
>>>
>>> Seriously, those are just "helper" functions if your code happens to
>>> follow the pattern they provide, if not, then don't use them, it's not
>>> that hard to provide the correct code to unwind things properly by "open
>>> coding" this logic as needed.
>>>
>>> The devm_*irq() functions are known for not being able to be used all of
>>> the time for lots of shutdown and cleanup issues, this isn't the first
>>> time it has happened, which is why we are very careful when taking
>>> "cleanup" patches that use those functions.
>> I see, thanks for the clarification.
>>
>> OK, we'll need to rework the driver somewhat, then.
>
> Even if we got rid of devm_*irq() functions, I see that the free_irq() function
> requires dev_id argument.
>
>          * There can be multiple actions per IRQ descriptor, find the right
>          * one based on the dev_id:
>
> I still need to keep track of the dev_ids attached to request_irq() functions.

Right.

> My take away from the discussion is:
> 1. don't use devm family of functions for IRQ registration/free
> 2. still keep track of the events
> 3. call free_irq on shutdown.

Yes.  And on remove too.

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Thu, Dec 7, 2017 at 3:52 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> Rafael,
>
> On 12/7/2017 8:00 AM, Rafael J. Wysocki wrote:
>>> Just don't use devm_request_threaded_irq()?  :)
>>>
>>> Seriously, those are just "helper" functions if your code happens to
>>> follow the pattern they provide, if not, then don't use them, it's not
>>> that hard to provide the correct code to unwind things properly by "open
>>> coding" this logic as needed.
>>>
>>> The devm_*irq() functions are known for not being able to be used all of
>>> the time for lots of shutdown and cleanup issues, this isn't the first
>>> time it has happened, which is why we are very careful when taking
>>> "cleanup" patches that use those functions.
>> I see, thanks for the clarification.
>>
>> OK, we'll need to rework the driver somewhat, then.
>
> Even if we got rid of devm_*irq() functions, I see that the free_irq() function
> requires dev_id argument.
>
>          * There can be multiple actions per IRQ descriptor, find the right
>          * one based on the dev_id:
>
> I still need to keep track of the dev_ids attached to request_irq() functions.

Right.

> My take away from the discussion is:
> 1. don't use devm family of functions for IRQ registration/free
> 2. still keep track of the events
> 3. call free_irq on shutdown.

Yes.  And on remove too.

Thanks,
Rafael

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

[Sinan Kaya]

On 12/7/2017 12:10 PM, Rafael J. Wysocki wrote:
>> My take away from the discussion is:
>> 1. don't use devm family of functions for IRQ registration/free
>> 2. still keep track of the events
>> 3. call free_irq on shutdown.
> Yes.  And on remove too.

OK, will make the changes and post a newer version. Thanks for the
confirmation.

-- 
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.

[PATCH] ACPI / GED: unregister interrupts during shutdown

[Sinan Kaya]

On 12/7/2017 12:10 PM, Rafael J. Wysocki wrote:
>> My take away from the discussion is:
>> 1. don't use devm family of functions for IRQ registration/free
>> 2. still keep track of the events
>> 3. call free_irq on shutdown.
> Yes.  And on remove too.

OK, will make the changes and post a newer version. Thanks for the
confirmation.

-- 
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.

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

[Sinan Kaya]

On 12/7/2017 12:10 PM, Rafael J. Wysocki wrote:
>> My take away from the discussion is:
>> 1. don't use devm family of functions for IRQ registration/free
>> 2. still keep track of the events
>> 3. call free_irq on shutdown.
> Yes.  And on remove too.

OK, will make the changes and post a newer version. Thanks for the
confirmation.

-- 
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.