[PATCH v2] gpio: Return EPROBE_DEFER if gc->to_irq is NULL

[PATCH v2] gpio: Return EPROBE_DEFER if gc->to_irq is NULL

[Shreeya Patel]

We are racing the registering of .to_irq when probing the
i2c driver. This results in random failure of touchscreen
devices.

Following errors could be seen in dmesg logs when gc->to_irq is NULL

[2.101857] i2c_hid i2c-FTS3528:00: HID over i2c has not been provided an Int IRQ
[2.101953] i2c_hid: probe of i2c-FTS3528:00 failed with error -22

To avoid this situation, defer probing until to_irq is registered.

This issue has been reported many times in past and people have been
using workarounds like changing the pinctrl_amd to built-in instead
of loading it as a module or by adding a softdep for pinctrl_amd into
the config file.

References :-
https://bugzilla.kernel.org/show_bug.cgi?id=209413
https://github.com/Syniurge/i2c-amd-mp2/issues/3

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>

---
Changes in v2
  - Add a condition to check for irq chip to avoid bogus error.

---
 drivers/gpio/gpiolib.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 27c07108496d..12c088706167 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3084,6 +3084,14 @@ int gpiod_to_irq(const struct gpio_desc *desc)
 
 		return retirq;
 	}
+	if (gc->irq.chip) {
+		/* avoid race condition with other code, which tries to lookup
+		 * an IRQ before the irqchip has been properly registered
+		 * (i.e. while gpiochip is still being brought up).
+		 */
+		return -EPROBE_DEFER;
+	}
+
 	return -ENXIO;
 }
 EXPORT_SYMBOL_GPL(gpiod_to_irq);
-- 
2.30.2

Re: [PATCH v2] gpio: Return EPROBE_DEFER if gc->to_irq is NULL

[Linus Walleij]

On Tue, Oct 19, 2021 at 12:05 AM Shreeya Patel
<shreeya.patel@collabora.com> wrote:

> We are racing the registering of .to_irq when probing the
> i2c driver. This results in random failure of touchscreen
> devices.
>
> Following errors could be seen in dmesg logs when gc->to_irq is NULL
>
> [2.101857] i2c_hid i2c-FTS3528:00: HID over i2c has not been provided an Int IRQ
> [2.101953] i2c_hid: probe of i2c-FTS3528:00 failed with error -22
>
> To avoid this situation, defer probing until to_irq is registered.
>
> This issue has been reported many times in past and people have been
> using workarounds like changing the pinctrl_amd to built-in instead
> of loading it as a module or by adding a softdep for pinctrl_amd into
> the config file.
>
> References :-
> https://bugzilla.kernel.org/show_bug.cgi?id=209413
> https://github.com/Syniurge/i2c-amd-mp2/issues/3
>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
>
> ---
> Changes in v2
>   - Add a condition to check for irq chip to avoid bogus error.

This v2 looks acceptable to me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2] gpio: Return EPROBE_DEFER if gc->to_irq is NULL

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3430 bytes --]

Hi Shreeya,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linusw-gpio/for-next]
[also build test ERROR on v5.15 next-20211110]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Shreeya-Patel/gpio-Return-EPROBE_DEFER-if-gc-to_irq-is-NULL/20211019-060640
base:   https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: sh-rsk7269_defconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e807153b3cae5acb6d78393087c28e4004cb9856
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Shreeya-Patel/gpio-Return-EPROBE_DEFER-if-gc-to_irq-is-NULL/20211019-060640
        git checkout e807153b3cae5acb6d78393087c28e4004cb9856
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=sh SHELL=/bin/bash drivers/gpio/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   cc1: warning: arch/sh/include/mach-rsk: No such file or directory [-Wmissing-include-dirs]
   cc1: warning: arch/sh/include/mach-rsk: No such file or directory [-Wmissing-include-dirs]
   drivers/gpio/gpiolib.c: In function 'gpiod_to_irq':
>> drivers/gpio/gpiolib.c:3068:15: error: 'struct gpio_chip' has no member named 'irq'
    3068 |         if (gc->irq.chip) {
         |               ^~


vim +3068 drivers/gpio/gpiolib.c

  3036	
  3037	/**
  3038	 * gpiod_to_irq() - return the IRQ corresponding to a GPIO
  3039	 * @desc: gpio whose IRQ will be returned (already requested)
  3040	 *
  3041	 * Return the IRQ corresponding to the passed GPIO, or an error code in case of
  3042	 * error.
  3043	 */
  3044	int gpiod_to_irq(const struct gpio_desc *desc)
  3045	{
  3046		struct gpio_chip *gc;
  3047		int offset;
  3048	
  3049		/*
  3050		 * Cannot VALIDATE_DESC() here as gpiod_to_irq() consumer semantics
  3051		 * requires this function to not return zero on an invalid descriptor
  3052		 * but rather a negative error number.
  3053		 */
  3054		if (!desc || IS_ERR(desc) || !desc->gdev || !desc->gdev->chip)
  3055			return -EINVAL;
  3056	
  3057		gc = desc->gdev->chip;
  3058		offset = gpio_chip_hwgpio(desc);
  3059		if (gc->to_irq) {
  3060			int retirq = gc->to_irq(gc, offset);
  3061	
  3062			/* Zero means NO_IRQ */
  3063			if (!retirq)
  3064				return -ENXIO;
  3065	
  3066			return retirq;
  3067		}
> 3068		if (gc->irq.chip) {
  3069			/* avoid race condition with other code, which tries to lookup
  3070			 * an IRQ before the irqchip has been properly registered
  3071			 * (i.e. while gpiochip is still being brought up).
  3072			 */
  3073			return -EPROBE_DEFER;
  3074		}
  3075	
  3076		return -ENXIO;
  3077	}
  3078	EXPORT_SYMBOL_GPL(gpiod_to_irq);
  3079	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 11712 bytes --]

Re: [PATCH v2] gpio: Return EPROBE_DEFER if gc->to_irq is NULL

[Linus Walleij]

On Wed, Nov 10, 2021 at 10:16 AM kernel test robot <lkp@intel.com> wrote:

>    drivers/gpio/gpiolib.c: In function 'gpiod_to_irq':
> >> drivers/gpio/gpiolib.c:3068:15: error: 'struct gpio_chip' has no member named 'irq'
>     3068 |         if (gc->irq.chip) {
>          |               ^~

Right you need an #ifdef CONFIG_GPIOLIB_IRQCHIP in this
case I would try:

if (IS_ENABLED(CONFIG_GPIOLIB_IRQCHIP) && gc->irq.chip)

hoping the compiler is smart enough to deal with that
(but I'm not sure it is)

Yours,
Linus Walleij

Re: [PATCH v2] gpio: Return EPROBE_DEFER if gc->to_irq is NULL

[Andy Shevchenko]

On Tue, Oct 19, 2021 at 1:07 AM Shreeya Patel
<shreeya.patel@collabora.com> wrote:
>
> We are racing the registering of .to_irq when probing the
> i2c driver. This results in random failure of touchscreen
> devices.
>
> Following errors could be seen in dmesg logs when gc->to_irq is NULL
>
> [2.101857] i2c_hid i2c-FTS3528:00: HID over i2c has not been provided an Int IRQ
> [2.101953] i2c_hid: probe of i2c-FTS3528:00 failed with error -22
>
> To avoid this situation, defer probing until to_irq is registered.
>
> This issue has been reported many times in past and people have been
> using workarounds like changing the pinctrl_amd to built-in instead
> of loading it as a module or by adding a softdep for pinctrl_amd into
> the config file.

> References :-
> https://bugzilla.kernel.org/show_bug.cgi?id=209413
> https://github.com/Syniurge/i2c-amd-mp2/issues/3

Please, convert them to BugLink: tags.

> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>

...

> +       if (gc->irq.chip) {
> +               /* avoid race condition with other code, which tries to lookup
> +                * an IRQ before the irqchip has been properly registered
> +                * (i.e. while gpiochip is still being brought up).
> +                */

/*
 * The style of multi-line comments should be
 * like this. Pay attention to the grammar.
 * In your comment the parentheses can be replaced
 * by a simple ',' (comma without quotes).
 */

> +               return -EPROBE_DEFER;
> +       }


With above (including kbuildbot thingy) addressed
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

-- 
With Best Regards,
Andy Shevchenko