| Message ID | 20170323194618.26548-5-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Thu, Mar 23, 2017 at 09:46:14PM +0200, Andy Shevchenko wrote: > The commit 10cf4899f8af ("gpiolib: tighten up ACPI legacy gpio lookups") > prevents to getting same resource twice if the driver asks twice using same s/same/different/ > connection ID. > > But the whole idea of fallback might bring some problems. Imagine the case when > we have two versions of BIOS/hardware where in one _DSD is introduced along > with GPIO resources, but the other one uses just plain GPIO resource for > another purpose > > Case 1: > > Device (DEVX) > { > ... > Name (_CRS, ResourceTemplate () > { > GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly, > "\\_SB.GPO0", 0, ResourceConsumer) {15} > }) > Name (_DSD, Package () > { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () > { > Package () {"some-gpios", Package() {^DEVX, 0, 0, 0 }}, > } > }) > } > > Case 2: > > Device (DEVX) > { > ... > Name (_CRS, ResourceTemplate () > { > GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly, > "\\_SB.GPO0", 0, ResourceConsumer) {27} > }) > } > > To prevent the possible misconfiguration tighten up even more ACPI GPIO lookups > for case without connection ID provided. I wonder if this will break Goodix. Irina, Bastien? > > In the past the issue had been triggered by "use mctrl_gpio helpers" series > [1,2]. > > Besides above, removal of the main logic of acpi_can_fallback_to_crs() > eliminates a potential memory leak when the same device has been unbound and > bound again. Where? We'll reuse lookup table as ACPI device is still the same. > > [1] commit 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers") > [2] https://patchwork.kernel.org/patch/9283745/ > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpiolib-acpi.c | 36 +----------------------------------- > 1 file changed, 1 insertion(+), 35 deletions(-) > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > index 21e4930ca2db..e516b7a0cc50 100644 > --- a/drivers/gpio/gpiolib-acpi.c > +++ b/drivers/gpio/gpiolib-acpi.c > @@ -1121,45 +1121,11 @@ int acpi_gpio_count(struct device *dev, const char *con_id) > return count ? count : -ENOENT; > } > > -struct acpi_crs_lookup { > - struct list_head node; > - struct acpi_device *adev; > - const char *con_id; > -}; > - > -static DEFINE_MUTEX(acpi_crs_lookup_lock); > -static LIST_HEAD(acpi_crs_lookup_list); > - > bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id) > { > - struct acpi_crs_lookup *l, *lookup = NULL; > - > /* Never allow fallback if the device has properties */ > if (adev->data.properties || adev->driver_gpios) > return false; > > - mutex_lock(&acpi_crs_lookup_lock); > - > - list_for_each_entry(l, &acpi_crs_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 = kstrdup(con_id, GFP_KERNEL); > - list_add_tail(&lookup->node, &acpi_crs_lookup_list); > - } > - } > - > - mutex_unlock(&acpi_crs_lookup_lock); > - > - return lookup && > - ((!lookup->con_id && !con_id) || > - (lookup->con_id && con_id && > - strcmp(lookup->con_id, con_id) == 0)); > + return con_id == NULL; > } > -- > 2.11.0 >
On Thu, 2017-03-23 at 13:12 -0700, Dmitry Torokhov wrote: > <snip> > > To prevent the possible misconfiguration tighten up even more ACPI > > GPIO lookups > > for case without connection ID provided. > > I wonder if this will break Goodix. Irina, Bastien? My Goodix tablet has been on the fritz for nearly a year. Hopefully, 32-bit-UEFI support will be available for my preferred distribution soon, and I'd be able to tell you. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2017-03-23 at 13:12 -0700, Dmitry Torokhov wrote: > On Thu, Mar 23, 2017 at 09:46:14PM +0200, Andy Shevchenko wrote: > > The commit 10cf4899f8af ("gpiolib: tighten up ACPI legacy gpio > > lookups") > > prevents to getting same resource twice if the driver asks twice > > using same > > s/same/different/ > > > connection ID. Oh, yeah, though it didn't fix a potential issue described below. > > But the whole idea of fallback might bring some problems. Imagine > > the case when > > we have two versions of BIOS/hardware where in one _DSD is > > introduced along > > with GPIO resources, but the other one uses just plain GPIO resource > > for > > another purpose > > > > Case 1: > > > > Device (DEVX) > > { > > ... > > Name (_CRS, ResourceTemplate () > > { > > GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly, > > "\\_SB.GPO0", 0, ResourceConsumer) {15} > > }) > > Name (_DSD, Package () > > { > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > Package () > > { > > Package () {"some-gpios", Package() {^DEVX, 0, 0, 0 > > }}, > > } > > }) > > } > > > > Case 2: > > > > Device (DEVX) > > { > > ... > > Name (_CRS, ResourceTemplate () > > { > > GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly, > > "\\_SB.GPO0", 0, ResourceConsumer) {27} > > }) > > } > > > > To prevent the possible misconfiguration tighten up even more ACPI > > GPIO lookups > > for case without connection ID provided. > I wonder if this will break Goodix. Irina, Bastien? It would be helpful if anyone can test it. Btw, which particular driver do you have in mind that might be broken after such change? Ah, Goodix is a vendor of touchscreens, right? I'm going through drivers which are using ACPI enumeration and gpiod_get() API to create mapping tables. I didn't touch drivers/input/ folder yet. So, potential problems might be there: drivers/input/touchscreen/elants_i2c.c drivers/input/touchscreen/goodix.c drivers/input/touchscreen/melfas_mip4.c drivers/input/touchscreen/raydium_i2c_ts.c drivers/input/touchscreen/silead.c drivers/input/touchscreen/surface3_spi.c (except silead which Hans tested few times) > > In the past the issue had been triggered by "use mctrl_gpio helpers" > > series > > [1,2]. > > > > Besides above, removal of the main logic of > > acpi_can_fallback_to_crs() > > eliminates a potential memory leak when the same device has been > > unbound and > > bound again. > > Where? We'll reuse lookup table as ACPI device is still the same. I used to have issues with unbind/bind cycle with pointer to struct device * (IIRC platform device), but you probably right since pointer to struct acpi_device is used here in your change. I will remove this paragraph from commit message. Thanks for review!
On Tue, Mar 28, 2017 at 07:31:24PM +0300, Andy Shevchenko wrote: > On Thu, 2017-03-23 at 13:12 -0700, Dmitry Torokhov wrote: > > On Thu, Mar 23, 2017 at 09:46:14PM +0200, Andy Shevchenko wrote: > > > The commit 10cf4899f8af ("gpiolib: tighten up ACPI legacy gpio > > > lookups") > > > prevents to getting same resource twice if the driver asks twice > > > using same > > > > s/same/different/ > > > > > connection ID. > > Oh, yeah, though it didn't fix a potential issue described below. > > > > But the whole idea of fallback might bring some problems. Imagine > > > the case when > > > we have two versions of BIOS/hardware where in one _DSD is > > > introduced along > > > with GPIO resources, but the other one uses just plain GPIO resource > > > for > > > another purpose > > > > > > Case 1: > > > > > > Device (DEVX) > > > { > > > ... > > > Name (_CRS, ResourceTemplate () > > > { > > > GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly, > > > "\\_SB.GPO0", 0, ResourceConsumer) {15} > > > }) > > > Name (_DSD, Package () > > > { > > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > > Package () > > > { > > > Package () {"some-gpios", Package() {^DEVX, 0, 0, 0 > > > }}, > > > } > > > }) > > > } > > > > > > Case 2: > > > > > > Device (DEVX) > > > { > > > ... > > > Name (_CRS, ResourceTemplate () > > > { > > > GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly, > > > "\\_SB.GPO0", 0, ResourceConsumer) {27} > > > }) > > > } > > > > > > To prevent the possible misconfiguration tighten up even more ACPI > > > GPIO lookups > > > for case without connection ID provided. > > > I wonder if this will break Goodix. Irina, Bastien? > > It would be helpful if anyone can test it. > > Btw, which particular driver do you have in mind that might be broken > after such change? Ah, Goodix is a vendor of touchscreens, right? Yes, and it uses named GPIOs.
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index 21e4930ca2db..e516b7a0cc50 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -1121,45 +1121,11 @@ int acpi_gpio_count(struct device *dev, const char *con_id) return count ? count : -ENOENT; } -struct acpi_crs_lookup { - struct list_head node; - struct acpi_device *adev; - const char *con_id; -}; - -static DEFINE_MUTEX(acpi_crs_lookup_lock); -static LIST_HEAD(acpi_crs_lookup_list); - bool acpi_can_fallback_to_crs(struct acpi_device *adev, const char *con_id) { - struct acpi_crs_lookup *l, *lookup = NULL; - /* Never allow fallback if the device has properties */ if (adev->data.properties || adev->driver_gpios) return false; - mutex_lock(&acpi_crs_lookup_lock); - - list_for_each_entry(l, &acpi_crs_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 = kstrdup(con_id, GFP_KERNEL); - list_add_tail(&lookup->node, &acpi_crs_lookup_list); - } - } - - mutex_unlock(&acpi_crs_lookup_lock); - - return lookup && - ((!lookup->con_id && !con_id) || - (lookup->con_id && con_id && - strcmp(lookup->con_id, con_id) == 0)); + return con_id == NULL; }
The commit 10cf4899f8af ("gpiolib: tighten up ACPI legacy gpio lookups") prevents to getting same resource twice if the driver asks twice using same connection ID. But the whole idea of fallback might bring some problems. Imagine the case when we have two versions of BIOS/hardware where in one _DSD is introduced along with GPIO resources, but the other one uses just plain GPIO resource for another purpose Case 1: Device (DEVX) { ... Name (_CRS, ResourceTemplate () { GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly, "\\_SB.GPO0", 0, ResourceConsumer) {15} }) Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () {"some-gpios", Package() {^DEVX, 0, 0, 0 }}, } }) } Case 2: Device (DEVX) { ... Name (_CRS, ResourceTemplate () { GpioIo (Exclusive, PullUp, 0, 0, IoRestrictionInputOnly, "\\_SB.GPO0", 0, ResourceConsumer) {27} }) } To prevent the possible misconfiguration tighten up even more ACPI GPIO lookups for case without connection ID provided. In the past the issue had been triggered by "use mctrl_gpio helpers" series [1,2]. Besides above, removal of the main logic of acpi_can_fallback_to_crs() eliminates a potential memory leak when the same device has been unbound and bound again. [1] commit 4ef03d328769 ("tty/serial/8250: use mctrl_gpio helpers") [2] https://patchwork.kernel.org/patch/9283745/ Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib-acpi.c | 36 +----------------------------------- 1 file changed, 1 insertion(+), 35 deletions(-)