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.
I was just really asking how you know interrupts are disabled already?
Maybe it's obvious.
cheers,
Restoring the interrupt state later does not change anything because
they were disabled and remain disabled. Therefore remove the operations
which do not change the behaviour.
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
---
drivers/scsi/libsas/sas_ata.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 0cc1567eacc1..bc5d917ff643 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -176,7 +176,6 @@ static void sas_ata_task_done(struct sas_task *task)
static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
{
- unsigned long flags;
struct sas_task *task;
struct scatterlist *sg;
int ret = AC_ERR_SYSTEM;
@@ -190,7 +189,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
/* TODO: audit callers to ensure they are ready for qc_issue to
* unconditionally re-enable interrupts
*/
Is the "TODO" comment still relevant?
can't tell. The interrupts are never enabled during the unlock.
cheers,
- local_irq_save(flags);
spin_unlock(ap->lock);
/* If the device fell off, no sense in issuing commands */
@@ -252,7 +250,6 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
out:
spin_lock(ap->lock);
- local_irq_restore(flags);
return ret;
}
Sebastian
.
