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.
> > 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 <[email protected]>
> > ---
> > 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
