[Cocci] [PATCH v2 0/9] fixup usage of non-raw spinlocks in irqchips

[Cocci] [PATCH v2 0/9] fixup usage of non-raw spinlocks in irqchips

[Julia Cartwright]

Changes since v1[1]:
 - Dropped already applied patches in pinctrl, gpio, and mux trees.
 - Gained three new patches destined for gpio tree, due to loosening constraints in
   the semantic patch.

The following patchset introduces a new coccinelle patch,
irq_chip_raw_spinlock.cocci, which is used to identify irq_chip implementors
which acquire/release non-raw spinlocks, and in addition, a set of generated
patches for most cases identified.

On mainline builds, there exists no functional difference between
raw_spinlock_t and spinlock_t.  However, w/ PREEMPT_RT, the spinlock_t will
cause the calling thread to sleep when the lock is contended.  Because sleeping
is illegal in hardirq context, and because the irqchip callbacks are invoked in
hardirq context, irqchip implementations must not use spin_lock_t for
synchronization.

Patches build tested only.  All patches are independent of one another and can
be applied to the relevant trees.

Some notes:

  - In order to ensure that latency problems are not introduced for realtime
    kernels, the generated patches need to be hand audited to ensure that
    raw-spinlock protected regions are bounded and minimal.  I've done a quick
    audit for each of the modified drivers in this series, but please check my
    work.

  - There are a couple of matches which will require further intervention to
    fully fix.  Namely the Intel LPE Audio driver:

       drivers/gpu/drm/i915/intel_lpe_audio.c:169:30-38: Use of non-raw spinlock is illegal in this context (struct drm_i915_private::irq_lock)

    and the adi2 pinctrl driver:

       drivers/pinctrl/pinctrl-adi2.c:308:26-30: Use of non-raw spinlock is illegal in this context (struct gpio_port::lock)

  - This semantic patch is not comprehensive, there are other equally broken
    usecases which are not properly identified yet, such as the use of a single
    global spinlock_t declared via DEFINE_SPINLOCK() (found in arch/alpha, and
    perhaps elsewhere).

    An additional currently unhandled case is the irq_chip_generic callbacks.

   Julia

[1]: http://lkml.kernel.org/r/cover.1489015238.git.julia at ni.com

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Julia Lawall <Julia.Lawall@lip6.fr>
Cc: Gilles Muller <Gilles.Muller@lip6.fr>
Cc: Nicolas Palix <nicolas.palix@imag.fr>
Cc: Michal Marek <mmarek@suse.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: cocci at systeme.lip6.fr
--------------
Julia Cartwright (9):
  Coccinelle: locks: identify callers of spin_lock{,_irq,_irqsave}() in
    irqchip implementations
  alpha: marvel: make use of raw_spinlock variants
  powerpc: mpc52xx_gpt: make use of raw_spinlock variants
  mfd: asic3: make use of raw_spinlock variants
  mfd: t7l66xb: make use of raw_spinlock variants
  mfd: tc6393xb: make use of raw_spinlock variants
  gpio: 104-idi-48: make use of raw_spinlock variants
  gpio: 104-idio-16: make use of raw_spinlock variants
  gpio: pci-idio-16: make use of raw_spinlock variants

 arch/alpha/include/asm/core_marvel.h               |  2 +-
 arch/alpha/kernel/core_marvel.c                    |  2 +-
 arch/alpha/kernel/sys_marvel.c                     | 12 +--
 arch/powerpc/platforms/52xx/mpc52xx_gpt.c          | 52 ++++++------
 drivers/gpio/gpio-104-idi-48.c                     | 18 ++--
 drivers/gpio/gpio-104-idio-16.c                    | 24 +++---
 drivers/gpio/gpio-pci-idio-16.c                    | 28 +++----
 drivers/mfd/asic3.c                                | 56 ++++++-------
 drivers/mfd/t7l66xb.c                              | 20 ++---
 drivers/mfd/tc6393xb.c                             | 52 ++++++------
 .../coccinelle/locks/irq_chip_raw_spinlock.cocci   | 96 ++++++++++++++++++++++
 11 files changed, 230 insertions(+), 132 deletions(-)
 create mode 100644 scripts/coccinelle/locks/irq_chip_raw_spinlock.cocci

-- 
2.12.0

[Cocci] [PATCH v2 1/9] Coccinelle: locks: identify callers of spin_lock{, _irq, _irqsave}() in irqchip implementations

[Julia Cartwright]

On PREEMPT_RT, the spinlock_t type becomes an object which sleeps under
contention.  The codepaths used to support scheduling (irq dispatching, arch
code, the scheduler, timers) therefore must make use of the
raw_spin_lock{,_irq,_irqsave}() variations which preserve the non-sleeping
spinlock behavior.

Because the irq_chip callbacks are invoked in the process of interrupt
dispatch, they cannot therefore make use of spin_lock_t type.  Instead, the
usage of raw_spinlock_t is appropriate.

Provide a spatch to identify (and attempt to patch) such problematic irqchip
implementations.

Note to those generating patches using this spatch; in order to maintain
correct semantics w/ PREEMPT_RT, it is necessary to audit the
raw_spinlock_t-protected codepaths to ensure their execution is bounded and
minimal.  This is a manual audit process.

See commit 47b03ca903fb0 ("pinctrl: qcom: Use raw spinlock variants") as an
example of _one_ such instance, which fixed a real bug seen in the field.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Julia Cartwright <julia@ni.com>
---
v1 -> v2:
  - Make use of 'exists' on match to find any codepath which acquires a lock. (via Julia Lawall)
  - Use 'when any' on '...' matches to loosen constraint (via Julia Lawall).

 .../coccinelle/locks/irq_chip_raw_spinlock.cocci   | 96 ++++++++++++++++++++++
 1 file changed, 96 insertions(+)
 create mode 100644 scripts/coccinelle/locks/irq_chip_raw_spinlock.cocci

diff --git a/scripts/coccinelle/locks/irq_chip_raw_spinlock.cocci b/scripts/coccinelle/locks/irq_chip_raw_spinlock.cocci
new file mode 100644
index 000000000000..0294831297d3
--- /dev/null
+++ b/scripts/coccinelle/locks/irq_chip_raw_spinlock.cocci
@@ -0,0 +1,96 @@
+// Copyright: (C) 2017 National Instruments Corp. GPLv2.
+// Author: Julia Cartwright <julia@ni.com>
+//
+// Identify callers of non-raw spinlock_t functions in hardirq irq_chip
+// callbacks.
+//
+// spin_lock{_irq,_irqsave}(), w/ PREEMPT_RT are "sleeping" spinlocks, and so
+// therefore "sleep" under contention; identify (and potentially patch) callers
+// to use raw_spinlock_t instead.
+//
+// Confidence: Moderate
+
+virtual report
+virtual patch
+
+ at match@
+identifier __irqchip;
+identifier __irq_mask;
+@@
+	static struct irq_chip __irqchip = {
+		.irq_mask	= __irq_mask,
+	};
+
+ at match2 depends on match exists@
+identifier match.__irq_mask;
+identifier data;
+identifier x;
+identifier l;
+type T;
+position j0;
+expression flags;
+@@
+	static void __irq_mask(struct irq_data *data)
+	{
+		... when any
+		T *x;
+		... when any
+(
+		spin_lock_irqsave(&x->l at j0, flags);
+|
+		spin_lock_irq(&x->l at j0);
+|
+		spin_lock(&x->l at j0);
+)
+		... when any
+	}
+
+ at match3 depends on match2 && patch@
+type match2.T;
+identifier match2.l;
+@@
+	T {
+		...
+-		spinlock_t	l;
++		raw_spinlock_t	l;
+		...
+	};
+
+ at match4 depends on match2 && patch@
+type match2.T;
+identifier match2.l;
+expression flags;
+T *x;
+@@
+
+(
+-spin_lock(&x->l)
++raw_spin_lock(&x->l)
+|
+-spin_lock_irqsave(&x->l, flags)
++raw_spin_lock_irqsave(&x->l, flags)
+|
+-spin_lock_irq(&x->l)
++raw_spin_lock_irq(&x->l)
+|
+-spin_unlock(&x->l)
++raw_spin_unlock(&x->l)
+|
+-spin_unlock_irq(&x->l)
++raw_spin_unlock_irq(&x->l)
+|
+-spin_unlock_irqrestore(&x->l, flags)
++raw_spin_unlock_irqrestore(&x->l, flags)
+|
+-spin_lock_init(&x->l)
++raw_spin_lock_init(&x->l)
+)
+
+ at script:python wat depends on match2 && report@
+j0 << match2.j0;
+t << match2.T;
+l << match2.l;
+@@
+
+msg = "Use of non-raw spinlock is illegal in this context (%s::%s)" % (t, l)
+coccilib.report.print_report(j0[0], msg)
-- 
2.12.0

[Cocci] [PATCH v2 1/9] Coccinelle: locks: identify callers of spin_lock{, _irq, _irqsave}() in irqchip implementations

[Julia Lawall]

On Tue, 21 Mar 2017, Julia Cartwright wrote:

> On PREEMPT_RT, the spinlock_t type becomes an object which sleeps under
> contention.  The codepaths used to support scheduling (irq dispatching, arch
> code, the scheduler, timers) therefore must make use of the
> raw_spin_lock{,_irq,_irqsave}() variations which preserve the non-sleeping
> spinlock behavior.
>
> Because the irq_chip callbacks are invoked in the process of interrupt
> dispatch, they cannot therefore make use of spin_lock_t type.  Instead, the
> usage of raw_spinlock_t is appropriate.
>
> Provide a spatch to identify (and attempt to patch) such problematic irqchip
> implementations.
>
> Note to those generating patches using this spatch; in order to maintain
> correct semantics w/ PREEMPT_RT, it is necessary to audit the
> raw_spinlock_t-protected codepaths to ensure their execution is bounded and
> minimal.  This is a manual audit process.
>
> See commit 47b03ca903fb0 ("pinctrl: qcom: Use raw spinlock variants") as an
> example of _one_ such instance, which fixed a real bug seen in the field.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Julia Cartwright <julia@ni.com>

Acked-by: Julia Lawall <julia.lawall@lip6.fr>

> ---
> v1 -> v2:
>   - Make use of 'exists' on match to find any codepath which acquires a lock. (via Julia Lawall)
>   - Use 'when any' on '...' matches to loosen constraint (via Julia Lawall).
>
>  .../coccinelle/locks/irq_chip_raw_spinlock.cocci   | 96 ++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
>  create mode 100644 scripts/coccinelle/locks/irq_chip_raw_spinlock.cocci
>
> diff --git a/scripts/coccinelle/locks/irq_chip_raw_spinlock.cocci b/scripts/coccinelle/locks/irq_chip_raw_spinlock.cocci
> new file mode 100644
> index 000000000000..0294831297d3
> --- /dev/null
> +++ b/scripts/coccinelle/locks/irq_chip_raw_spinlock.cocci
> @@ -0,0 +1,96 @@
> +// Copyright: (C) 2017 National Instruments Corp. GPLv2.
> +// Author: Julia Cartwright <julia@ni.com>
> +//
> +// Identify callers of non-raw spinlock_t functions in hardirq irq_chip
> +// callbacks.
> +//
> +// spin_lock{_irq,_irqsave}(), w/ PREEMPT_RT are "sleeping" spinlocks, and so
> +// therefore "sleep" under contention; identify (and potentially patch) callers
> +// to use raw_spinlock_t instead.
> +//
> +// Confidence: Moderate
> +
> +virtual report
> +virtual patch
> +
> + at match@
> +identifier __irqchip;
> +identifier __irq_mask;
> +@@
> +	static struct irq_chip __irqchip = {
> +		.irq_mask	= __irq_mask,
> +	};
> +
> + at match2 depends on match exists@
> +identifier match.__irq_mask;
> +identifier data;
> +identifier x;
> +identifier l;
> +type T;
> +position j0;
> +expression flags;
> +@@
> +	static void __irq_mask(struct irq_data *data)
> +	{
> +		... when any
> +		T *x;
> +		... when any
> +(
> +		spin_lock_irqsave(&x->l at j0, flags);
> +|
> +		spin_lock_irq(&x->l at j0);
> +|
> +		spin_lock(&x->l at j0);
> +)
> +		... when any
> +	}
> +
> + at match3 depends on match2 && patch@
> +type match2.T;
> +identifier match2.l;
> +@@
> +	T {
> +		...
> +-		spinlock_t	l;
> ++		raw_spinlock_t	l;
> +		...
> +	};
> +
> + at match4 depends on match2 && patch@
> +type match2.T;
> +identifier match2.l;
> +expression flags;
> +T *x;
> +@@
> +
> +(
> +-spin_lock(&x->l)
> ++raw_spin_lock(&x->l)
> +|
> +-spin_lock_irqsave(&x->l, flags)
> ++raw_spin_lock_irqsave(&x->l, flags)
> +|
> +-spin_lock_irq(&x->l)
> ++raw_spin_lock_irq(&x->l)
> +|
> +-spin_unlock(&x->l)
> ++raw_spin_unlock(&x->l)
> +|
> +-spin_unlock_irq(&x->l)
> ++raw_spin_unlock_irq(&x->l)
> +|
> +-spin_unlock_irqrestore(&x->l, flags)
> ++raw_spin_unlock_irqrestore(&x->l, flags)
> +|
> +-spin_lock_init(&x->l)
> ++raw_spin_lock_init(&x->l)
> +)
> +
> + at script:python wat depends on match2 && report@
> +j0 << match2.j0;
> +t << match2.T;
> +l << match2.l;
> +@@
> +
> +msg = "Use of non-raw spinlock is illegal in this context (%s::%s)" % (t, l)
> +coccilib.report.print_report(j0[0], msg)
> --
> 2.12.0
>
>

[Cocci] [PATCH v2 1/9] Coccinelle: locks: identify callers of spin_lock{, _irq, _irqsave}() in irqchip implementations

[Julia Cartwright]

On Wed, Mar 22, 2017 at 10:54:15AM +0100, Julia Lawall wrote:
> On Tue, 21 Mar 2017, Julia Cartwright wrote:
> > On PREEMPT_RT, the spinlock_t type becomes an object which sleeps under
> > contention.  The codepaths used to support scheduling (irq dispatching, arch
> > code, the scheduler, timers) therefore must make use of the
> > raw_spin_lock{,_irq,_irqsave}() variations which preserve the non-sleeping
> > spinlock behavior.
> >
> > Because the irq_chip callbacks are invoked in the process of interrupt
> > dispatch, they cannot therefore make use of spin_lock_t type.  Instead, the
> > usage of raw_spinlock_t is appropriate.
> >
> > Provide a spatch to identify (and attempt to patch) such problematic irqchip
> > implementations.
> >
> > Note to those generating patches using this spatch; in order to maintain
> > correct semantics w/ PREEMPT_RT, it is necessary to audit the
> > raw_spinlock_t-protected codepaths to ensure their execution is bounded and
> > minimal.  This is a manual audit process.
> >
> > See commit 47b03ca903fb0 ("pinctrl: qcom: Use raw spinlock variants") as an
> > example of _one_ such instance, which fixed a real bug seen in the field.
> >
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Julia Cartwright <julia@ni.com>
> 
> Acked-by: Julia Lawall <julia.lawall@lip6.fr>

Thanks, Julia.

How do these semantic patches normally land?  It looks like they quite a
few have gone through the kbuild tree, is this the norm?

Thanks,
   Other Julia

[Cocci] [PATCH v2 1/9] Coccinelle: locks: identify callers of spin_lock{, _irq, _irqsave}() in irqchip implementations

[Julia Lawall]

On Wed, 22 Mar 2017, Julia Cartwright wrote:

> On Wed, Mar 22, 2017 at 10:54:15AM +0100, Julia Lawall wrote:
> > On Tue, 21 Mar 2017, Julia Cartwright wrote:
> > > On PREEMPT_RT, the spinlock_t type becomes an object which sleeps under
> > > contention.  The codepaths used to support scheduling (irq dispatching, arch
> > > code, the scheduler, timers) therefore must make use of the
> > > raw_spin_lock{,_irq,_irqsave}() variations which preserve the non-sleeping
> > > spinlock behavior.
> > >
> > > Because the irq_chip callbacks are invoked in the process of interrupt
> > > dispatch, they cannot therefore make use of spin_lock_t type.  Instead, the
> > > usage of raw_spinlock_t is appropriate.
> > >
> > > Provide a spatch to identify (and attempt to patch) such problematic irqchip
> > > implementations.
> > >
> > > Note to those generating patches using this spatch; in order to maintain
> > > correct semantics w/ PREEMPT_RT, it is necessary to audit the
> > > raw_spinlock_t-protected codepaths to ensure their execution is bounded and
> > > minimal.  This is a manual audit process.
> > >
> > > See commit 47b03ca903fb0 ("pinctrl: qcom: Use raw spinlock variants") as an
> > > example of _one_ such instance, which fixed a real bug seen in the field.
> > >
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > > Cc: Linus Walleij <linus.walleij@linaro.org>
> > > Signed-off-by: Julia Cartwright <julia@ni.com>
> >
> > Acked-by: Julia Lawall <julia.lawall@lip6.fr>
>
> Thanks, Julia.
>
> How do these semantic patches normally land?  It looks like they quite a
> few have gone through the kbuild tree, is this the norm?

Michal Marek takes care of them.

julia

>
> Thanks,
>    Other Julia
>