On Wed, May 16, 2018 at 03:51:14PM +0100, James Morse wrote:
> The first two overload existing architectural behavior, the third improves all
> this with a third standard option. Its the standard story!
:-)
> I thought this was safe because its just ghes_copy_tofrom_phys()s access to the
> fixmap slots that needs mutual exclusion.
>
> Polled and all the IRQ flavours are kept apart by the spin_lock_irqsave(), and
> the NMIs have their own fixmap entry. (This is fine until there is more than
> once source of NMI)
For example:
ghes_probe()
switch (generic->notify.type) {
...
case ACPI_HEST_NOTIFY_NMI:
ghes_nmi_add(ghes);
}
...
ghes_proc();
ghes_read_estatus();
spin_lock_irqsave(&ghes_ioremap_lock_irq, flags);
memcpy...
-> NMI
ghes_notify_nmi();
ghes_read_estatus();
..
if (in_nmi) {
raw_spin_lock(&ghes_ioremap_lock_nmi);
...
<- NMI
ghes->estatus from above, before the NMI fired, has gotten some nice
scribbling over. AFAICT.
Now, I don't know whether this can happen with the ARM facilities but if
they're NMI-like, I don't see why not.
Which means, that this code is not really reentrant and if should be
fixed to be callable from different contexts, then it should use private
buffers and be careful about locking.
Oh, and that
if (in_nmi)
lock()
else
lock_irqsave()
pattern is really yucky. And it is an explosion waiting to happen.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
