linux-kernel - Re: [PATCH 4/6] gpio / ACPI: Embed events list directly into struct acpi_gpio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <46469934.18k5deNECp@vostro.rjw.lan>
Date:	Tue, 25 Feb 2014 15:26:12 +0100
From:	"Rafael J. Wysocki" <rjw@...ysocki.net>
To:	Mika Westerberg <mika.westerberg@...ux.intel.com>
Cc:	Linus Walleij <linus.walleij@...aro.org>,
	Alexandre Courbot <gnurou@...il.com>,
	Lan Tianyu <tianyu.lan@...el.com>,
	Lv Zheng <lv.zheng@...el.com>, Alan Cox <alan.cox@...el.com>,
	Mathias Nyman <mathias.nyman@...ux.intel.com>,
	linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/6] gpio / ACPI: Embed events list directly into struct acpi_gpio_chip

On Monday, February 24, 2014 06:00:09 PM Mika Westerberg wrote:
> It is not necessary to have events as a pointer to list in struct
> acpi_gpio_chip. Instead we can embed the list_head directly to struct
> acpi_gpio_chip itself. This makes event handling a bit simpler because now
> we don't need to check whether the pointer is NULL or not.

It looks like I should have reviewed the whole series before commenting [2/6].

Well, my modified comment would be "Why don't you fold this one into [2/6]?", then.

> Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 35 +++++++++++++----------------------
>  1 file changed, 13 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index cb99dc2c552e..c60db4ddc166 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -28,7 +28,7 @@ struct acpi_gpio_event {
>  
>  struct acpi_gpio_chip {
>  	struct gpio_chip *chip;
> -	struct list_head *events;
> +	struct list_head events;
>  };
>  
>  static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
> @@ -106,8 +106,7 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
>  	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
>  	struct gpio_chip *chip = achip->chip;
>  	struct acpi_resource *res;
> -	acpi_handle handle, evt_handle;
> -	struct list_head *events = NULL;
> +	acpi_handle handle;
>  	acpi_status status;
>  	unsigned int pin;
>  	int irq, ret;
> @@ -120,19 +119,12 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
>  	if (!handle)
>  		return;
>  
> +	INIT_LIST_HEAD(&achip->events);
> +
>  	status = acpi_get_event_resources(handle, &buf);
>  	if (ACPI_FAILURE(status))
>  		return;
>  
> -	status = acpi_get_handle(handle, "_EVT", &evt_handle);
> -	if (ACPI_SUCCESS(status)) {
> -		events = kzalloc(sizeof(*events), GFP_KERNEL);
> -		if (events) {
> -			INIT_LIST_HEAD(events);
> -			achip->events = events;
> -		}
> -	}
> -
>  	/*
>  	 * If a GPIO interrupt has an ACPI event handler method, or _EVT is
>  	 * present, set up an interrupt handler that calls the ACPI event
> @@ -168,14 +160,19 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
>  				data = ev_handle;
>  			}
>  		}
> -		if (!handler && events) {
> +		if (!handler) {
>  			struct acpi_gpio_event *event;
> +			acpi_handle evt_handle;
> +
> +			status = acpi_get_handle(handle, "_EVT", &evt_handle);
> +			if (ACPI_FAILURE(status))
> +				continue;
>  
>  			event = kzalloc(sizeof(*event), GFP_KERNEL);
>  			if (!event)
>  				continue;
>  
> -			list_add_tail(&event->node, events);
> +			list_add_tail(&event->node, &achip->events);
>  			event->evt_handle = evt_handle;
>  			event->pin = pin;
>  			event->irq = irq;
> @@ -207,23 +204,17 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
>   */
>  static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
>  {
> -	struct list_head *events;
>  	struct acpi_gpio_event *event, *ep;
>  	struct gpio_chip *chip = achip->chip;
>  
> -	if (!chip->dev || !chip->to_irq || !achip->events)
> +	if (!chip->dev || !chip->to_irq)
>  		return;
>  
> -	events = achip->events;
> -	achip->events = NULL;
> -
> -	list_for_each_entry_safe_reverse(event, ep, events, node) {
> +	list_for_each_entry_safe_reverse(event, ep, &achip->events, node) {
>  		devm_free_irq(chip->dev, event->irq, event);
>  		list_del(&event->node);
>  		kfree(event);
>  	}
> -
> -	kfree(events);
>  }
>  
>  struct acpi_gpio_lookup {
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/