On 2018-05-23 09:46:30 [+0100], John Garry wrote:
> On 22/05/2018 18:31, Sebastian Andrzej Siewior wrote:
> > On 2018-05-18 14:31:27 [+0100], John Garry wrote:
> > > On 04/05/2018 15:50, Sebastian Andrzej Siewior wrote:
> > > > Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
> > > > management duties from lldds") the sas_ata_qc_issue() function unlocks
> > > > the ata_port.lock and disables interrupts before doing so.
> > > > That lock is always taken with disabled interrupts so at this point, the
> > > > interrupts are already disabled. There is no need to disable the
> > > > interrupts before the unlock operation because they are already
> > > > disabled.
> > >
> > > Is the only caller ata_qc_issue(), which has spin_lock_irqsave(host lock)
> > > enabled?
> >
> > I don't understand the question.
>
> It seems to me that ap->lock can be taken in interrupt context, so then we
> know it should be locked with interrupts disabled always.
yes, the comment above the function says so, too.
> I was just really asking how you know interrupts are disabled already? Maybe
> it's obvious.
The lock has to be taken with interrupts disabled. If the interrupts
were enabled at the time sas_ata_qc_issue() was invoked then the lock
was already taken in a bad way and disabling interrupts before the
unlock does not make it any better.
To verify that the locking is okay you can build the kernel with lockdep
enabled and CONFIG_PROVE_LOCKING=y set. That way lockdep will inform you
as soon as it notices the lock taken in interrupt context and in
process-context with enabled interrupts.
> cheers,
Sebastian
