On 23/05/2018 10: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.
cheers,
Sorry, but personally I don't see much value in this change. I think
it's better for safety to be consistent in how we lock & unlock the
spinlock, i.e. use irqsave variant (or similar).
And even if we have audited the callers to know the irqstate when this
function is called, since sas_ata_qc_issue() is a callback for
ata_port_operations.qc_issue, then it should be documented that
interrupts should always be disabled for this callback.
All the best,
John
Sebastian
.
