gpiolib: tighten up ACPI legacy gpio lookups
diff mbox

Message ID 20151105193838.GA33410@dtor-ws
State New
Headers show

Commit Message

Dmitry Torokhov Nov. 5, 2015, 7:38 p.m. UTC
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(+)

Comments

kernel test robot Nov. 5, 2015, 9 p.m. UTC | #1
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
kernel test robot Nov. 5, 2015, 9:02 p.m. UTC | #2
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
Mika Westerberg Nov. 6, 2015, 8:08 a.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov Nov. 6, 2015, 8:25 a.m. UTC | #4
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.
Mika Westerberg Nov. 6, 2015, 8:31 a.m. UTC | #5
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 :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Nov. 17, 2015, 10:57 a.m. UTC | #6
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;