On 2018/5/23 17:24, Sebastian Andrzej Siewior wrote:
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.
For libsas, ata_qc_issue() now have three callers:
1. sas_queuecommand(), the normal io patch, have taken ap->lock with
irq disabled
2. ata_exec_internal_sg(), from EH context, have taken ap->lock with
irq disabled too
3. __ata_qc_complete(), from io completion path, have taken ap->lock
with irq disabled too(this may be in interrupts context)
I think it's fine to delete this irq save code. As for the "TODO"
comment, I think we can add:
BUG_ON(!irqs_disabled());
or
WARN_ON_ONCE(!irqs_disabled());
and then delete the "TODO" comment.
cheers,
Sebastian
.
