| Message ID | 20210701211224.17070-3-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | UFS patches for kernel v5.15 | expand |
Hi Bart, 在 2021/7/2 5:12, Bart Van Assche 写道: > According to the information I found in patch commit descriptions, for SATA > devices commands must be aborted from inside the libsas error handler. > Check host->ehandler to determine whether or not running inside the error > handler since host->host_eh_scheduled != 0 indicates that the SCSI error > handler has been scheduled but does not mean that is already running. This > patch restores code that was removed by commit 909657615d9b ("scsi: libsas: > allow async aborts"). > > Cc: Hannes Reinecke <hare@suse.de> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Ming Lei <ming.lei@redhat.com> > Cc: John Garry <john.garry@huawei.com> > Cc: Yves-Alexis Perez <corsac@debian.org> > Fixes: c9f926000fe3 ("scsi: libsas: Disable asynchronous aborts for SATA devices") > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/libsas/sas_scsi_host.c | 5 ++++- > include/scsi/libsas.h | 1 + > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c > index ee44a0d7730b..95e4d58ab9d4 100644 > --- a/drivers/scsi/libsas/sas_scsi_host.c > +++ b/drivers/scsi/libsas/sas_scsi_host.c > @@ -462,6 +462,7 @@ int sas_eh_abort_handler(struct scsi_cmnd *cmd) > int res = TMF_RESP_FUNC_FAILED; > struct sas_task *task = TO_SAS_TASK(cmd); > struct Scsi_Host *host = cmd->device->host; > + struct sas_ha_struct *ha = SHOST_TO_SAS_HA(host); > struct domain_device *dev = cmd_to_domain_dev(cmd); > struct sas_internal *i = to_sas_internal(host->transportt); > unsigned long flags; > @@ -471,7 +472,7 @@ int sas_eh_abort_handler(struct scsi_cmnd *cmd) > > spin_lock_irqsave(host->host_lock, flags); > /* We cannot do async aborts for SATA devices */ > - if (dev_is_sata(dev) && !host->host_eh_scheduled) { > + if (dev_is_sata(dev) && !ha->eh_running) { > spin_unlock_irqrestore(host->host_lock, flags); > return FAILED; > } No idea why sas_eh_abort_handler() is only used by isci. The other libsas drivers just let the error handler do the aborting work. So my question is can the isci driver delete this callback and let the error handler do this? If so, then we cann remove this function directly. Thanks, Jason > @@ -731,6 +732,7 @@ void sas_scsi_recover_host(struct Scsi_Host *shost) > tries++; > retry = true; > spin_lock_irq(shost->host_lock); > + ha->eh_running = true; > list_splice_init(&shost->eh_cmd_q, &eh_work_q); > spin_unlock_irq(shost->host_lock); > > @@ -767,6 +769,7 @@ void sas_scsi_recover_host(struct Scsi_Host *shost) > > /* check if any new eh work was scheduled during the last run */ > spin_lock_irq(&ha->lock); > + ha->eh_running = false; > if (ha->eh_active == 0) { > shost->host_eh_scheduled = 0; > retry = false; > diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h > index 6fe125a71b60..4a8fb324140e 100644 > --- a/include/scsi/libsas.h > +++ b/include/scsi/libsas.h > @@ -364,6 +364,7 @@ struct sas_ha_struct { > struct mutex drain_mutex; > unsigned long state; > spinlock_t lock; > + bool eh_running; > int eh_active; > wait_queue_head_t eh_wait_q; > struct list_head eh_dev_q; > . >
On 7/2/21 7:32 PM, Jason Yan wrote: > No idea why sas_eh_abort_handler() is only used by isci. The other > libsas drivers just let the error handler do the aborting work. So my > question is can the isci driver delete this callback and let the error > handler do this? If so, then we cann remove this function directly. Hmm ... I think that's a question for an isci expert. Unfortunately I'm not familiar with the isci driver myself. Thanks, Bart.
On 7/3/21 4:32 AM, Jason Yan wrote: > Hi Bart, > > 在 2021/7/2 5:12, Bart Van Assche 写道: >> According to the information I found in patch commit descriptions, for >> SATA >> devices commands must be aborted from inside the libsas error handler. >> Check host->ehandler to determine whether or not running inside the error >> handler since host->host_eh_scheduled != 0 indicates that the SCSI error >> handler has been scheduled but does not mean that is already running. >> This >> patch restores code that was removed by commit 909657615d9b ("scsi: >> libsas: >> allow async aborts"). >> >> Cc: Hannes Reinecke <hare@suse.de> >> Cc: Christoph Hellwig <hch@lst.de> >> Cc: Ming Lei <ming.lei@redhat.com> >> Cc: John Garry <john.garry@huawei.com> >> Cc: Yves-Alexis Perez <corsac@debian.org> >> Fixes: c9f926000fe3 ("scsi: libsas: Disable asynchronous aborts for >> SATA devices") >> Signed-off-by: Bart Van Assche <bvanassche@acm.org> >> --- >> drivers/scsi/libsas/sas_scsi_host.c | 5 ++++- >> include/scsi/libsas.h | 1 + >> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/libsas/sas_scsi_host.c >> b/drivers/scsi/libsas/sas_scsi_host.c >> index ee44a0d7730b..95e4d58ab9d4 100644 >> --- a/drivers/scsi/libsas/sas_scsi_host.c >> +++ b/drivers/scsi/libsas/sas_scsi_host.c >> @@ -462,6 +462,7 @@ int sas_eh_abort_handler(struct scsi_cmnd *cmd) >> int res = TMF_RESP_FUNC_FAILED; >> struct sas_task *task = TO_SAS_TASK(cmd); >> struct Scsi_Host *host = cmd->device->host; >> + struct sas_ha_struct *ha = SHOST_TO_SAS_HA(host); >> struct domain_device *dev = cmd_to_domain_dev(cmd); >> struct sas_internal *i = to_sas_internal(host->transportt); >> unsigned long flags; >> @@ -471,7 +472,7 @@ int sas_eh_abort_handler(struct scsi_cmnd *cmd) >> spin_lock_irqsave(host->host_lock, flags); >> /* We cannot do async aborts for SATA devices */ >> - if (dev_is_sata(dev) && !host->host_eh_scheduled) { >> + if (dev_is_sata(dev) && !ha->eh_running) { >> spin_unlock_irqrestore(host->host_lock, flags); >> return FAILED; >> } > > > No idea why sas_eh_abort_handler() is only used by isci. The other > libsas drivers just let the error handler do the aborting work. So my > question is can the isci driver delete this callback and let the error > handler do this? If so, then we cann remove this function directly. > That, indeed, is a good point. SATA commands are being handled by the sas_ata_strategy_handler (as they don't really match the SAM logic wrt TMF). And actually there is no 'abort' command on SATA; the best you can do is 'abort queue', but that's more like an 'ABORT TASK SET' in SAM. So either way that callback is pointless. Cheers, Hannes -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index ee44a0d7730b..95e4d58ab9d4 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -462,6 +462,7 @@ int sas_eh_abort_handler(struct scsi_cmnd *cmd) int res = TMF_RESP_FUNC_FAILED; struct sas_task *task = TO_SAS_TASK(cmd); struct Scsi_Host *host = cmd->device->host; + struct sas_ha_struct *ha = SHOST_TO_SAS_HA(host); struct domain_device *dev = cmd_to_domain_dev(cmd); struct sas_internal *i = to_sas_internal(host->transportt); unsigned long flags; @@ -471,7 +472,7 @@ int sas_eh_abort_handler(struct scsi_cmnd *cmd) spin_lock_irqsave(host->host_lock, flags); /* We cannot do async aborts for SATA devices */ - if (dev_is_sata(dev) && !host->host_eh_scheduled) { + if (dev_is_sata(dev) && !ha->eh_running) { spin_unlock_irqrestore(host->host_lock, flags); return FAILED; } @@ -731,6 +732,7 @@ void sas_scsi_recover_host(struct Scsi_Host *shost) tries++; retry = true; spin_lock_irq(shost->host_lock); + ha->eh_running = true; list_splice_init(&shost->eh_cmd_q, &eh_work_q); spin_unlock_irq(shost->host_lock); @@ -767,6 +769,7 @@ void sas_scsi_recover_host(struct Scsi_Host *shost) /* check if any new eh work was scheduled during the last run */ spin_lock_irq(&ha->lock); + ha->eh_running = false; if (ha->eh_active == 0) { shost->host_eh_scheduled = 0; retry = false; diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index 6fe125a71b60..4a8fb324140e 100644 --- a/include/scsi/libsas.h +++ b/include/scsi/libsas.h @@ -364,6 +364,7 @@ struct sas_ha_struct { struct mutex drain_mutex; unsigned long state; spinlock_t lock; + bool eh_running; int eh_active; wait_queue_head_t eh_wait_q; struct list_head eh_dev_q;
According to the information I found in patch commit descriptions, for SATA devices commands must be aborted from inside the libsas error handler. Check host->ehandler to determine whether or not running inside the error handler since host->host_eh_scheduled != 0 indicates that the SCSI error handler has been scheduled but does not mean that is already running. This patch restores code that was removed by commit 909657615d9b ("scsi: libsas: allow async aborts"). Cc: Hannes Reinecke <hare@suse.de> Cc: Christoph Hellwig <hch@lst.de> Cc: Ming Lei <ming.lei@redhat.com> Cc: John Garry <john.garry@huawei.com> Cc: Yves-Alexis Perez <corsac@debian.org> Fixes: c9f926000fe3 ("scsi: libsas: Disable asynchronous aborts for SATA devices") Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/libsas/sas_scsi_host.c | 5 ++++- include/scsi/libsas.h | 1 + 2 files changed, 5 insertions(+), 1 deletion(-)