Re: acpi_find_gpio with absent GPIOs

[Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>]

On Fri, Oct 23, 2015 at 04:56:54PM +0200, Daniel Glöckner wrote:
> Hi,
> 
> I'm currently trying to use rfkill-gpio with a device that has just a
> single GPIO assigned by ACPI. rfkill-gpio calls acpi_dev_add_driver_gpios
> to assign names to the ACPI GPIOs and then uses devm_gpiod_get_optional
> to request both of them. The problem is that on the second call to
> devm_gpiod_get_optional acpi_find_gpio falls back to using the GPIO index
> 0 (from gpiod_get) in _CRS, which leads to the same GPIO being returned
> as in the first call. Probing the driver then fails with -EBUSY.
> 
> In my opinion it is a bad idea to fall back to indexing the _CRS if the
> con_id was found in the _DSD or the GPIOs added by
> acpi_dev_add_driver_gpios, but I don't know if there are drivers relying
> on this behavior.

I agree it is bad idea and I think this is actually a bug in the
implementation rather than wanted behavior. No drivers should rely on
that anyway.

> Luckily acpi_get_gpiod_by_index returns -ENODATA if the name can't be
> found and -ENOENT if the GPIO is absent, so we can distinguish the two
> cases. -EPROBE_DEFER also should not make acpi_find_gpio try to use
> another GPIO from the _CRS.
> 
> There is also the possibility that the GPIO index exceeds the size of
> the package found in _DSD or added with acpi_dev_add_driver_gpios.
> The former will return -EPROTO, the latter will forward the error
> from acpi_dev_get_property_reference (usually -ENODATA). of_find_gpio
> returns -ENOENT in this case.
> 
> So, what of this should be fixed?

I think both should be fixed.

For the first maybe something like below?

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5db3445552b1..441be96e18e7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1765,6 +1765,11 @@ static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
 
 	/* Then from plain _CRS GPIOs */
 	if (IS_ERR(desc)) {
+		/* Only fallback if the device has no properties at all */
+		if (PTR_ERR(desc) == -ENODATA &&
+		    (adev->data.properties || adev->driver_gpios))
+			return ERR_PTR(-ENOENT);
+
 		desc = acpi_get_gpiod_by_index(adev, NULL, idx, &info);
 		if (IS_ERR(desc))
 			return desc;
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html