[PATCH] gpiolib: tighten up ACPI legacy gpio lookups

[PATCH] gpiolib: tighten up ACPI legacy gpio lookups

[Dmitry Torokhov]

We should not fall back to the legacy unnamed gpio lookup style if the
driver requests gpios with different names, because we'll give out the same
gpio twice. Let's keep track of the names that were used for the device and
only do the fallback for the first name used.

Also disable fallback to _CRS gpios if ACPI device has DT-like
properties or driver-supplied gpio mappings.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

This version incorporates changes suggested by Mika Westerberg in
response to the draft patch I posted in Goodix thread.

 drivers/gpio/gpiolib.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a18f00f..9631ee8 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1841,6 +1841,50 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 	return desc;
 }
 
+struct acpi_gpio_lookup {
+	struct list_head node;
+	struct acpi_device *adev;
+	const char *con_id;
+};
+
+static DEFINE_MUTEX(acpi_gpio_lookup_lock);
+static LIST_HEAD(acpi_gpio_lookup_list);
+
+static bool acpi_can_fallback_to_crs(struct acpi_device *adev,
+				     const char *con_id)
+{
+	struct acpi_gpio_lookup *l, *lookup = NULL;
+
+	/* Never fallback if the device has properties */
+	if (adev->data.properties || adev->driver_gpios)
+		return false;
+
+	mutex_lock(&acpi_gpio_lookup_lock);
+
+	list_for_each_entry(l, &acpi_gpio_lookup_list, node) {
+		if (l->adev == adev) {
+			lookup = l;
+			break;
+		}
+	}
+
+	if (!lookup) {
+		lookup = kmalloc(sizeof(*lookup), GFP_KERNEL);
+		if (lookup) {
+			lookup->adev = adev;
+			lookup->con_id = con_id;
+			list_add_tail(&lookup->node, &acpi_gpio_lookup_list);
+		}
+	}
+
+	mutex_unlock(&acpi_gpio_lookup_lock);
+
+	return lookup &&
+		((!lookup->con_id && !con_id) ||
+		 (lookup->con_id && con_id &&
+		  strcmp(lookup->con_id, con_id) == 0));
+}
+
 static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
 					unsigned int idx,
 					enum gpio_lookup_flags *flags)
@@ -1868,6 +1912,9 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
 
 	/* Then from plain _CRS GPIOs */
 	if (IS_ERR(desc)) {
+		if (!acpi_can_fallback_to_crs(adev, con_id))
+			return ERR_PTR(-ENOENT);
+
 		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
 		if (IS_ERR(desc))
 			return desc;
-- 
2.6.0.rc2.230.g3dd15c0


-- 
Dmitry

Re: [PATCH] gpiolib: tighten up ACPI legacy gpio lookups

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1444 bytes --]

Hi Dmitry,

[auto build test ERROR on: gpio/for-next]
[also build test ERROR on: v4.3 next-20151105]

url:    https://github.com/0day-ci/linux/commits/Dmitry-Torokhov/gpiolib-tighten-up-ACPI-legacy-gpio-lookups/20151106-034359
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: mips-txx9 (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All errors (new ones prefixed by >>):

   drivers/gpio/gpiolib.c: In function 'acpi_can_fallback_to_crs':
>> drivers/gpio/gpiolib.c:1859:10: error: dereferencing pointer to incomplete type 'struct acpi_device'
     if (adev->data.properties || adev->driver_gpios)
             ^

vim +1859 drivers/gpio/gpiolib.c

  1853	static bool acpi_can_fallback_to_crs(struct acpi_device *adev,
  1854					     const char *con_id)
  1855	{
  1856		struct acpi_gpio_lookup *l, *lookup = NULL;
  1857	
  1858		/* Never fallback if the device has properties */
> 1859		if (adev->data.properties || adev->driver_gpios)
  1860			return false;
  1861	
  1862		mutex_lock(&acpi_gpio_lookup_lock);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 21272 bytes --]

Re: [PATCH] gpiolib: tighten up ACPI legacy gpio lookups

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1457 bytes --]

Hi Dmitry,

[auto build test ERROR on: gpio/for-next]
[also build test ERROR on: v4.3 next-20151105]

url:    https://github.com/0day-ci/linux/commits/Dmitry-Torokhov/gpiolib-tighten-up-ACPI-legacy-gpio-lookups/20151106-034359
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: cris-etrax-100lx_v2_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=cris 

All errors (new ones prefixed by >>):

   drivers/gpio/gpiolib.c: In function 'acpi_can_fallback_to_crs':
>> drivers/gpio/gpiolib.c:1859:10: error: dereferencing pointer to incomplete type
   drivers/gpio/gpiolib.c:1859:35: error: dereferencing pointer to incomplete type

vim +1859 drivers/gpio/gpiolib.c

  1853	static bool acpi_can_fallback_to_crs(struct acpi_device *adev,
  1854					     const char *con_id)
  1855	{
  1856		struct acpi_gpio_lookup *l, *lookup = NULL;
  1857	
  1858		/* Never fallback if the device has properties */
> 1859		if (adev->data.properties || adev->driver_gpios)
  1860			return false;
  1861	
  1862		mutex_lock(&acpi_gpio_lookup_lock);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 7949 bytes --]

Re: [PATCH] gpiolib: tighten up ACPI legacy gpio lookups

[Mika Westerberg]

On Thu, Nov 05, 2015 at 11:38:38AM -0800, Dmitry Torokhov wrote:
> We should not fall back to the legacy unnamed gpio lookup style if the
> driver requests gpios with different names, because we'll give out the same
> gpio twice. Let's keep track of the names that were used for the device and
> only do the fallback for the first name used.
> 
> Also disable fallback to _CRS gpios if ACPI device has DT-like
> properties or driver-supplied gpio mappings.

Thanks for taking care of this!

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> 
> This version incorporates changes suggested by Mika Westerberg in
> response to the draft patch I posted in Goodix thread.
> 
>  drivers/gpio/gpiolib.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a18f00f..9631ee8 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1841,6 +1841,50 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
>  	return desc;
>  }
>  
> +struct acpi_gpio_lookup {
> +	struct list_head node;
> +	struct acpi_device *adev;
> +	const char *con_id;
> +};
> +
> +static DEFINE_MUTEX(acpi_gpio_lookup_lock);
> +static LIST_HEAD(acpi_gpio_lookup_list);
> +
> +static bool acpi_can_fallback_to_crs(struct acpi_device *adev,
> +				     const char *con_id)
> +{
> +	struct acpi_gpio_lookup *l, *lookup = NULL;
> +
> +	/* Never fallback if the device has properties */
> +	if (adev->data.properties || adev->driver_gpios)
> +		return false;

I missed the fact that struct acpi_device is declared in
<acpi/acpi_bus.h> so we can't use the two fields directly here when
!CONFIG_ACPI. Sorry about that.

Do you think we can just add 

#ifdef CONFIG_ACPI

#endif

around the above check?

Alternatively we could add an inline function to drivers/gpio/gpiolib.h
like:

#ifdef CONFIG_ACPI
static inline bool acpi_gpio_has_properties(struct acpi_device *adev)
{
	return adev->data.properties || adev->driver_gpios;
}
#else
static inline bool acpi_gpio_has_properties(struct acpi_device *adev)
{
	return false;
}
#endif

> +
> +	mutex_lock(&acpi_gpio_lookup_lock);
> +
> +	list_for_each_entry(l, &acpi_gpio_lookup_list, node) {
> +		if (l->adev == adev) {
> +			lookup = l;
> +			break;
> +		}
> +	}
> +
> +	if (!lookup) {
> +		lookup = kmalloc(sizeof(*lookup), GFP_KERNEL);
> +		if (lookup) {
> +			lookup->adev = adev;
> +			lookup->con_id = con_id;
> +			list_add_tail(&lookup->node, &acpi_gpio_lookup_list);
> +		}
> +	}
> +
> +	mutex_unlock(&acpi_gpio_lookup_lock);
> +
> +	return lookup &&
> +		((!lookup->con_id && !con_id) ||
> +		 (lookup->con_id && con_id &&
> +		  strcmp(lookup->con_id, con_id) == 0));
> +}
> +
>  static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>  					unsigned int idx,
>  					enum gpio_lookup_flags *flags)
> @@ -1868,6 +1912,9 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
>  
>  	/* Then from plain _CRS GPIOs */
>  	if (IS_ERR(desc)) {
> +		if (!acpi_can_fallback_to_crs(adev, con_id))
> +			return ERR_PTR(-ENOENT);
> +
>  		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
>  		if (IS_ERR(desc))
>  			return desc;
> -- 
> 2.6.0.rc2.230.g3dd15c0
> 
> 
> -- 
> Dmitry

Re: [PATCH] gpiolib: tighten up ACPI legacy gpio lookups

[Dmitry Torokhov]

On Fri, Nov 6, 2015 at 12:08 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Thu, Nov 05, 2015 at 11:38:38AM -0800, Dmitry Torokhov wrote:
>> We should not fall back to the legacy unnamed gpio lookup style if the
>> driver requests gpios with different names, because we'll give out the same
>> gpio twice. Let's keep track of the names that were used for the device and
>> only do the fallback for the first name used.
>>
>> Also disable fallback to _CRS gpios if ACPI device has DT-like
>> properties or driver-supplied gpio mappings.
>
> Thanks for taking care of this!
>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>>
>> This version incorporates changes suggested by Mika Westerberg in
>> response to the draft patch I posted in Goodix thread.
>>
>>  drivers/gpio/gpiolib.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index a18f00f..9631ee8 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -1841,6 +1841,50 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
>>       return desc;
>>  }
>>
>> +struct acpi_gpio_lookup {
>> +     struct list_head node;
>> +     struct acpi_device *adev;
>> +     const char *con_id;
>> +};
>> +
>> +static DEFINE_MUTEX(acpi_gpio_lookup_lock);
>> +static LIST_HEAD(acpi_gpio_lookup_list);
>> +
>> +static bool acpi_can_fallback_to_crs(struct acpi_device *adev,
>> +                                  const char *con_id)
>> +{
>> +     struct acpi_gpio_lookup *l, *lookup = NULL;
>> +
>> +     /* Never fallback if the device has properties */
>> +     if (adev->data.properties || adev->driver_gpios)
>> +             return false;
>
> I missed the fact that struct acpi_device is declared in
> <acpi/acpi_bus.h> so we can't use the two fields directly here when
> !CONFIG_ACPI. Sorry about that.
>
> Do you think we can just add
>
> #ifdef CONFIG_ACPI
>
> #endif
>
> around the above check?
>
> Alternatively we could add an inline function to drivers/gpio/gpiolib.h
> like:
>
> #ifdef CONFIG_ACPI
> static inline bool acpi_gpio_has_properties(struct acpi_device *adev)
> {
>         return adev->data.properties || adev->driver_gpios;
> }
> #else
> static inline bool acpi_gpio_has_properties(struct acpi_device *adev)
> {
>         return false;
> }
> #endif
>

I wonder if I should move acpi_can_fallback_to_crs() into
gpiolib-acpi.c and provide a stub for it...

Thanks.

-- 
Dmitry

Re: [PATCH] gpiolib: tighten up ACPI legacy gpio lookups

[Mika Westerberg]

On Fri, Nov 06, 2015 at 12:25:34AM -0800, Dmitry Torokhov wrote:
> I wonder if I should move acpi_can_fallback_to_crs() into
> gpiolib-acpi.c and provide a stub for it...

That works too :-)

Re: [PATCH] gpiolib: tighten up ACPI legacy gpio lookups

[Linus Walleij]

On Thu, Nov 5, 2015 at 8:38 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> We should not fall back to the legacy unnamed gpio lookup style if the
> driver requests gpios with different names, because we'll give out the same
> gpio twice. Let's keep track of the names that were used for the device and
> only do the fallback for the first name used.
>
> Also disable fallback to _CRS gpios if ACPI device has DT-like
> properties or driver-supplied gpio mappings.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Looking for a newer version of this I think, also would like
Mika's and/or Rafael's ACKs.

Yours,
Linus Walleij