All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] gpio: add a real time compliance checklist
@ 2015-10-02 21:31 Linus Walleij
  2015-10-12  8:48 ` Linus Walleij
  0 siblings, 1 reply; 4+ messages in thread
From: Linus Walleij @ 2015-10-02 21:31 UTC (permalink / raw)
  To: linux-gpio; +Cc: Alexandre Courbot, Linus Walleij, Grygorii Strashko

Add some information about real time compliance to the driver document.
Inspired by Grygorii Strashko's real time compliance patches.

Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Grygorii: can you help out with some heavy comments on all I do wrong
in this document? Thx. For example I have no real clue of the difference
between generic_handle_irq() and handle_nested_irq() in this context.

We also need to think about new helper functions that can aid driver writers
in getting drivers real time compliant and properly preemptible. Should we
have the goal to make *all* drivers using the helpers use nested/threaded IRQs,
or do you think there are (old) systems around that would suffer too much
from this?
---
 Documentation/gpio/driver.txt | 72 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 60 insertions(+), 12 deletions(-)

diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
index 90d0f6aba7a6..9d7985171f07 100644
--- a/Documentation/gpio/driver.txt
+++ b/Documentation/gpio/driver.txt
@@ -93,22 +93,37 @@ GPIO irqchips usually fall in one of two categories:
   Chained GPIO irqchips typically can NOT set the .can_sleep flag on
   struct gpio_chip, as everything happens directly in the callbacks.
 
-* NESTED THREADED GPIO irqchips: these are off-chip GPIO expanders and any
-  other GPIO irqchip residing on the other side of a sleeping bus. Of course
-  such drivers that need slow bus traffic to read out IRQ status and similar,
-  traffic which may in turn incur other IRQs to happen, cannot be handled
-  in a quick IRQ handler with IRQs disabled. Instead they need to spawn a
-  thread and then mask the parent IRQ line until the interrupt is handled
-  by the driver. The hallmark of this driver is to call something like
-  this in its interrupt handler:
+  NOTE: chained IRQ handlers are usually not good for real time. If you
+  are submitting a new driver or refactoring a driver for real time compliance,
+  consider using creating a nested/threaded irqchip instead, see below.
+
+* NESTED THREADED GPIO irqchips: these are traditionally off-chip GPIO
+  expanders and any other GPIO irqchip residing on the other side of a
+  sleeping bus. Of course such drivers that need slow bus traffic to read
+  out IRQ status and similar, traffic which may in turn incur other IRQs to
+  happen, cannot be handled in a quick IRQ handler with IRQs disabled.
+
+  With the introduction of real time support in the Linux kernel, also other
+  GPIO irqchips are encouraged to use a nested and threaded IRQ handler.
+  Doing so makes the interrupts naturally preemptible on a real time
+  setup, which means the system can easily be configured for real time with
+  a (usually negligable) performance loss.
+
+  These drivers spawn a thread and then mask the parent IRQ line until the
+  interrupt is handled by the driver. The hallmark of this driver is to call
+  something like this in its interrupt handler:
 
   static irqreturn_t tc3589x_gpio_irq(int irq, void *data)
       ...
       handle_nested_irq(irq);
+      OR
+      generic_handle_irq(irq);
 
-  The hallmark of threaded GPIO irqchips is that they set the .can_sleep
-  flag on struct gpio_chip to true, indicating that this chip may sleep
-  when accessing the GPIOs.
+  Threaded GPIO irqchips should set the .can_sleep flag on struct gpio_chip
+  to true if they are e.g. accessing the chip over I2C or SPI, indicating that
+  this chip may sleep when accessing the GPIOs. irqchips that are just made
+  threaded to be preemptible and thus real time compliant need not do this:
+  preemption is not sleeping.
 
 To help out in handling the set-up and management of GPIO irqchips and the
 associated irqdomain and resource allocation callbacks, the gpiolib has
@@ -125,7 +140,7 @@ symbol:
   gpio_chip from a parent IRQ and passes the struct gpio_chip* as handler
   data. (Notice handler data, since the irqchip data is likely used by the
   parent irqchip!) This is for the chained type of chip. This is also used
-  to set up a nested irqchip if NULL is passed as handler.
+  to set up a threaded/nested irqchip if NULL is passed as handler.
 
 To use the helpers please keep the following in mind:
 
@@ -170,6 +185,39 @@ typically be called in the .startup() and .shutdown() callbacks from the
 irqchip.
 
 
+Real-Time compliance for GPIO IRQ chips
+---------------------------------------
+
+Any provider of irqchips needs to be carefully tailored to support Real Time
+preemption. It is desireable that all irqchips in the GPIO subsystem keep this
+in mind and does the proper testing to assure they are real time-enabled. The
+following is a checklist to follow when preparing a driver for real
+time-compliance:
+
+- Nominally use raw_spinlock_t in the IRQ context path of the IRQ handler as
+  we do not want these sections to be preempted.
+
+- Do NOT use chained_irq_enter() or chained_irq_exit() in the IRQ handler,
+  as we want the hotpath to be preemptible.
+
+- Instead use nested IRQs and generic handlers such as handle_bad_irq(),
+  handle_level_irq() and handle_edge_irq(). Consequentally the handler
+  argument of gpiochip_set_chained_irqchip() should be NULL when using the
+  gpiolib irqchip helpers.
+
+- Nominally set all handlers to handle_bad_irq() in the setup call, then
+  set the handler to handle_level_irq() and/or handle_edge_irq() in the irqchip
+  .set_type() callback depending on what your controller supports.
+
+- If you need to use the pm_runtime_get*()/pm_runtime_put*() callbacks in some
+  of the irqchip callbacks, these should be moved to the .irq_bus_lock()
+  and .irq_bus_unlock() callbacks respectively, as these are the only
+  slowpath callbacks on an irqchip. Create the callbacks if need be.
+
+- Test your driver with the apropriate in-kernel real time test cases for both
+  level and edge IRQs.
+
+
 Requesting self-owned GPIO pins
 -------------------------------
 
-- 
2.4.3


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] gpio: add a real time compliance checklist
  2015-10-02 21:31 [PATCH] gpio: add a real time compliance checklist Linus Walleij
@ 2015-10-12  8:48 ` Linus Walleij
  2015-10-20 14:22   ` Grygorii Strashko
  0 siblings, 1 reply; 4+ messages in thread
From: Linus Walleij @ 2015-10-12  8:48 UTC (permalink / raw)
  To: linux-gpio, Grygorii Strashko; +Cc: Alexandre Courbot, Linus Walleij

On Fri, Oct 2, 2015 at 11:31 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> Add some information about real time compliance to the driver document.
> Inspired by Grygorii Strashko's real time compliance patches.
>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Grygorii: can you help out with some heavy comments on all I do wrong
> in this document? Thx. For example I have no real clue of the difference
> between generic_handle_irq() and handle_nested_irq() in this context.
>
> We also need to think about new helper functions that can aid driver writers
> in getting drivers real time compliant and properly preemptible. Should we
> have the goal to make *all* drivers using the helpers use nested/threaded IRQs,
> or do you think there are (old) systems around that would suffer too much
> from this?

Poking Grygorii about this!

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] gpio: add a real time compliance checklist
  2015-10-12  8:48 ` Linus Walleij
@ 2015-10-20 14:22   ` Grygorii Strashko
  2015-10-27 10:15     ` Linus Walleij
  0 siblings, 1 reply; 4+ messages in thread
From: Grygorii Strashko @ 2015-10-20 14:22 UTC (permalink / raw)
  To: Linus Walleij, linux-gpio; +Cc: Alexandre Courbot

On 10/12/2015 11:48 AM, Linus Walleij wrote:
> On Fri, Oct 2, 2015 at 11:31 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
>> Add some information about real time compliance to the driver document.
>> Inspired by Grygorii Strashko's real time compliance patches.
>>
>> Cc: Grygorii Strashko <grygorii.strashko@ti.com>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> Grygorii: can you help out with some heavy comments on all I do wrong
>> in this document? Thx. For example I have no real clue of the difference
>> between generic_handle_irq() and handle_nested_irq() in this context.
>>
>> We also need to think about new helper functions that can aid driver writers
>> in getting drivers real time compliant and properly preemptible. Should we
>> have the goal to make *all* drivers using the helpers use nested/threaded IRQs,
>> or do you think there are (old) systems around that would suffer too much
>> from this?
> 
> Poking Grygorii about this!

I'm very sorry for delay (a month-long business trip made me non-functional for some time)

Below is what i come up with:

Subject: [PATCH] gpio: add a real time compliance notes

---
 Documentation/gpio/driver.txt | 80 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
index 90d0f6a..12a6194 100644
--- a/Documentation/gpio/driver.txt
+++ b/Documentation/gpio/driver.txt
@@ -62,6 +62,11 @@ Any debugfs dump method should normally ignore signals which haven't been
 requested as GPIOs. They can use gpiochip_is_requested(), which returns either
 NULL or the label associated with that GPIO when it was requested.
 
+RT_FULL: GPIO driver should not use spinlock_t or any sleepable APIs
+(like PM runtime) in its gpio_chip implementation (.get/.set and direction
+control callbacks) if it is expected to call GPIO APIs from atomic context
+on -RT (inside hard IRQ handlers and similar contexts). Normally this should
+not be required.
 
 GPIO drivers providing IRQs
 ---------------------------
@@ -73,6 +78,13 @@ The IRQ portions of the GPIO block are implemented using an irqchip, using
 the header <linux/irq.h>. So basically such a driver is utilizing two sub-
 systems simultaneously: gpio and irq.
 
+RT_FULL: GPIO driver should not use spinlock_t or any sleepable APIs
+(like PM runtime) as part of its irq_chip implementation on -RT.
+- spinlock_t should be replaced with raw_spinlock_t [1].
+- If sleepable APIs have to be used, these can be done from the .irq_bus_lock()
+  and .irq_bus_unlock() callbacks, as these are the only slowpath callbacks
+  on an irqchip. Create the callbacks if needed [2].
+
 GPIO irqchips usually fall in one of two categories:
 
 * CHAINED GPIO irqchips: these are usually the type that is embedded on
@@ -93,6 +105,38 @@ GPIO irqchips usually fall in one of two categories:
   Chained GPIO irqchips typically can NOT set the .can_sleep flag on
   struct gpio_chip, as everything happens directly in the callbacks.
 
+  RT_FULL: Note, chained IRQ handlers will not be forced threaded on -RT.
+  As result, spinlock_t or any sleepable APIs (like PM runtime) can't be used
+  in chained IRQ handler.
+  if required (and if it can't be converted to the nested threaded GPIO irqchip)
+  - chained IRQ handler can be converted to generic irq handler and this way
+  it will be threaded IRQ handler on -RT and hard IRQ handler on non-RT
+  (for example, see [3]).
+  Know W/A: The generic_handle_irq() is expected to be called with IRQ disabled,
+  so IRQ core will complain if it will be called from IRQ handler wich is forced
+  thread. The "fake?" raw lock can be used to W/A this problem:
+
+	raw_spinlock_t wa_lock;
+	static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
+		unsigned long wa_lock_flags;
+		raw_spin_lock_irqsave(&bank->wa_lock, wa_lock_flags);
+		generic_handle_irq(irq_find_mapping(bank->chip.irqdomain, bit));
+		raw_spin_unlock_irqrestore(&bank->wa_lock, wa_lock_flags);
+
+* GENERIC CHAINED GPIO irqchips: these are the same as "CHAINED GPIO irqchips",
+  but chained IRQ handlers are not used. Instead GPIO IRQs dispatching is
+  performed by generic IRQ handler which is configured using request_irq().
+  The GPIO irqchip will then end up calling something like this sequence in
+  its interrupt handler:
+
+  static irqreturn_t gpio_rcar_irq_handler(int irq, void *dev_id)
+	for each detected GPIO IRQ
+		generic_handle_irq(...);
+
+  RT_FULL: Such kind of handlers will be forced threaded on -RT, as result IRQ
+  core will complain that generic_handle_irq() is called with IRQ enabled and
+  the same W/A as for "CHAINED GPIO irqchips" can be applied.
+
 * NESTED THREADED GPIO irqchips: these are off-chip GPIO expanders and any
   other GPIO irqchip residing on the other side of a sleeping bus. Of course
   such drivers that need slow bus traffic to read out IRQ status and similar,
@@ -133,6 +177,13 @@ To use the helpers please keep the following in mind:
   the irqchip can initialize. E.g. .dev and .can_sleep shall be set up
   properly.
 
+- Nominally set all handlers to handle_bad_irq() in the setup call and pass
+  handle_bad_irq() as flow handler parameter in gpiochip_irqchip_add() if it is
+  expected for GPIO driver that irqchip .set_type() callback have to be called
+  before using/enabling GPIO IRQ. Then set the handler to handle_level_irq()
+  and/or handle_edge_irq() in the irqchip .set_type() callback depending on
+  what your controller supports.
+
 It is legal for any IRQ consumer to request an IRQ from any irqchip no matter
 if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
 irq_chip are orthogonal, and offering their services independent of each
@@ -169,6 +220,31 @@ When implementing an irqchip inside a GPIO driver, these two functions should
 typically be called in the .startup() and .shutdown() callbacks from the
 irqchip.
 
+Real-Time compliance for GPIO IRQ chips
+---------------------------------------
+
+Any provider of irqchips needs to be carefully tailored to support Real Time
+preemption. It is desireable that all irqchips in the GPIO subsystem keep this
+in mind and does the proper testing to assure they are real time-enabled.
+So, pay attention on above " RT_FULL:" notes, please.
+The following is a checklist to follow when preparing a driver for real
+time-compliance:
+
+- ensure spinlock_t is not used as part irq_chip implementation;
+- ensure that sleepable APIs are not used as part irq_chip implementation.
+  If sleepable APIs have to be used, these can be done from the .irq_bus_lock()
+  and .irq_bus_unlock() callbacks;
+- Chained GPIO irqchips: ensure spinlock_t or any sleepable APIs are not used
+  from chained IRQ handler;
+- Generic chained GPIO irqchips: take care about generic_handle_irq() calls and
+  apply corresponding W/A;
+- Chained GPIO irqchips: get rid of chained IRQ handler and use generic irq
+  handler if possible :)
+- regmap_mmio: Sry, but you are in trouble :( if MMIO regmap is used as for
+  GPIO IRQ chip implementation;
+- Test your driver with the appropriate in-kernel real time test cases for both
+  level and edge IRQs.
+
 
 Requesting self-owned GPIO pins
 -------------------------------
@@ -190,3 +266,7 @@ gpiochip_free_own_desc().
 These functions must be used with care since they do not affect module use
 count. Do not use the functions to request gpio descriptors not owned by the
 calling driver.
+
+[1] http://www.spinics.net/lists/linux-omap/msg120425.html
+[2] https://lkml.org/lkml/2015/9/25/494
+[3] https://lkml.org/lkml/2015/9/25/495
-- 
2.5.1

-- 
regards,
-grygorii

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] gpio: add a real time compliance checklist
  2015-10-20 14:22   ` Grygorii Strashko
@ 2015-10-27 10:15     ` Linus Walleij
  0 siblings, 0 replies; 4+ messages in thread
From: Linus Walleij @ 2015-10-27 10:15 UTC (permalink / raw)
  To: Grygorii Strashko; +Cc: linux-gpio, Alexandre Courbot

On Tue, Oct 20, 2015 at 4:22 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

>> Poking Grygorii about this!
>
> I'm very sorry for delay (a month-long business trip made me non-functional for some time)
>
> Below is what i come up with:
>
> Subject: [PATCH] gpio: add a real time compliance notes

Thanks a lot, I reverted my own amateurish docs and applied
this instead.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-10-27 10:15 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-02 21:31 [PATCH] gpio: add a real time compliance checklist Linus Walleij
2015-10-12  8:48 ` Linus Walleij
2015-10-20 14:22   ` Grygorii Strashko
2015-10-27 10:15     ` Linus Walleij

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.