Re: [PATCH] tmscsim: fix spinlock usage in isr

[Hannes Reinecke <hare@suse.de>]

On 11/07/2014 02:09 PM, James Bottomley wrote:
> On Fri, 2014-11-07 at 13:49 +0100, Hannes Reinecke wrote:
>> Loading the tmscsim driver would result in kernel splat as
>> the interrupt service routine would only use spin_lock_irq,
>> not spin_lock_irqsave.
> 
> I don't understand the reasoning ... what kernel splat?
>

This one:
kernel: [   13.748186] ------------[ cut here ]------------
kernel: [   13.748195] WARNING: CPU: 0 PID: 214 at
kernel/irq/handle.c:147 handle_irq_event_percpu+0x18b/0x1a0()
kernel: [   13.748200] irq 20 handler do_DC390_Interrupt+0x0/0x9d0
[tmscsim] enabled interrupts
kernel: [   13.748214] Modules linked in: crct10dif_pclmul
crc32_pclmul ghash_clmulni_intel aesni_intel aes_x86_64 lrw gf128mul
glue_helper bochs_drm(+) ttm ablk_helper drm_kms_helper pvpanic
pcspkr cryptd drm tmscsim(+) serio_raw i2c_i801 syscopyarea
sysfillrect lpc_ich mfd_core igbvf sysimgblt 8250_fintek processor
thermal_sys button dm_mod efivarfs sr_mod cdrom btrfs xor raid6_pq
sd_mod crc32c_intel ahci libahci libata megaraid_sas floppy sg autofs4
kernel: [   13.748216] CPU: 0 PID: 214 Comm: kworker/0:1H Not
tainted 3.18.0-rc2-default+ #104

> Linux runs interrupt routines with interrupts enabled, so you know the
> irq state on entry; that's why you can do spin_lock_irq ... because you
> know it was enabled before and now it's disabled.  It only saves a
> couple of instructions, so there's no real point using a different
> pattern, but there's no reason to change it either, because it's
> harmless when done correctly.
> 

Well, apparently it's not done correctly, then :-)
I must admit is got the 'solution' off google, so I cannot vouch for
the correctness here. The only thing I know is that with switching
to the irqsave/irqrestore version the warning went away.

>> And while we're at it there is not need to release the
>> spin lock when calling udelay; it's implemented via a
>> processor counter, not via interrupts.
> 
> That looks a bit incorrect:  The potential timeout is jiffies + HZ, i.e.
> up to 1 second of udelay.  If it didn't drop and retake the lock (and
> enable interrupts), it would spin with interrupts disabled for up to a
> second ... with all the badness that would result from that.
> 
Hmm. I could put them back in, but for completeness one would have
to pass the irq flags taken in the outer routine to avoid lock
imbalancing.

Unless we manage to figure out the real reason for the above kernel
splat, in which case we can leave things in.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html