From: Pierluigi Passaro <[email protected]>
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.
Fixes: 9091373ab7ea ("gpio: remove less important #ifdef around declarations")
Signed-off-by: Pierluigi Passaro <[email protected]>
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
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 */
--
2.39.0
Le 25/01/2023 à 21:10, Andy Shevchenko a écrit :
> From: Pierluigi Passaro <[email protected]>
>
> 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 <[email protected]>
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> 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 <[email protected]>
>>
>> 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 <[email protected]>
> >>
> >> 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?
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
> >
> > 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.
--
With Best Regards,
Andy Shevchenko
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.
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
> >>>>
> >>>> 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
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 <[email protected]>
> > >>>>
> > >>>> 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/[email protected]/
- https://lore.kernel.org/all/[email protected]/
- https://lore.kernel.org/all/[email protected]/
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/[email protected]/
> - https://lore.kernel.org/all/[email protected]/
> - https://lore.kernel.org/all/[email protected]/
> 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