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