[PATCH] gpiolib: fix issue of gpio acpi handler

[PATCH] gpiolib: fix issue of gpio acpi handler

[qipeng.zha]

From: "qipeng.zha" <qipeng.zha@intel.com>

If GPIO driver use pin mapping, need to tranlate pin number
between ACPI table and GPIO driver.

This issue is found on one platform with Cherryview gpio
controller, kernel is hang when executed _PS0 method of
one ACPI device, since without this translation, it access
invalid gpiodesc array.

Verified it works again with this patch.

Signed-off-by: qipeng.zha <qipeng.zha@intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index c0929d9..661e272 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -551,6 +551,13 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 		struct gpio_desc *desc;
 		bool found;
 
+		/*Translate ACPI gpio pin to offset in gpio driver*/
+		pin = acpi_gpiochip_pin_to_gpio_offset(chip, pin);
+		if (pin < 0) {
+			status = AE_BAD_PARAMETER;
+			goto out;
+		}
+
 		mutex_lock(&achip->conn_lock);
 
 		found = false;
-- 
1.8.3.2

Re: [PATCH] gpiolib: fix issue of gpio acpi handler

[Mika Westerberg]

On Fri, Mar 06, 2015 at 11:08:38PM +0800, qipeng.zha wrote:
> From: "qipeng.zha" <qipeng.zha@intel.com>
> 
> If GPIO driver use pin mapping, need to tranlate pin number
                                          ^^^^^^^^
typo, should be "translate"

> between ACPI table and GPIO driver.
> 
> This issue is found on one platform with Cherryview gpio
> controller, kernel is hang when executed _PS0 method of
> one ACPI device, since without this translation, it access
> invalid gpiodesc array.
> 
> Verified it works again with this patch.
> 
> Signed-off-by: qipeng.zha <qipeng.zha@intel.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index c0929d9..661e272 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -551,6 +551,13 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  		struct gpio_desc *desc;
>  		bool found;
>  
> +		/*Translate ACPI gpio pin to offset in gpio driver*/

/* Comments look like this */

notice that there is space between /* and beginning of the sentence.

But this comment does not add any new information so I would just drop
it.

Otherwise this patch seems right to me.

BTW, I wonder if we need to do the same in
acpi_gpiochip_request_interrupt()?

> +		pin = acpi_gpiochip_pin_to_gpio_offset(chip, pin);
> +		if (pin < 0) {
> +			status = AE_BAD_PARAMETER;
> +			goto out;
> +		}
> +
>  		mutex_lock(&achip->conn_lock);
>  
>  		found = false;
> -- 
> 1.8.3.2

Re: [PATCH] gpiolib: fix issue of gpio acpi handler

[Mika Westerberg]

On Mon, Mar 09, 2015 at 01:12:03PM +0200, Mika Westerberg wrote:
> On Fri, Mar 06, 2015 at 11:08:38PM +0800, qipeng.zha wrote:
> > From: "qipeng.zha" <qipeng.zha@intel.com>
> > 
> > If GPIO driver use pin mapping, need to tranlate pin number
>                                           ^^^^^^^^
> typo, should be "translate"
> 
> > between ACPI table and GPIO driver.
> > 
> > This issue is found on one platform with Cherryview gpio
> > controller, kernel is hang when executed _PS0 method of
> > one ACPI device, since without this translation, it access
> > invalid gpiodesc array.
> > 
> > Verified it works again with this patch.
> > 
> > Signed-off-by: qipeng.zha <qipeng.zha@intel.com>
> > ---
> >  drivers/gpio/gpiolib-acpi.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > index c0929d9..661e272 100644
> > --- a/drivers/gpio/gpiolib-acpi.c
> > +++ b/drivers/gpio/gpiolib-acpi.c
> > @@ -551,6 +551,13 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
> >  		struct gpio_desc *desc;
> >  		bool found;
> >  
> > +		/*Translate ACPI gpio pin to offset in gpio driver*/
> 
> /* Comments look like this */
> 
> notice that there is space between /* and beginning of the sentence.
> 
> But this comment does not add any new information so I would just drop
> it.
> 
> Otherwise this patch seems right to me.

One more thing, though. The title of the patch should be more
descriptive. So that you get the idea what the patch is fixing when you
run "git log --oneline".