| Message ID | 20151105193838.GA33410@dtor-ws |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.
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
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
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;
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(+)