| Message ID | 20230125201020.10948-2-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: First attempt to clean up headers | expand |
Le 25/01/2023 à 21:10, Andy Shevchenko a écrit : > From: Pierluigi Passaro <pierluigi.p@variscite.com> > > Both the functions gpiochip_request_own_desc and > gpiochip_free_own_desc are exported from > drivers/gpio/gpiolib.c > but this file is compiled only when CONFIG_GPIOLIB is enabled. > Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide > reasonable definitions and includes in the "#else" branch. Can you give more details on when and why link fails ? You are adding a WARN(), I understand it mean the function should never ever be called. Shouldn't it be dropped completely by the compiler ? In that case, no call to gpiochip_request_own_desc() should be emitted and so link should be ok. If link fails, it means we still have unexpected calls to gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should fix the root cause instead of hiding it with a WARN(). Christophe > > Fixes: 9091373ab7ea ("gpio: remove less important #ifdef around declarations") > Signed-off-by: Pierluigi Passaro <pierluigi.p@variscite.com> > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > include/linux/gpio/driver.h | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h > index 26a808fb8a25..67990908a040 100644 > --- a/include/linux/gpio/driver.h > +++ b/include/linux/gpio/driver.h > @@ -751,6 +751,8 @@ gpiochip_remove_pin_ranges(struct gpio_chip *gc) > > #endif /* CONFIG_PINCTRL */ > > +#ifdef CONFIG_GPIOLIB > + > struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc, > unsigned int hwnum, > const char *label, > @@ -758,8 +760,6 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc, > enum gpiod_flags dflags); > void gpiochip_free_own_desc(struct gpio_desc *desc); > > -#ifdef CONFIG_GPIOLIB > - > /* lock/unlock as IRQ */ > int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset); > void gpiochip_unlock_as_irq(struct gpio_chip *gc, unsigned int offset); > @@ -769,6 +769,25 @@ struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc); > > #else /* CONFIG_GPIOLIB */ > > +#include <linux/gpio/machine.h> > +#include <linux/gpio/consumer.h> > + > +static inline struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc, > + unsigned int hwnum, > + const char *label, > + enum gpio_lookup_flags lflags, > + enum gpiod_flags dflags) > +{ > + /* GPIO can never have been requested */ > + WARN_ON(1); > + return ERR_PTR(-ENODEV); > +} > + > +static inline void gpiochip_free_own_desc(struct gpio_desc *desc) > +{ > + WARN_ON(1); > +} > + > static inline struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc) > { > /* GPIO can never have been requested */
On Thu, Jan 26, 2023, at 09:14, Christophe Leroy wrote: > Le 25/01/2023 à 21:10, Andy Shevchenko a écrit : >> From: Pierluigi Passaro <pierluigi.p@variscite.com> >> >> Both the functions gpiochip_request_own_desc and >> gpiochip_free_own_desc are exported from >> drivers/gpio/gpiolib.c >> but this file is compiled only when CONFIG_GPIOLIB is enabled. >> Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide >> reasonable definitions and includes in the "#else" branch. > > Can you give more details on when and why link fails ? > > You are adding a WARN(), I understand it mean the function should never > ever be called. Shouldn't it be dropped completely by the compiler ? In > that case, no call to gpiochip_request_own_desc() should be emitted and > so link should be ok. > > If link fails, it means we still have unexpected calls to > gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should > fix the root cause instead of hiding it with a WARN(). There are only a handful of files calling these functions: $ git grep -l gpiochip_request_own_desc Documentation/driver-api/gpio/driver.rst arch/arm/mach-omap1/ams-delta-fiq.c arch/arm/mach-omap1/board-ams-delta.c drivers/gpio/gpio-mvebu.c drivers/gpio/gpiolib-acpi.c drivers/gpio/gpiolib.c drivers/hid/hid-cp2112.c drivers/memory/omap-gpmc.c drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c drivers/power/supply/collie_battery.c drivers/spi/spi-bcm2835.c include/linux/gpio/driver.h All of these should already prevent the link failure through a Kconfig 'depends on GPIOLIB' for the driver, or 'select GPIOLIB' for the platform code. I checked all of the above and they seem fine. If anything else calls the function, I'd add the same dependency there. Arnd
On Thu, Jan 26, 2023 at 09:40:18AM +0100, Arnd Bergmann wrote: > On Thu, Jan 26, 2023, at 09:14, Christophe Leroy wrote: > > Le 25/01/2023 à 21:10, Andy Shevchenko a écrit : > >> From: Pierluigi Passaro <pierluigi.p@variscite.com> > >> > >> Both the functions gpiochip_request_own_desc and > >> gpiochip_free_own_desc are exported from > >> drivers/gpio/gpiolib.c > >> but this file is compiled only when CONFIG_GPIOLIB is enabled. > >> Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide > >> reasonable definitions and includes in the "#else" branch. > > > > Can you give more details on when and why link fails ? > > > > You are adding a WARN(), I understand it mean the function should never > > ever be called. Shouldn't it be dropped completely by the compiler ? In > > that case, no call to gpiochip_request_own_desc() should be emitted and > > so link should be ok. > > > > If link fails, it means we still have unexpected calls to > > gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should > > fix the root cause instead of hiding it with a WARN(). > > There are only a handful of files calling these functions: > > $ git grep -l gpiochip_request_own_desc > Documentation/driver-api/gpio/driver.rst > arch/arm/mach-omap1/ams-delta-fiq.c > arch/arm/mach-omap1/board-ams-delta.c > drivers/gpio/gpio-mvebu.c > drivers/gpio/gpiolib-acpi.c > drivers/gpio/gpiolib.c > drivers/hid/hid-cp2112.c > drivers/memory/omap-gpmc.c > drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c > drivers/power/supply/collie_battery.c > drivers/spi/spi-bcm2835.c > include/linux/gpio/driver.h > > All of these should already prevent the link failure through > a Kconfig 'depends on GPIOLIB' for the driver, or 'select GPIOLIB' > for the platform code. I checked all of the above and they seem fine. > If anything else calls the function, I'd add the same dependency > there. So, you think it's worth to send a few separate fixes as adding that dependency? But doesn't it feel like a papering over the issue with that APIs used in some of the drivers in the first place?
On Thu, Jan 26, 2023 at 08:14:49AM +0000, Christophe Leroy wrote: > Le 25/01/2023 à 21:10, Andy Shevchenko a écrit : > > From: Pierluigi Passaro <pierluigi.p@variscite.com> > > > > Both the functions gpiochip_request_own_desc and > > gpiochip_free_own_desc are exported from > > drivers/gpio/gpiolib.c > > but this file is compiled only when CONFIG_GPIOLIB is enabled. > > Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide > > reasonable definitions and includes in the "#else" branch. > > Can you give more details on when and why link fails ? > > You are adding a WARN(), I understand it mean the function should never > ever be called. Shouldn't it be dropped completely by the compiler ? In > that case, no call to gpiochip_request_own_desc() should be emitted and > so link should be ok. > > If link fails, it means we still have unexpected calls to > gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should > fix the root cause instead of hiding it with a WARN(). I agree, but what do you suggest exactly? I think the calls to that functions shouldn't be in the some drivers as it's layering violation (they are not a GPIO chips to begin with). Simply adding a dependency not better than this one.
On Thu, Jan 26, 2023, at 11:17, Andy Shevchenko wrote: > On Thu, Jan 26, 2023 at 09:40:18AM +0100, Arnd Bergmann wrote: >> On Thu, Jan 26, 2023, at 09:14, Christophe Leroy wrote: >> >> All of these should already prevent the link failure through >> a Kconfig 'depends on GPIOLIB' for the driver, or 'select GPIOLIB' >> for the platform code. I checked all of the above and they seem fine. >> If anything else calls the function, I'd add the same dependency >> there. > > So, you think it's worth to send a few separate fixes as adding that > dependency? But doesn't it feel like a papering over the issue with > that APIs used in some of the drivers in the first place? If there are drivers that use the interfaces but shouldn't then fixing those drivers is clearly better than adding a dependency, but we can decide that when someone sends a patch. Adding a stub helper that can never be used legitimately but turns a build time error into a run time warning seems counterproductive to me, as the CI systems are no longer able to report these automatically. Arnd
On Thu, Jan 26, 2023 at 11:27:51AM +0100, Arnd Bergmann wrote: > On Thu, Jan 26, 2023, at 11:17, Andy Shevchenko wrote: > > On Thu, Jan 26, 2023 at 09:40:18AM +0100, Arnd Bergmann wrote: > >> On Thu, Jan 26, 2023, at 09:14, Christophe Leroy wrote: > >> > >> All of these should already prevent the link failure through > >> a Kconfig 'depends on GPIOLIB' for the driver, or 'select GPIOLIB' > >> for the platform code. I checked all of the above and they seem fine. > >> If anything else calls the function, I'd add the same dependency > >> there. > > > > So, you think it's worth to send a few separate fixes as adding that > > dependency? But doesn't it feel like a papering over the issue with > > that APIs used in some of the drivers in the first place? > > If there are drivers that use the interfaces but shouldn't then > fixing those drivers is clearly better than adding a dependency, > but we can decide that when someone sends a patch. > > Adding a stub helper that can never be used legitimately > but turns a build time error into a run time warning seems > counterproductive to me, as the CI systems are no longer > able to report these automatically. What about adding ifdeffery in their code instead with a FIXME comment? So we will know that it's ugly and needs to be solved better sooner than later.
Le 26/01/2023 à 11:19, Andy Shevchenko a écrit : > On Thu, Jan 26, 2023 at 08:14:49AM +0000, Christophe Leroy wrote: >> Le 25/01/2023 à 21:10, Andy Shevchenko a écrit : >>> From: Pierluigi Passaro <pierluigi.p@variscite.com> >>> >>> Both the functions gpiochip_request_own_desc and >>> gpiochip_free_own_desc are exported from >>> drivers/gpio/gpiolib.c >>> but this file is compiled only when CONFIG_GPIOLIB is enabled. >>> Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide >>> reasonable definitions and includes in the "#else" branch. >> >> Can you give more details on when and why link fails ? >> >> You are adding a WARN(), I understand it mean the function should never >> ever be called. Shouldn't it be dropped completely by the compiler ? In >> that case, no call to gpiochip_request_own_desc() should be emitted and >> so link should be ok. >> >> If link fails, it means we still have unexpected calls to >> gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should >> fix the root cause instead of hiding it with a WARN(). > > I agree, but what do you suggest exactly? I think the calls to that functions > shouldn't be in the some drivers as it's layering violation (they are not a > GPIO chips to begin with). Simply adding a dependency not better than this one. > My suggestion is to go step by step. First step is to explicitely list drivers that call those functions without selecting GPIOLIB. Once we have this list we can see one by one how we solve it. And if we want to catch the problem before the final link, then I think we may use BUILD_BUG() but not WARN or WARN_ON. Christophe
On Thu, Jan 26, 2023, at 13:44, Christophe Leroy wrote: > Le 26/01/2023 à 11:19, Andy Shevchenko a écrit : >> On Thu, Jan 26, 2023 at 08:14:49AM +0000, Christophe Leroy wrote: >>> Le 25/01/2023 à 21:10, Andy Shevchenko a écrit : >>>> From: Pierluigi Passaro <pierluigi.p@variscite.com> >>>> >>>> Both the functions gpiochip_request_own_desc and >>>> gpiochip_free_own_desc are exported from >>>> drivers/gpio/gpiolib.c >>>> but this file is compiled only when CONFIG_GPIOLIB is enabled. >>>> Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide >>>> reasonable definitions and includes in the "#else" branch. >>> >>> Can you give more details on when and why link fails ? >>> >>> You are adding a WARN(), I understand it mean the function should never >>> ever be called. Shouldn't it be dropped completely by the compiler ? In >>> that case, no call to gpiochip_request_own_desc() should be emitted and >>> so link should be ok. >>> >>> If link fails, it means we still have unexpected calls to >>> gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should >>> fix the root cause instead of hiding it with a WARN(). >> >> I agree, but what do you suggest exactly? I think the calls to that functions >> shouldn't be in the some drivers as it's layering violation (they are not a >> GPIO chips to begin with). Simply adding a dependency not better than this one. >> > > My suggestion is to go step by step. First step is to explicitely list > drivers that call those functions without selecting GPIOLIB. I tried that and sent the list of the drivers that call these functions, but as I wrote, all of them already require GPIOLIB to be set. This means either I made a mistake in my search, or the problem has already been fixed. Either way, I think Andy should provide the exact build failure he observed so we know what caller caused the issue. Arnd
On Thu, Jan 26, 2023 at 01:56:26PM +0100, Arnd Bergmann wrote: > On Thu, Jan 26, 2023, at 13:44, Christophe Leroy wrote: > > Le 26/01/2023 à 11:19, Andy Shevchenko a écrit : > >> On Thu, Jan 26, 2023 at 08:14:49AM +0000, Christophe Leroy wrote: > >>> Le 25/01/2023 à 21:10, Andy Shevchenko a écrit : > >>>> From: Pierluigi Passaro <pierluigi.p@variscite.com> > >>>> > >>>> Both the functions gpiochip_request_own_desc and > >>>> gpiochip_free_own_desc are exported from > >>>> drivers/gpio/gpiolib.c > >>>> but this file is compiled only when CONFIG_GPIOLIB is enabled. > >>>> Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide > >>>> reasonable definitions and includes in the "#else" branch. > >>> > >>> Can you give more details on when and why link fails ? > >>> > >>> You are adding a WARN(), I understand it mean the function should never > >>> ever be called. Shouldn't it be dropped completely by the compiler ? In > >>> that case, no call to gpiochip_request_own_desc() should be emitted and > >>> so link should be ok. > >>> > >>> If link fails, it means we still have unexpected calls to > >>> gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should > >>> fix the root cause instead of hiding it with a WARN(). > >> > >> I agree, but what do you suggest exactly? I think the calls to that functions > >> shouldn't be in the some drivers as it's layering violation (they are not a > >> GPIO chips to begin with). Simply adding a dependency not better than this one. > >> > > > > My suggestion is to go step by step. First step is to explicitely list > > drivers that call those functions without selecting GPIOLIB. > > I tried that and sent the list of the drivers that call these functions, > but as I wrote, all of them already require GPIOLIB to be set. > > This means either I made a mistake in my search, or the problem > has already been fixed. Either way, I think Andy should provide > the exact build failure he observed so we know what caller caused > the issue. I believe it's not me, who first reported it. So, Pierluigi, can you point out to the LKP message that reported the issue? P.S> LKP sometimes finds a really twisted configurations to probe on.
On Thu, Jan 26, 2023 at 02:29PM +0100, Arnd Bergmann wrote: > On Thu, Jan 26, 2023 at 01:56:26PM +0100, Arnd Bergmann wrote: > > On Thu, Jan 26, 2023, at 13:44, Christophe Leroy wrote: > > > Le 26/01/2023 à 11:19, Andy Shevchenko a écrit : > > >> On Thu, Jan 26, 2023 at 08:14:49AM +0000, Christophe Leroy wrote: > > >>> Le 25/01/2023 à 21:10, Andy Shevchenko a écrit : > > >>>> From: Pierluigi Passaro <pierluigi.p@variscite.com> > > >>>> > > >>>> Both the functions gpiochip_request_own_desc and > > >>>> gpiochip_free_own_desc are exported from > > >>>> drivers/gpio/gpiolib.c > > >>>> but this file is compiled only when CONFIG_GPIOLIB is enabled. > > >>>> Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide > > >>>> reasonable definitions and includes in the "#else" branch. > > >>> > > >>> Can you give more details on when and why link fails ? > > >>> > > >>> You are adding a WARN(), I understand it mean the function should never > > >>> ever be called. Shouldn't it be dropped completely by the compiler ? In > > >>> that case, no call to gpiochip_request_own_desc() should be emitted and > > >>> so link should be ok. > > >>> > > >>> If link fails, it means we still have unexpected calls to > > >>> gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should > > >>> fix the root cause instead of hiding it with a WARN(). > > >> > > >> I agree, but what do you suggest exactly? I think the calls to that functions > > >> shouldn't be in the some drivers as it's layering violation (they are not a > > >> GPIO chips to begin with). Simply adding a dependency not better than this one. > > >> > > > > > > My suggestion is to go step by step. First step is to explicitely list > > > drivers that call those functions without selecting GPIOLIB. > > > > I tried that and sent the list of the drivers that call these functions, > > but as I wrote, all of them already require GPIOLIB to be set. > > > > This means either I made a mistake in my search, or the problem > > has already been fixed. Either way, I think Andy should provide > > the exact build failure he observed so we know what caller caused > > the issue. > > I believe it's not me, who first reported it. So, Pierluigi, can you point > out to the LKP message that reported the issue? > > P.S> LKP sometimes finds a really twisted configurations to probe on. > > -- > With Best Regards, > Andy Shevchenko > I've received the following messages: - https://lore.kernel.org/all/202301240409.tZdm0o0a-lkp@intel.com/ - https://lore.kernel.org/all/202301240439.wYz6uU0k-lkp@intel.com/ - https://lore.kernel.org/all/20230124075600.649bd7bb@canb.auug.org.au/ Please let me know if you need further details. Regards Pier
On Thu, Jan 26, 2023, at 14:41, Pierluigi Passaro wrote: > On Thu, Jan 26, 2023 at 02:29PM +0100, Arnd Bergmann wrote: >> > >>> >> > >>> If link fails, it means we still have unexpected calls to >> > >>> gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should >> > >>> fix the root cause instead of hiding it with a WARN(). >> > This means either I made a mistake in my search, or the problem >> > has already been fixed. Either way, I think Andy should provide >> > the exact build failure he observed so we know what caller caused >> > the issue. >> >> I believe it's not me, who first reported it. So, Pierluigi, can you point >> out to the LKP message that reported the issue? >> >> P.S> LKP sometimes finds a really twisted configurations to probe on. >> >> > I've received the following messages: > - https://lore.kernel.org/all/202301240409.tZdm0o0a-lkp@intel.com/ > - https://lore.kernel.org/all/202301240439.wYz6uU0k-lkp@intel.com/ > - https://lore.kernel.org/all/20230124075600.649bd7bb@canb.auug.org.au/ > Please let me know if you need further details. I think these three are all regressions that are caused by the patch in this thread, rather than the original problem that it was trying to fix. The one we're looking for is a randconfig bug that showed up as a link failure when referencing gpiochip_request_own_desc or gpiochip_free_own_desc. Arnd
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 26a808fb8a25..67990908a040 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -751,6 +751,8 @@ gpiochip_remove_pin_ranges(struct gpio_chip *gc) #endif /* CONFIG_PINCTRL */ +#ifdef CONFIG_GPIOLIB + struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc, unsigned int hwnum, const char *label, @@ -758,8 +760,6 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc, enum gpiod_flags dflags); void gpiochip_free_own_desc(struct gpio_desc *desc); -#ifdef CONFIG_GPIOLIB - /* lock/unlock as IRQ */ int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset); void gpiochip_unlock_as_irq(struct gpio_chip *gc, unsigned int offset); @@ -769,6 +769,25 @@ struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc); #else /* CONFIG_GPIOLIB */ +#include <linux/gpio/machine.h> +#include <linux/gpio/consumer.h> + +static inline struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc, + unsigned int hwnum, + const char *label, + enum gpio_lookup_flags lflags, + enum gpiod_flags dflags) +{ + /* GPIO can never have been requested */ + WARN_ON(1); + return ERR_PTR(-ENODEV); +} + +static inline void gpiochip_free_own_desc(struct gpio_desc *desc) +{ + WARN_ON(1); +} + static inline struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc) { /* GPIO can never have been requested */