Re: [PATCH 21/22] iwlwifi: change spin_lock to spin_lock_irqsave

["Guy, Wey-Yi" <wey-yi.w.guy@intel.com>]

Hi Pavel,

On Thu, 2010-03-25 at 14:10 -0700, Pavel Roskin wrote:
> On Thu, 2010-03-25 at 13:44 -0700, Reinette Chatre wrote:
> > From: Wey-Yi Guy <wey-yi.w.guy@intel.com>
> > 
> > Use spin_lock_irqsave() in interrupt handler to disable interrupts locally
> > and provide the spinlock on SMP. This covers both interrupt and SMP
> > concurrency.
> > 
> > With this changes, also fix the sparse warning issues.
> 
> As far as I understand, spin_lock_irqsave() is superfluous in interrupt
> handlers, since interrupt handlers are run with interrupts disabled.
> There are two solutions.  The first is to take the
lock with spin_lock_irqsave() or spin_lock_irq() (see
Documentation/DocBook/kernel-locking).  The second is to specify
IRQF_DISABLED to request_irq() so that the kernel runs the entire
interrupt routine with interrupts disabled.

If your MSI interrupt routine does not hold the lock for the whole time
it is running, the first solution may be best.  The second solution is
normally preferred as it avoids making two transitions from interrupt
disabled to enabled and back again.
> I wonder if you hit the problem that my patch should fix:
> https://patchwork.kernel.org/patch/84441/
> 
> Basically, sparse diagnostics for not fully preemptible SMP kernels is
> so bogus that it should be ignored completely.
> 
Just like Yi's reply, with pin-based interrupts or a single MSI, it is
not necessary to disable interrupts (Linux guarantees the same interrupt
will not be re-entered).  If a device uses multiple interrupts, the
driver must disable interrupts while the lock is held.  If the device
sends a different interrupt, the driver will deadlock trying to
recursively acquire the spinlock.

There are two solutions.  The first is to take the
lock with spin_lock_irqsave(). The second is to specify
IRQF_DISABLED to request_irq() so that the kernel runs the entire
interrupt routine with interrupts disabled.

Since we do not need to hold the lock for the whole time interrupt
is running, the first solution is more suitable for it.

Best Regards
Wey