Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964814AbdCWUMr (ORCPT ); Thu, 23 Mar 2017 16:12:47 -0400 Received: from mail-pf0-f194.google.com ([209.85.192.194]:34323 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934376AbdCWUMp (ORCPT ); Thu, 23 Mar 2017 16:12:45 -0400 Date: Thu, 23 Mar 2017 13:12:41 -0700 From: Dmitry Torokhov To: Andy Shevchenko Cc: Linus Walleij , Alexandre Courbot , linux-gpio@vger.kernel.org, Hans de Goede , linux-kernel@vger.kernel.org, Mika Westerberg , Jarkko Nikula , linux-acpi@vger.kernel.org, Irina Tirdea , Bastien Nocera Subject: Re: [PATCH v1 4/8] gpio: acpi: Even more tighten up ACPI GPIO lookups Message-ID: <20170323201241.GB2502@dtor-ws> References: <20170323194618.26548-1-andriy.shevchenko@linux.intel.com> <20170323194618.26548-5-andriy.shevchenko@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170323194618.26548-5-andriy.shevchenko@linux.intel.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3652 Lines: 127 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 > Signed-off-by: Andy Shevchenko > --- > 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 > -- Dmitry