On Fri, 2014-11-07 at 14:16 +0100, Hannes Reinecke wrote:
> 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
OK, I'm losing track of how we handle interrupts, so this shows all
interrupt routines are now run with interrupts disabled.
> > 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.
That's fine ... irqsave/restore makes sure it always works.
> >> 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.
As long as the assertion in the comment that the lock is locked is
correct it's OK (and if it weren't true, we'd have deadlocked). All we
care about is enabling interrupts during the potentially long udelay so
the timer still ticks and the softlockup dosn't go off.
So I think the code you're removing here is doing what it's supposed to.
James
> Unless we manage to figure out the real reason for the above kernel
> splat, in which case we can leave things in.
>
> Cheers,
>
> Hannes
��.n��������+%������w��{.n�����{������ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
