| Message ID | 1590153569-21706-2-git-send-email-mkshah@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | irqchip: qcom: pdc: Introduce irq_set_wake call | expand |
On 2020-05-22 14:19, Maulik Shah wrote: > With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' > gpiolib > overrides irqchip's irq_enable and irq_disable callbacks. If > irq_disable > callback is implemented then genirq takes unlazy path to disable irq. > > Underlying irqchip may not want to implement irq_disable callback to > lazy > disable irq when client drivers invokes disable_irq(). By overriding > irq_disable callback, gpiolib ends up always unlazy disabling IRQ. > > Allow gpiolib to lazy disable IRQs by overriding irq_disable callback > only > if irqchip implemented irq_disable. In cases where irq_disable is not > implemented irq_mask is overridden. Similarly override irq_enable > callback > only if irqchip implemented irq_enable otherwise irq_unmask is > overridden. > > Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable) > Signed-off-by: Maulik Shah <mkshah@codeaurora.org> > --- > drivers/gpio/gpiolib.c | 59 > +++++++++++++++++++++++++++++---------------- > include/linux/gpio/driver.h | 13 ++++++++++ > 2 files changed, 51 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index eaa0e20..a8fdc74 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -2465,33 +2465,38 @@ static void gpiochip_irq_relres(struct irq_data > *d) > gpiochip_relres_irq(gc, d->hwirq); > } > > +static void gpiochip_irq_mask(struct irq_data *d) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > + > + if (chip->irq.irq_mask) > + chip->irq.irq_mask(d); > + gpiochip_disable_irq(chip, d->hwirq); > +} > + > +static void gpiochip_irq_unmask(struct irq_data *d) > +{ > + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); > + > + gpiochip_enable_irq(chip, d->hwirq); > + if (chip->irq.irq_unmask) > + chip->irq.irq_unmask(d); > +} > + > static void gpiochip_irq_enable(struct irq_data *d) > { > struct gpio_chip *gc = irq_data_get_irq_chip_data(d); > > - gpiochip_enable_irq(gc, d->hwirq); > - if (gc->irq.irq_enable) > - gc->irq.irq_enable(d); > - else > - gc->irq.chip->irq_unmask(d); > + gpiochip_enable_irq(chip, d->hwirq); You really never compiled this, did you? I've stopped looking at this. Please send something that you will have actually tested. M.
Hi Maulik,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on gpio/for-next]
[also build test ERROR on pinctrl/devel v5.7-rc6 next-20200522]
[cannot apply to tip/irq/core]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Maulik-Shah/irqchip-qcom-pdc-Introduce-irq_set_wake-call/20200522-212226
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: x86_64-randconfig-a013-20200521 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 3393cc4cebf9969db94dc424b7a2b6195589c33b)
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
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> drivers/gpio/gpiolib.c:2490:22: error: use of undeclared identifier 'chip'
gpiochip_enable_irq(chip, d->hwirq);
^
drivers/gpio/gpiolib.c:2491:2: error: use of undeclared identifier 'chip'
chip->irq.irq_enable(d);
^
drivers/gpio/gpiolib.c:2498:2: error: use of undeclared identifier 'chip'
chip->irq.irq_disable(d);
^
drivers/gpio/gpiolib.c:2499:23: error: use of undeclared identifier 'chip'
gpiochip_disable_irq(chip, d->hwirq);
^
>> drivers/gpio/gpiolib.c:2525:3: error: use of undeclared identifier 'gpiochip'
gpiochip->irq.irq_disable = irqchip->irq_disable;
^
drivers/gpio/gpiolib.c:2528:3: error: use of undeclared identifier 'gpiochip'
gpiochip->irq.irq_mask = irqchip->irq_mask;
^
drivers/gpio/gpiolib.c:2533:3: error: use of undeclared identifier 'gpiochip'
gpiochip->irq.irq_enable = irqchip->irq_enable;
^
drivers/gpio/gpiolib.c:2536:3: error: use of undeclared identifier 'gpiochip'
gpiochip->irq.irq_unmask = irqchip->irq_unmask;
^
8 errors generated.
vim +/chip +2490 drivers/gpio/gpiolib.c
2485
2486 static void gpiochip_irq_enable(struct irq_data *d)
2487 {
2488 struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
2489
> 2490 gpiochip_enable_irq(chip, d->hwirq);
2491 chip->irq.irq_enable(d);
2492 }
2493
2494 static void gpiochip_irq_disable(struct irq_data *d)
2495 {
2496 struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
2497
2498 chip->irq.irq_disable(d);
2499 gpiochip_disable_irq(chip, d->hwirq);
2500 }
2501
2502 static void gpiochip_set_irq_hooks(struct gpio_chip *gc)
2503 {
2504 struct irq_chip *irqchip = gc->irq.chip;
2505
2506 if (!irqchip->irq_request_resources &&
2507 !irqchip->irq_release_resources) {
2508 irqchip->irq_request_resources = gpiochip_irq_reqres;
2509 irqchip->irq_release_resources = gpiochip_irq_relres;
2510 }
2511 if (WARN_ON(gc->irq.irq_enable))
2512 return;
2513 /* Check if the irqchip already has this hook... */
2514 if (irqchip->irq_enable == gpiochip_irq_enable) {
2515 /*
2516 * ...and if so, give a gentle warning that this is bad
2517 * practice.
2518 */
2519 chip_info(gc,
2520 "detected irqchip that is shared with multiple gpiochips: please fix the driver.\n");
2521 return;
2522 }
2523
2524 if (irqchip->irq_disable) {
> 2525 gpiochip->irq.irq_disable = irqchip->irq_disable;
2526 irqchip->irq_disable = gpiochip_irq_disable;
2527 } else {
2528 gpiochip->irq.irq_mask = irqchip->irq_mask;
2529 irqchip->irq_mask = gpiochip_irq_mask;
2530 }
2531
2532 if (irqchip->irq_enable) {
2533 gpiochip->irq.irq_enable = irqchip->irq_enable;
2534 irqchip->irq_enable = gpiochip_irq_enable;
2535 } else {
2536 gpiochip->irq.irq_unmask = irqchip->irq_unmask;
2537 irqchip->irq_unmask = gpiochip_irq_unmask;
2538 }
2539 }
2540
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi, On 5/23/2020 3:12 PM, Marc Zyngier wrote: > On 2020-05-22 14:19, Maulik Shah wrote: >> With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' >> gpiolib >> overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable >> callback is implemented then genirq takes unlazy path to disable irq. >> >> Underlying irqchip may not want to implement irq_disable callback to >> lazy >> disable irq when client drivers invokes disable_irq(). By overriding >> irq_disable callback, gpiolib ends up always unlazy disabling IRQ. >> >> Allow gpiolib to lazy disable IRQs by overriding irq_disable callback >> only >> if irqchip implemented irq_disable. In cases where irq_disable is not >> implemented irq_mask is overridden. Similarly override irq_enable >> callback >> only if irqchip implemented irq_enable otherwise irq_unmask is >> overridden. >> >> Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable) >> Signed-off-by: Maulik Shah <mkshah@codeaurora.org> >> --- >> drivers/gpio/gpiolib.c | 59 >> +++++++++++++++++++++++++++++---------------- >> include/linux/gpio/driver.h | 13 ++++++++++ >> 2 files changed, 51 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c >> index eaa0e20..a8fdc74 100644 >> --- a/drivers/gpio/gpiolib.c >> +++ b/drivers/gpio/gpiolib.c >> @@ -2465,33 +2465,38 @@ static void gpiochip_irq_relres(struct >> irq_data *d) >> gpiochip_relres_irq(gc, d->hwirq); >> } >> >> +static void gpiochip_irq_mask(struct irq_data *d) >> +{ >> + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); >> + >> + if (chip->irq.irq_mask) >> + chip->irq.irq_mask(d); >> + gpiochip_disable_irq(chip, d->hwirq); >> +} >> + >> +static void gpiochip_irq_unmask(struct irq_data *d) >> +{ >> + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); >> + >> + gpiochip_enable_irq(chip, d->hwirq); >> + if (chip->irq.irq_unmask) >> + chip->irq.irq_unmask(d); >> +} >> + >> static void gpiochip_irq_enable(struct irq_data *d) >> { >> struct gpio_chip *gc = irq_data_get_irq_chip_data(d); >> >> - gpiochip_enable_irq(gc, d->hwirq); >> - if (gc->irq.irq_enable) >> - gc->irq.irq_enable(d); >> - else >> - gc->irq.chip->irq_unmask(d); >> + gpiochip_enable_irq(chip, d->hwirq); > > You really never compiled this, did you? > > I've stopped looking at this. Please send something that you will have > actually tested. > > M. Apologies. I did tested out on internal devices based on kernel 5.4 as well as linux-next with sc7180. While posting i somehow taken patch from kernel 5.4 and messed up this patch during merge conflicts. I fixed this in v2 and posted out changes that cleanly applies on latest linux-next tag (next-20200521). Thanks, Maulik
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index eaa0e20..a8fdc74 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2465,33 +2465,38 @@ static void gpiochip_irq_relres(struct irq_data *d) gpiochip_relres_irq(gc, d->hwirq); } +static void gpiochip_irq_mask(struct irq_data *d) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + + if (chip->irq.irq_mask) + chip->irq.irq_mask(d); + gpiochip_disable_irq(chip, d->hwirq); +} + +static void gpiochip_irq_unmask(struct irq_data *d) +{ + struct gpio_chip *chip = irq_data_get_irq_chip_data(d); + + gpiochip_enable_irq(chip, d->hwirq); + if (chip->irq.irq_unmask) + chip->irq.irq_unmask(d); +} + static void gpiochip_irq_enable(struct irq_data *d) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); - gpiochip_enable_irq(gc, d->hwirq); - if (gc->irq.irq_enable) - gc->irq.irq_enable(d); - else - gc->irq.chip->irq_unmask(d); + gpiochip_enable_irq(chip, d->hwirq); + chip->irq.irq_enable(d); } static void gpiochip_irq_disable(struct irq_data *d) { struct gpio_chip *gc = irq_data_get_irq_chip_data(d); - /* - * Since we override .irq_disable() we need to mimic the - * behaviour of __irq_disable() in irq/chip.c. - * First call .irq_disable() if it exists, else mimic the - * behaviour of mask_irq() which calls .irq_mask() if - * it exists. - */ - if (gc->irq.irq_disable) - gc->irq.irq_disable(d); - else if (gc->irq.chip->irq_mask) - gc->irq.chip->irq_mask(d); - gpiochip_disable_irq(gc, d->hwirq); + chip->irq.irq_disable(d); + gpiochip_disable_irq(chip, d->hwirq); } static void gpiochip_set_irq_hooks(struct gpio_chip *gc) @@ -2515,10 +2520,22 @@ static void gpiochip_set_irq_hooks(struct gpio_chip *gc) "detected irqchip that is shared with multiple gpiochips: please fix the driver.\n"); return; } - gc->irq.irq_enable = irqchip->irq_enable; - gc->irq.irq_disable = irqchip->irq_disable; - irqchip->irq_enable = gpiochip_irq_enable; - irqchip->irq_disable = gpiochip_irq_disable; + + if (irqchip->irq_disable) { + gpiochip->irq.irq_disable = irqchip->irq_disable; + irqchip->irq_disable = gpiochip_irq_disable; + } else { + gpiochip->irq.irq_mask = irqchip->irq_mask; + irqchip->irq_mask = gpiochip_irq_mask; + } + + if (irqchip->irq_enable) { + gpiochip->irq.irq_enable = irqchip->irq_enable; + irqchip->irq_enable = gpiochip_irq_enable; + } else { + gpiochip->irq.irq_unmask = irqchip->irq_unmask; + irqchip->irq_unmask = gpiochip_irq_unmask; + } } /** diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index 8c41ae4..c8bcaf3 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -253,6 +253,19 @@ struct gpio_irq_chip { * Store old irq_chip irq_disable callback */ void (*irq_disable)(struct irq_data *data); + /** + * @irq_unmask: + * + * Store old irq_chip irq_unmask callback + */ + void (*irq_unmask)(struct irq_data *data); + + /** + * @irq_mask: + * + * Store old irq_chip irq_mask callback + */ + void (*irq_mask)(struct irq_data *data); }; /**
With 'commit 461c1a7d4733 ("gpiolib: override irq_enable/disable")' gpiolib overrides irqchip's irq_enable and irq_disable callbacks. If irq_disable callback is implemented then genirq takes unlazy path to disable irq. Underlying irqchip may not want to implement irq_disable callback to lazy disable irq when client drivers invokes disable_irq(). By overriding irq_disable callback, gpiolib ends up always unlazy disabling IRQ. Allow gpiolib to lazy disable IRQs by overriding irq_disable callback only if irqchip implemented irq_disable. In cases where irq_disable is not implemented irq_mask is overridden. Similarly override irq_enable callback only if irqchip implemented irq_enable otherwise irq_unmask is overridden. Fixes: 461c1a7d47 (gpiolib: override irq_enable/disable) Signed-off-by: Maulik Shah <mkshah@codeaurora.org> --- drivers/gpio/gpiolib.c | 59 +++++++++++++++++++++++++++++---------------- include/linux/gpio/driver.h | 13 ++++++++++ 2 files changed, 51 insertions(+), 21 deletions(-)