Skip to content

Commit

Permalink
irq: Request and release resources for chained IRQs
Browse files Browse the repository at this point in the history
This addresses a bug in the irqchip core that was triggered
by new code assuming a strict semantic order of the irqchip
calls:

 .irq_request_resources()
 .irq_enable()
 .irq_disable()
 .irq_release_resources()

Mostly this is working fine when handled inside manage.c,
the calls are strictly happening in the above assumed order.

However on a Qualcomm platform I get the following in dmesg:

WARNING: CPU: 0 PID: 1 at ../drivers/gpio/gpiolib.c:3513
	 gpiochip_irq_enable+0x18/0x44
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0
     Not tainted 4.20.0-rc4-00002-g1b8a75b25c6e-dirty grate-driver#9
Hardware name: Generic DT based system
[<c03124ac>] (unwind_backtrace) from [<c030d994>] (show_stack+0x10/0x14)
[<c030d994>] (show_stack) from [<c0b48aec>] (dump_stack+0x78/0x8c)
[<c0b48aec>] (dump_stack) from [<c03213e4>] (__warn+0xe0/0xf8)
[<c03213e4>] (__warn) from [<c0321514>] (warn_slowpath_null+0x40/0x48)
[<c0321514>] (warn_slowpath_null) from [<c06ad5d0>]
	     (gpiochip_irq_enable+0x18/0x44)
[<c06ad5d0>] (gpiochip_irq_enable) from [<c0371198>] (irq_enable+0x44/0x64)
[<c0371198>] (irq_enable) from [<c0371258>] (__irq_startup+0xa0/0xa8)
[<c0371258>] (__irq_startup) from [<c03712ac>] (irq_startup+0x4c/0x130)
[<c03712ac>] (irq_startup) from [<c03716d0>]
	     (irq_set_chained_handler_and_data+0x4c/0x78)
[<c03716d0>] (irq_set_chained_handler_and_data) from [<c081c17c>]
	     (pm8xxx_probe+0x160/0x22c)
[<c081c17c>] (pm8xxx_probe) from [<c07f439c>] (platform_drv_probe+0x48/0x98)

What happens is that when the pm8xxx driver tries to register
a chained IRQ irq_set_chained_handler_and_data() will eventually
set the type and call irq_startup() and thus the .irq_enable()
callback on the irqchip.

This irqchip is in drivers/pinctrl/qcom/pinctrl-msm.c and known
as TLMM.

However, the irqchip core helpers in GPIO assumes that
the .irq_request_resources() callback is always called before
.irq_enable(), and the latter is where module refcount and
also gpiochip_lock_as_irq() is normally called.

When .irq_enable() is called without .irq_request_resources()
having been called first, it seems like we are enabling
an IRQ on a GPIO line that has not first been locked to be
used as IRQ and we get the above warning. This happens since
as of
commit 461c1a7 ("gpiolib: override irq_enable/disable")
this is strictly assumed for all GPIO-based IRQs.

As it seems reasonable to assume that .irq_request_resources()
is always strictly called before .irq_enable(), we add the
irq_[request|release]_resources() functions to the internal
API and call them also when adding a chained irqchip to
an IRQ.

I am a bit uncertain about the call site for
irq_released_resources() inside chip.c, but it appears this
path is for uninstalling a chained handler.

Cc: [email protected]
Cc: Bjorn Andersson <[email protected]>
Cc: Hans Verkuil <[email protected]>
Fixes: 461c1a7 ("gpiolib: override irq_enable/disable")
Signed-off-by: Linus Walleij <[email protected]>
  • Loading branch information
linusw authored and okias committed Sep 3, 2020
1 parent 677f1cd commit d9ece0a
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 2 deletions.
2 changes: 2 additions & 0 deletions kernel/irq/chip.c
Original file line number Diff line number Diff line change
Expand Up @@ -1008,6 +1008,7 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
if (desc->irq_data.chip != &no_irq_chip)
mask_ack_irq(desc);
irq_state_set_disabled(desc);
irq_release_resources(desc);
if (is_chained)
desc->action = NULL;
desc->depth = 1;
Expand Down Expand Up @@ -1035,6 +1036,7 @@ __irq_do_set_handler(struct irq_desc *desc, irq_flow_handler_t handle,
irq_settings_set_norequest(desc);
irq_settings_set_nothread(desc);
desc->action = &chained_action;
irq_request_resources(desc);
irq_activate_and_startup(desc, IRQ_RESEND);
}
}
Expand Down
3 changes: 3 additions & 0 deletions kernel/irq/internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ extern void __enable_irq(struct irq_desc *desc);
#define IRQ_START_FORCE true
#define IRQ_START_COND false

extern int irq_request_resources(struct irq_desc *desc);
extern void irq_release_resources(struct irq_desc *desc);

extern int irq_activate(struct irq_desc *desc);
extern int irq_activate_and_startup(struct irq_desc *desc, bool resend);
extern int irq_startup(struct irq_desc *desc, bool resend, bool force);
Expand Down
4 changes: 2 additions & 2 deletions kernel/irq/manage.c
Original file line number Diff line number Diff line change
Expand Up @@ -1254,15 +1254,15 @@ static int irq_setup_forced_threading(struct irqaction *new)
return 0;
}

static int irq_request_resources(struct irq_desc *desc)
int irq_request_resources(struct irq_desc *desc)
{
struct irq_data *d = &desc->irq_data;
struct irq_chip *c = d->chip;

return c->irq_request_resources ? c->irq_request_resources(d) : 0;
}

static void irq_release_resources(struct irq_desc *desc)
void irq_release_resources(struct irq_desc *desc)
{
struct irq_data *d = &desc->irq_data;
struct irq_chip *c = d->chip;
Expand Down

0 comments on commit d9ece0a

Please sign in to comment.