[PATCH v2 3/3] ARM: vfp: Fix broken softirq handling with instrumentation enabled

[Linus Walleij]

Hi Ard,

On Tue, Mar 14, 2023 at 8:28 PM Ard Biesheuvel <ardb at kernel.org> wrote:

> Commit 62b95a7b44d1 ("ARM: 9282/1: vfp: Manipulate task VFP state with
> softirqs disabled") replaced the en/disable preemption calls inside the
> VFP state handling code with en/disabling of soft IRQs, which is
> necessary to allow kernel use of the VFP/SIMD unit when handling a soft
> IRQ.
>
> Unfortunately, when lockdep is enabled (or other instrumentation that
> enables TRACE_IRQFLAGS), the disable path implemented in asm fails to
> perform the lockdep and RCU related bookkeeping, resulting in spurious
> warnings and other badness.

Not good.

> Set let's rework the VFP entry code a little bit so we can make the
> local_bh_disable() call from C, with all the instrumentations that
> happen to have been configured. Calling local_bh_enable() can be done
> from asm, as it is always a callable function.

Here it says local_bh_enable() is called

(...)

> +local_bh_enable_and_ret:
> +       adr     r0, .
> +       mov     r1, #SOFTIRQ_DISABLE_OFFSET
> +       b       __local_bh_enable_ip    @ tail call

Please add a comment here that this is an open coded version of
local_bh_enable() from the <linux/bottom_half.h> header file
and we hope that inline code will not change.

I.e this:

static inline void local_bh_enable(void)
{
        __local_bh_enable_ip(_THIS_IP_, SOFTIRQ_DISABLE_OFFSET);
}

I almost want to make a warning comment into <linux/bottom_half.h>
that the same function exists in an open coded version in arch/arm/vfp/vfphw.S
so this static inline cannot be refactored without consequences.

Either way:
Reviewed-by: Linus Walleij <linus.walleij at linaro.org>

The kernel definitely looks better after this than before and it fixes
a bug/bugs.

Yours,
Linus Walleij