[PATCH] gpiolib-acpi: Only defer request_irq for GpioInt ACPI event handlers

[PATCH] gpiolib-acpi: Only defer request_irq for GpioInt ACPI event handlers

[Hans de Goede]

Commit 78d3a92edbfb ("gpiolib-acpi: Register GpioInt ACPI event handlers
from a late_initcall") deferred the entire acpi_gpiochip_request_interrupt
call for each event resource.

This means it also delays the gpiochip_request_own_desc(..., "ACPI:Event")
call. This is a problem if some AML code reads the GPIO pin before we
run the deferred acpi_gpiochip_request_interrupt, because in that case
acpi_gpio_adr_space_handler() will already have called
gpiochip_request_own_desc(..., "ACPI:OpRegion") causing the call from
acpi_gpiochip_request_interrupt to fail with -EBUSY and we will fail to
register an event handler.

acpi_gpio_adr_space_handler is prepared for acpi_gpiochip_request_interrupt
already having claimed the pin, but the other way around does not work.

One example of a problem this causes, is the event handler for the OTG
ID pin on a Prowise PT301 tablet not registering, keeping the port stuck
in whatever mode it was in during boot and e.g. only allowing charging
after a reboot.

This commit fixes this by only deferring the request_irq call and the
initial run of edge-triggered IRQs instead of deferring all of
acpi_gpiochip_request_interrupt.

Cc: stable@vger.kernel.org
Fixes: 78d3a92edbfb ("gpiolib-acpi: Register GpioInt ACPI event ...")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpiolib-acpi.c | 136 +++++++++++++++++++-----------------
 1 file changed, 73 insertions(+), 63 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 55b72fbe1631..bda19373835a 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -22,8 +22,12 @@
 struct acpi_gpio_event {
 	struct list_head node;
 	acpi_handle handle;
+	irq_handler_t handler;
 	unsigned int pin;
 	unsigned int irq;
+	unsigned long irqflags;
+	bool irq_is_wake;
+	bool irq_requested;
 	struct gpio_desc *desc;
 };
 
@@ -49,10 +53,10 @@ struct acpi_gpio_chip {
 
 /*
  * For gpiochips which call acpi_gpiochip_request_interrupts() before late_init
- * (so builtin drivers) we register the ACPI GpioInt event handlers from a
+ * (so builtin drivers) we register the ACPI GpioInt irq-handlers from a
  * late_initcall_sync handler, so that other builtin drivers can register their
  * OpRegions before the event handlers can run.  This list contains gpiochips
- * for which the acpi_gpiochip_request_interrupts() has been deferred.
+ * for which the acpi_gpiochip_request_irqs() call has been deferred.
  */
 static DEFINE_MUTEX(acpi_gpio_deferred_req_irqs_lock);
 static LIST_HEAD(acpi_gpio_deferred_req_irqs_list);
@@ -133,8 +137,43 @@ bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
 }
 EXPORT_SYMBOL_GPL(acpi_gpio_get_irq_resource);
 
-static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
-						   void *context)
+static void acpi_gpiochip_request_irq(struct acpi_gpio_chip *acpi_gpio,
+				      struct acpi_gpio_event *event)
+{
+	int ret, value;
+
+	ret = request_threaded_irq(event->irq, NULL, event->handler,
+				   event->irqflags, "ACPI:Event", event);
+	if (ret) {
+		dev_err(acpi_gpio->chip->parent,
+			"Failed to setup interrupt handler for %d\n",
+			event->irq);
+		return;
+	}
+
+	if (event->irq_is_wake)
+		enable_irq_wake(event->irq);
+
+	event->irq_requested = true;
+
+	/* Make sure we trigger the initial state of edge-triggered IRQs */
+	value = gpiod_get_raw_value_cansleep(event->desc);
+	if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
+	    ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
+		event->handler(event->irq, event);
+}
+
+static void acpi_gpiochip_request_irqs(struct acpi_gpio_chip *acpi_gpio)
+{
+	struct acpi_gpio_event *event;
+
+	list_for_each_entry(event, &acpi_gpio->events, node)
+		acpi_gpiochip_request_irq(acpi_gpio, event);
+}
+
+static acpi_status
+acpi_gpiochip_request_interrupt_desc(struct acpi_resource *ares,
+				     void *context)
 {
 	struct acpi_gpio_chip *acpi_gpio = context;
 	struct gpio_chip *chip = acpi_gpio->chip;
@@ -143,8 +182,7 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
 	struct acpi_gpio_event *event;
 	irq_handler_t handler = NULL;
 	struct gpio_desc *desc;
-	unsigned long irqflags;
-	int ret, pin, irq, value;
+	int ret, pin, irq;
 
 	if (!acpi_gpio_get_irq_resource(ares, &agpio))
 		return AE_OK;
@@ -175,8 +213,6 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
 
 	gpiod_direction_input(desc);
 
-	value = gpiod_get_value_cansleep(desc);
-
 	ret = gpiochip_lock_as_irq(chip, pin);
 	if (ret) {
 		dev_err(chip->parent, "Failed to lock GPIO as interrupt\n");
@@ -189,64 +225,42 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
 		goto fail_unlock_irq;
 	}
 
-	irqflags = IRQF_ONESHOT;
+	event = kzalloc(sizeof(*event), GFP_KERNEL);
+	if (!event)
+		goto fail_unlock_irq;
+
+	event->irqflags = IRQF_ONESHOT;
 	if (agpio->triggering == ACPI_LEVEL_SENSITIVE) {
 		if (agpio->polarity == ACPI_ACTIVE_HIGH)
-			irqflags |= IRQF_TRIGGER_HIGH;
+			event->irqflags |= IRQF_TRIGGER_HIGH;
 		else
-			irqflags |= IRQF_TRIGGER_LOW;
+			event->irqflags |= IRQF_TRIGGER_LOW;
 	} else {
 		switch (agpio->polarity) {
 		case ACPI_ACTIVE_HIGH:
-			irqflags |= IRQF_TRIGGER_RISING;
+			event->irqflags |= IRQF_TRIGGER_RISING;
 			break;
 		case ACPI_ACTIVE_LOW:
-			irqflags |= IRQF_TRIGGER_FALLING;
+			event->irqflags |= IRQF_TRIGGER_FALLING;
 			break;
 		default:
-			irqflags |= IRQF_TRIGGER_RISING |
-				    IRQF_TRIGGER_FALLING;
+			event->irqflags |= IRQF_TRIGGER_RISING |
+					   IRQF_TRIGGER_FALLING;
 			break;
 		}
 	}
 
-	event = kzalloc(sizeof(*event), GFP_KERNEL);
-	if (!event)
-		goto fail_unlock_irq;
-
 	event->handle = evt_handle;
+	event->handler = handler;
 	event->irq = irq;
+	event->irq_is_wake = agpio->wake_capable == ACPI_WAKE_CAPABLE;
 	event->pin = pin;
 	event->desc = desc;
 
-	ret = request_threaded_irq(event->irq, NULL, handler, irqflags,
-				   "ACPI:Event", event);
-	if (ret) {
-		dev_err(chip->parent,
-			"Failed to setup interrupt handler for %d\n",
-			event->irq);
-		goto fail_free_event;
-	}
-
-	if (agpio->wake_capable == ACPI_WAKE_CAPABLE)
-		enable_irq_wake(irq);
-
 	list_add_tail(&event->node, &acpi_gpio->events);
 
-	/*
-	 * Make sure we trigger the initial state of the IRQ when using RISING
-	 * or FALLING.  Note we run the handlers on late_init, the AML code
-	 * may refer to OperationRegions from other (builtin) drivers which
-	 * may be probed after us.
-	 */
-	if (((irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
-	    ((irqflags & IRQF_TRIGGER_FALLING) && value == 0))
-		handler(event->irq, event);
-
 	return AE_OK;
 
-fail_free_event:
-	kfree(event);
 fail_unlock_irq:
 	gpiochip_unlock_as_irq(chip, pin);
 fail_free_desc:
@@ -283,6 +297,9 @@ void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
 	if (ACPI_FAILURE(status))
 		return;
 
+	acpi_walk_resources(handle, "_AEI",
+			    acpi_gpiochip_request_interrupt_desc, acpi_gpio);
+
 	mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
 	defer = !acpi_gpio_deferred_req_irqs_done;
 	if (defer)
@@ -290,11 +307,8 @@ void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
 			 &acpi_gpio_deferred_req_irqs_list);
 	mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
 
-	if (defer)
-		return;
-
-	acpi_walk_resources(handle, "_AEI",
-			    acpi_gpiochip_request_interrupt, acpi_gpio);
+	if (!defer)
+		acpi_gpiochip_request_irqs(acpi_gpio);
 }
 EXPORT_SYMBOL_GPL(acpi_gpiochip_request_interrupts);
 
@@ -331,10 +345,13 @@ void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
 	list_for_each_entry_safe_reverse(event, ep, &acpi_gpio->events, node) {
 		struct gpio_desc *desc;
 
-		if (irqd_is_wakeup_set(irq_get_irq_data(event->irq)))
-			disable_irq_wake(event->irq);
+		if (event->irq_requested) {
+			if (event->irq_is_wake)
+				disable_irq_wake(event->irq);
+
+			free_irq(event->irq, event);
+		}
 
-		free_irq(event->irq, event);
 		desc = event->desc;
 		if (WARN_ON(IS_ERR(desc)))
 			continue;
@@ -1200,23 +1217,16 @@ bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id)
 	return con_id == NULL;
 }
 
-/* Run deferred acpi_gpiochip_request_interrupts() */
-static int acpi_gpio_handle_deferred_request_interrupts(void)
+/* Run deferred acpi_gpiochip_request_irqs() */
+static int acpi_gpio_handle_deferred_request_irqs(void)
 {
 	struct acpi_gpio_chip *acpi_gpio, *tmp;
 
 	mutex_lock(&acpi_gpio_deferred_req_irqs_lock);
 	list_for_each_entry_safe(acpi_gpio, tmp,
 				 &acpi_gpio_deferred_req_irqs_list,
-				 deferred_req_irqs_list_entry) {
-		acpi_handle handle;
-
-		handle = ACPI_HANDLE(acpi_gpio->chip->parent);
-		acpi_walk_resources(handle, "_AEI",
-				    acpi_gpiochip_request_interrupt, acpi_gpio);
-
-		list_del_init(&acpi_gpio->deferred_req_irqs_list_entry);
-	}
+				 deferred_req_irqs_list_entry)
+		acpi_gpiochip_request_irqs(acpi_gpio);
 
 	acpi_gpio_deferred_req_irqs_done = true;
 	mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
@@ -1224,4 +1234,4 @@ static int acpi_gpio_handle_deferred_request_interrupts(void)
 	return 0;
 }
 /* We must use _sync so that this runs after the first deferred_probe run */
-late_initcall_sync(acpi_gpio_handle_deferred_request_interrupts);
+late_initcall_sync(acpi_gpio_handle_deferred_request_irqs);
-- 
2.19.1

Re: [PATCH] gpiolib-acpi: Only defer request_irq for GpioInt ACPI event handlers

[Mika Westerberg]

On Mon, Nov 26, 2018 at 11:42:31AM +0100, Hans de Goede wrote:
> Commit 78d3a92edbfb ("gpiolib-acpi: Register GpioInt ACPI event handlers
> from a late_initcall") deferred the entire acpi_gpiochip_request_interrupt
> call for each event resource.
> 
> This means it also delays the gpiochip_request_own_desc(..., "ACPI:Event")
> call. This is a problem if some AML code reads the GPIO pin before we
> run the deferred acpi_gpiochip_request_interrupt, because in that case
> acpi_gpio_adr_space_handler() will already have called
> gpiochip_request_own_desc(..., "ACPI:OpRegion") causing the call from
> acpi_gpiochip_request_interrupt to fail with -EBUSY and we will fail to
> register an event handler.
> 
> acpi_gpio_adr_space_handler is prepared for acpi_gpiochip_request_interrupt
> already having claimed the pin, but the other way around does not work.
> 
> One example of a problem this causes, is the event handler for the OTG
> ID pin on a Prowise PT301 tablet not registering, keeping the port stuck
> in whatever mode it was in during boot and e.g. only allowing charging
> after a reboot.
> 
> This commit fixes this by only deferring the request_irq call and the
> initial run of edge-triggered IRQs instead of deferring all of
> acpi_gpiochip_request_interrupt.
> 
> Cc: stable@vger.kernel.org
> Fixes: 78d3a92edbfb ("gpiolib-acpi: Register GpioInt ACPI event ...")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 136 +++++++++++++++++++-----------------
>  1 file changed, 73 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 55b72fbe1631..bda19373835a 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -22,8 +22,12 @@
>  struct acpi_gpio_event {
>  	struct list_head node;
>  	acpi_handle handle;
> +	irq_handler_t handler;
>  	unsigned int pin;
>  	unsigned int irq;
> +	unsigned long irqflags;
> +	bool irq_is_wake;
> +	bool irq_requested;

It might be a good idea to add kernel-doc now for the struct since it
seems to be growing and it is not immediately clear what each field mean
anymore ;-)

>  	struct gpio_desc *desc;
>  };
>  
> @@ -49,10 +53,10 @@ struct acpi_gpio_chip {
>  
>  /*
>   * For gpiochips which call acpi_gpiochip_request_interrupts() before late_init
> - * (so builtin drivers) we register the ACPI GpioInt event handlers from a
> + * (so builtin drivers) we register the ACPI GpioInt irq-handlers from a
>   * late_initcall_sync handler, so that other builtin drivers can register their
>   * OpRegions before the event handlers can run.  This list contains gpiochips
> - * for which the acpi_gpiochip_request_interrupts() has been deferred.
> + * for which the acpi_gpiochip_request_irqs() call has been deferred.
>   */
>  static DEFINE_MUTEX(acpi_gpio_deferred_req_irqs_lock);
>  static LIST_HEAD(acpi_gpio_deferred_req_irqs_list);
> @@ -133,8 +137,43 @@ bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
>  }
>  EXPORT_SYMBOL_GPL(acpi_gpio_get_irq_resource);
>  
> -static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
> -						   void *context)
> +static void acpi_gpiochip_request_irq(struct acpi_gpio_chip *acpi_gpio,
> +				      struct acpi_gpio_event *event)
> +{
> +	int ret, value;
> +
> +	ret = request_threaded_irq(event->irq, NULL, event->handler,
> +				   event->irqflags, "ACPI:Event", event);
> +	if (ret) {
> +		dev_err(acpi_gpio->chip->parent,
> +			"Failed to setup interrupt handler for %d\n",
> +			event->irq);
> +		return;
> +	}
> +
> +	if (event->irq_is_wake)
> +		enable_irq_wake(event->irq);
> +
> +	event->irq_requested = true;
> +
> +	/* Make sure we trigger the initial state of edge-triggered IRQs */
> +	value = gpiod_get_raw_value_cansleep(event->desc);
> +	if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
> +	    ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
> +		event->handler(event->irq, event);
> +}
> +
> +static void acpi_gpiochip_request_irqs(struct acpi_gpio_chip *acpi_gpio)
> +{
> +	struct acpi_gpio_event *event;
> +
> +	list_for_each_entry(event, &acpi_gpio->events, node)
> +		acpi_gpiochip_request_irq(acpi_gpio, event);
> +}
> +
> +static acpi_status
> +acpi_gpiochip_request_interrupt_desc(struct acpi_resource *ares,
> +				     void *context)

I wonder if there could be better name for this now that it does not
request anything? Maybe acpi_gpiochip_event_alloc() or similar?

Re: [PATCH] gpiolib-acpi: Only defer request_irq for GpioInt ACPI event handlers

[Andy Shevchenko]

On Mon, Nov 26, 2018 at 11:42:31AM +0100, Hans de Goede wrote:
> Commit 78d3a92edbfb ("gpiolib-acpi: Register GpioInt ACPI event handlers
> from a late_initcall") deferred the entire acpi_gpiochip_request_interrupt
> call for each event resource.
> 
> This means it also delays the gpiochip_request_own_desc(..., "ACPI:Event")
> call. This is a problem if some AML code reads the GPIO pin before we
> run the deferred acpi_gpiochip_request_interrupt, because in that case
> acpi_gpio_adr_space_handler() will already have called
> gpiochip_request_own_desc(..., "ACPI:OpRegion") causing the call from
> acpi_gpiochip_request_interrupt to fail with -EBUSY and we will fail to
> register an event handler.
> 
> acpi_gpio_adr_space_handler is prepared for acpi_gpiochip_request_interrupt
> already having claimed the pin, but the other way around does not work.
> 
> One example of a problem this causes, is the event handler for the OTG
> ID pin on a Prowise PT301 tablet not registering, keeping the port stuck
> in whatever mode it was in during boot and e.g. only allowing charging
> after a reboot.
> 
> This commit fixes this by only deferring the request_irq call and the
> initial run of edge-triggered IRQs instead of deferring all of
> acpi_gpiochip_request_interrupt.

Thanks for a patch with such nice description.
I have couple of nitpicks and one question below.


>   * For gpiochips which call acpi_gpiochip_request_interrupts() before late_init
> - * (so builtin drivers) we register the ACPI GpioInt event handlers from a
> + * (so builtin drivers) we register the ACPI GpioInt irq-handlers from a

I would rather spell "IRQ handlers"

>   * late_initcall_sync handler, so that other builtin drivers can register their
>   * OpRegions before the event handlers can run.  This list contains gpiochips
> - * for which the acpi_gpiochip_request_interrupts() has been deferred.
> + * for which the acpi_gpiochip_request_irqs() call has been deferred.

>  	mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
>  
> -	if (defer)
> -		return;
> -
> -	acpi_walk_resources(handle, "_AEI",
> -			    acpi_gpiochip_request_interrupt, acpi_gpio);
> +	if (!defer)
> +		acpi_gpiochip_request_irqs(acpi_gpio);

Can we leave above condition and put here just

	acpi_gpiochip_request_irqs(acpi_gpio);

?

>  	list_for_each_entry_safe_reverse(event, ep, &acpi_gpio->events, node) {
>  		struct gpio_desc *desc;
>  
> +		if (event->irq_requested) {
> +			if (event->irq_is_wake)
> +				disable_irq_wake(event->irq);
> +
> +			free_irq(event->irq, event);
> +		}
>  


>  		desc = event->desc;
>  		if (WARN_ON(IS_ERR(desc)))

I don't remember if I asked already about possible memory leak here.
Am I reading code right?

>  			continue;

Reading more code, I'm even not sure if this condition could ever happen.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] gpiolib-acpi: Only defer request_irq for GpioInt ACPI event handlers

[Hans de Goede]

Hi,

On 26-11-18 13:39, Mika Westerberg wrote:
> On Mon, Nov 26, 2018 at 11:42:31AM +0100, Hans de Goede wrote:
>> Commit 78d3a92edbfb ("gpiolib-acpi: Register GpioInt ACPI event handlers
>> from a late_initcall") deferred the entire acpi_gpiochip_request_interrupt
>> call for each event resource.
>>
>> This means it also delays the gpiochip_request_own_desc(..., "ACPI:Event")
>> call. This is a problem if some AML code reads the GPIO pin before we
>> run the deferred acpi_gpiochip_request_interrupt, because in that case
>> acpi_gpio_adr_space_handler() will already have called
>> gpiochip_request_own_desc(..., "ACPI:OpRegion") causing the call from
>> acpi_gpiochip_request_interrupt to fail with -EBUSY and we will fail to
>> register an event handler.
>>
>> acpi_gpio_adr_space_handler is prepared for acpi_gpiochip_request_interrupt
>> already having claimed the pin, but the other way around does not work.
>>
>> One example of a problem this causes, is the event handler for the OTG
>> ID pin on a Prowise PT301 tablet not registering, keeping the port stuck
>> in whatever mode it was in during boot and e.g. only allowing charging
>> after a reboot.
>>
>> This commit fixes this by only deferring the request_irq call and the
>> initial run of edge-triggered IRQs instead of deferring all of
>> acpi_gpiochip_request_interrupt.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 78d3a92edbfb ("gpiolib-acpi: Register GpioInt ACPI event ...")
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/gpio/gpiolib-acpi.c | 136 +++++++++++++++++++-----------------
>>   1 file changed, 73 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
>> index 55b72fbe1631..bda19373835a 100644
>> --- a/drivers/gpio/gpiolib-acpi.c
>> +++ b/drivers/gpio/gpiolib-acpi.c
>> @@ -22,8 +22,12 @@
>>   struct acpi_gpio_event {
>>   	struct list_head node;
>>   	acpi_handle handle;
>> +	irq_handler_t handler;
>>   	unsigned int pin;
>>   	unsigned int irq;
>> +	unsigned long irqflags;
>> +	bool irq_is_wake;
>> +	bool irq_requested;
> 
> It might be a good idea to add kernel-doc now for the struct since it
> seems to be growing and it is not immediately clear what each field mean
> anymore ;-)

Ok, will do for v2.

>>   	struct gpio_desc *desc;
>>   };
>>   
>> @@ -49,10 +53,10 @@ struct acpi_gpio_chip {
>>   
>>   /*
>>    * For gpiochips which call acpi_gpiochip_request_interrupts() before late_init
>> - * (so builtin drivers) we register the ACPI GpioInt event handlers from a
>> + * (so builtin drivers) we register the ACPI GpioInt irq-handlers from a
>>    * late_initcall_sync handler, so that other builtin drivers can register their
>>    * OpRegions before the event handlers can run.  This list contains gpiochips
>> - * for which the acpi_gpiochip_request_interrupts() has been deferred.
>> + * for which the acpi_gpiochip_request_irqs() call has been deferred.
>>    */
>>   static DEFINE_MUTEX(acpi_gpio_deferred_req_irqs_lock);
>>   static LIST_HEAD(acpi_gpio_deferred_req_irqs_list);
>> @@ -133,8 +137,43 @@ bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
>>   }
>>   EXPORT_SYMBOL_GPL(acpi_gpio_get_irq_resource);
>>   
>> -static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
>> -						   void *context)
>> +static void acpi_gpiochip_request_irq(struct acpi_gpio_chip *acpi_gpio,
>> +				      struct acpi_gpio_event *event)
>> +{
>> +	int ret, value;
>> +
>> +	ret = request_threaded_irq(event->irq, NULL, event->handler,
>> +				   event->irqflags, "ACPI:Event", event);
>> +	if (ret) {
>> +		dev_err(acpi_gpio->chip->parent,
>> +			"Failed to setup interrupt handler for %d\n",
>> +			event->irq);
>> +		return;
>> +	}
>> +
>> +	if (event->irq_is_wake)
>> +		enable_irq_wake(event->irq);
>> +
>> +	event->irq_requested = true;
>> +
>> +	/* Make sure we trigger the initial state of edge-triggered IRQs */
>> +	value = gpiod_get_raw_value_cansleep(event->desc);
>> +	if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
>> +	    ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
>> +		event->handler(event->irq, event);
>> +}
>> +
>> +static void acpi_gpiochip_request_irqs(struct acpi_gpio_chip *acpi_gpio)
>> +{
>> +	struct acpi_gpio_event *event;
>> +
>> +	list_for_each_entry(event, &acpi_gpio->events, node)
>> +		acpi_gpiochip_request_irq(acpi_gpio, event);
>> +}
>> +
>> +static acpi_status
>> +acpi_gpiochip_request_interrupt_desc(struct acpi_resource *ares,
>> +				     void *context)
> 
> I wonder if there could be better name for this now that it does not
> request anything? Maybe acpi_gpiochip_event_alloc() or similar?

Sure I will rename this to acpi_gpiochip_alloc_event (verb then subject
to follow the style of the rest of the file) for v2.

I will prepare a v2 of this patch this evening or tomorrow.

Regards,

Hans

Re: [PATCH] gpiolib-acpi: Only defer request_irq for GpioInt ACPI event handlers

[Hans de Goede]

Hi,

On 26-11-18 13:53, Andy Shevchenko wrote:
> On Mon, Nov 26, 2018 at 11:42:31AM +0100, Hans de Goede wrote:
>> Commit 78d3a92edbfb ("gpiolib-acpi: Register GpioInt ACPI event handlers
>> from a late_initcall") deferred the entire acpi_gpiochip_request_interrupt
>> call for each event resource.
>>
>> This means it also delays the gpiochip_request_own_desc(..., "ACPI:Event")
>> call. This is a problem if some AML code reads the GPIO pin before we
>> run the deferred acpi_gpiochip_request_interrupt, because in that case
>> acpi_gpio_adr_space_handler() will already have called
>> gpiochip_request_own_desc(..., "ACPI:OpRegion") causing the call from
>> acpi_gpiochip_request_interrupt to fail with -EBUSY and we will fail to
>> register an event handler.
>>
>> acpi_gpio_adr_space_handler is prepared for acpi_gpiochip_request_interrupt
>> already having claimed the pin, but the other way around does not work.
>>
>> One example of a problem this causes, is the event handler for the OTG
>> ID pin on a Prowise PT301 tablet not registering, keeping the port stuck
>> in whatever mode it was in during boot and e.g. only allowing charging
>> after a reboot.
>>
>> This commit fixes this by only deferring the request_irq call and the
>> initial run of edge-triggered IRQs instead of deferring all of
>> acpi_gpiochip_request_interrupt.
> 
> Thanks for a patch with such nice description.
> I have couple of nitpicks and one question below.
> 
> 
>>    * For gpiochips which call acpi_gpiochip_request_interrupts() before late_init
>> - * (so builtin drivers) we register the ACPI GpioInt event handlers from a
>> + * (so builtin drivers) we register the ACPI GpioInt irq-handlers from a
> 
> I would rather spell "IRQ handlers"

Ok.

>>    * late_initcall_sync handler, so that other builtin drivers can register their
>>    * OpRegions before the event handlers can run.  This list contains gpiochips
>> - * for which the acpi_gpiochip_request_interrupts() has been deferred.
>> + * for which the acpi_gpiochip_request_irqs() call has been deferred.
> 
>>   	mutex_unlock(&acpi_gpio_deferred_req_irqs_lock);
>>   
>> -	if (defer)
>> -		return;
>> -
>> -	acpi_walk_resources(handle, "_AEI",
>> -			    acpi_gpiochip_request_interrupt, acpi_gpio);
>> +	if (!defer)
>> +		acpi_gpiochip_request_irqs(acpi_gpio);
> 
> Can we leave above condition and put here just

Ok.

> 	acpi_gpiochip_request_irqs(acpi_gpio);
> 
> ?
> 
>>   	list_for_each_entry_safe_reverse(event, ep, &acpi_gpio->events, node) {
>>   		struct gpio_desc *desc;
>>   
>> +		if (event->irq_requested) {
>> +			if (event->irq_is_wake)
>> +				disable_irq_wake(event->irq);
>> +
>> +			free_irq(event->irq, event);
>> +		}
>>   
> 
> 
>>   		desc = event->desc;
>>   		if (WARN_ON(IS_ERR(desc)))
> 
> I don't remember if I asked already about possible memory leak here.
> Am I reading code right?
> 
>>   			continue;
> 
> Reading more code, I'm even not sure if this condition could ever happen.

You're right an event with an IS_ERR(desc) is never added to the list
of events, so this condition can never happen. I will add a follow up
patch to v2 removing this unnecessary check.

I will prepare a v2 of this patch this evening or tomorrow.

Regards,

Hans