| Message ID | 20230729102451.2452826-1-haowenchao2@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi:libsas: Simplify sas_queue_reset and remove unused code | expand |
Hi Wenchao, On 2023/7/29 18:24, Wenchao Hao wrote: > sas_queue_reset is always called with param "wait" set to 0, so > remove it from this function's param list. And remove unused > function sas_wait_eh. I did not find any users passing '1' to sas_queue_reset() in the git history. It seems it is never used. So It's ok to remove it, I guess. Just one nit, there should be a blank between "scsi:" and "libsas" in the subject: scsi: libsas: Simplify sas_queue_reset and remove unused code Otherwise looks good to me: Reviewed-by: Jason Yan <yanaijie@huawei.com>
On 7/31/23 10:44, Jason Yan wrote: > Hi Wenchao, > > On 2023/7/29 18:24, Wenchao Hao wrote: >> sas_queue_reset is always called with param "wait" set to 0, so >> remove it from this function's param list. And remove unused >> function sas_wait_eh. > > I did not find any users passing '1' to sas_queue_reset() in the git > history. It seems it is never used. So It's ok to remove it, I guess. > > Just one nit, there should be a blank between "scsi:" and "libsas" in > the subject: > > scsi: libsas: Simplify sas_queue_reset and remove unused code > > Otherwise looks good to me: > Reviewed-by: Jason Yan <yanaijie@huawei.com> Yeah, code wise, it looks good. However, I am seeing issues with completions for commands that use command duration limits. There are some unusual completions that needs special handling with CDL (e.g. some aborted commands can be notified with a success and sense-data-available-bit set. For these, we kick libata-EH but it seems that this is not well working with libsas. So I wonder if this code may need to be used for CDL... So let's please hold on before applying this. Let me check the CDL issues I am seeing first.
On 2023/7/31 10:05, Damien Le Moal wrote: > On 7/31/23 10:44, Jason Yan wrote: >> Hi Wenchao, >> >> On 2023/7/29 18:24, Wenchao Hao wrote: >>> sas_queue_reset is always called with param "wait" set to 0, so >>> remove it from this function's param list. And remove unused >>> function sas_wait_eh. >> >> I did not find any users passing '1' to sas_queue_reset() in the git >> history. It seems it is never used. So It's ok to remove it, I guess. >> >> Just one nit, there should be a blank between "scsi:" and "libsas" in >> the subject: >> >> scsi: libsas: Simplify sas_queue_reset and remove unused code >> >> Otherwise looks good to me: >> Reviewed-by: Jason Yan <yanaijie@huawei.com> > > Yeah, code wise, it looks good. > However, I am seeing issues with completions for commands that use command > duration limits. There are some unusual completions that needs special handling > with CDL (e.g. some aborted commands can be notified with a success and > sense-data-available-bit set. For these, we kick libata-EH but it seems that > this is not well working with libsas. So I wonder if this code may need to be > used for CDL... So let's please hold on before applying this. Let me check the > CDL issues I am seeing first. > Sure. It's just a cleanup. Let's hold on until it is confirmed. Thanks, Jason
On 2023/7/29 18:24, Wenchao Hao wrote: > sas_queue_reset is always called with param "wait" set to 0, so > remove it from this function's param list. And remove unused > function sas_wait_eh. > Ping... > Signed-off-by: Wenchao Hao <haowenchao2@huawei.com> > --- > drivers/scsi/libsas/sas_scsi_host.c | 41 +++-------------------------- > 1 file changed, 3 insertions(+), 38 deletions(-) > > diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c > index 94c5f14f3c16..3f01e77eaee3 100644 > --- a/drivers/scsi/libsas/sas_scsi_host.c > +++ b/drivers/scsi/libsas/sas_scsi_host.c > @@ -387,37 +387,7 @@ struct sas_phy *sas_get_local_phy(struct domain_device *dev) > } > EXPORT_SYMBOL_GPL(sas_get_local_phy); > > -static void sas_wait_eh(struct domain_device *dev) > -{ > - struct sas_ha_struct *ha = dev->port->ha; > - DEFINE_WAIT(wait); > - > - if (dev_is_sata(dev)) { > - ata_port_wait_eh(dev->sata_dev.ap); > - return; > - } > - retry: > - spin_lock_irq(&ha->lock); > - > - while (test_bit(SAS_DEV_EH_PENDING, &dev->state)) { > - prepare_to_wait(&ha->eh_wait_q, &wait, TASK_UNINTERRUPTIBLE); > - spin_unlock_irq(&ha->lock); > - schedule(); > - spin_lock_irq(&ha->lock); > - } > - finish_wait(&ha->eh_wait_q, &wait); > - > - spin_unlock_irq(&ha->lock); > - > - /* make sure SCSI EH is complete */ > - if (scsi_host_in_recovery(ha->core.shost)) { > - msleep(10); > - goto retry; > - } > -} > - > -static int sas_queue_reset(struct domain_device *dev, int reset_type, > - u64 lun, int wait) > +static int sas_queue_reset(struct domain_device *dev, int reset_type, u64 lun) > { > struct sas_ha_struct *ha = dev->port->ha; > int scheduled = 0, tries = 100; > @@ -425,8 +395,6 @@ static int sas_queue_reset(struct domain_device *dev, int reset_type, > /* ata: promote lun reset to bus reset */ > if (dev_is_sata(dev)) { > sas_ata_schedule_reset(dev); > - if (wait) > - sas_ata_wait_eh(dev); > return SUCCESS; > } > > @@ -444,9 +412,6 @@ static int sas_queue_reset(struct domain_device *dev, int reset_type, > } > spin_unlock_irq(&ha->lock); > > - if (wait) > - sas_wait_eh(dev); > - > if (scheduled) > return SUCCESS; > } > @@ -499,7 +464,7 @@ int sas_eh_device_reset_handler(struct scsi_cmnd *cmd) > struct sas_internal *i = to_sas_internal(host->transportt); > > if (current != host->ehandler) > - return sas_queue_reset(dev, SAS_DEV_LU_RESET, cmd->device->lun, 0); > + return sas_queue_reset(dev, SAS_DEV_LU_RESET, cmd->device->lun); > > int_to_scsilun(cmd->device->lun, &lun); > > @@ -522,7 +487,7 @@ int sas_eh_target_reset_handler(struct scsi_cmnd *cmd) > struct sas_internal *i = to_sas_internal(host->transportt); > > if (current != host->ehandler) > - return sas_queue_reset(dev, SAS_DEV_RESET, 0, 0); > + return sas_queue_reset(dev, SAS_DEV_RESET, 0); > > if (!i->dft->lldd_I_T_nexus_reset) > return FAILED;
>> sas_queue_reset is always called with param "wait" set to 0, so >> remove it from this function's param list. And remove unused function >> sas_wait_eh. > > Ping... Applied to 6.6/scsi-staging, thanks!
On Sat, 29 Jul 2023 18:24:51 +0800, Wenchao Hao wrote: > sas_queue_reset is always called with param "wait" set to 0, so > remove it from this function's param list. And remove unused > function sas_wait_eh. > > Applied to 6.6/scsi-queue, thanks! [1/1] scsi:libsas: Simplify sas_queue_reset and remove unused code https://git.kernel.org/mkp/scsi/c/be946e31bcf2
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index 94c5f14f3c16..3f01e77eaee3 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -387,37 +387,7 @@ struct sas_phy *sas_get_local_phy(struct domain_device *dev) } EXPORT_SYMBOL_GPL(sas_get_local_phy); -static void sas_wait_eh(struct domain_device *dev) -{ - struct sas_ha_struct *ha = dev->port->ha; - DEFINE_WAIT(wait); - - if (dev_is_sata(dev)) { - ata_port_wait_eh(dev->sata_dev.ap); - return; - } - retry: - spin_lock_irq(&ha->lock); - - while (test_bit(SAS_DEV_EH_PENDING, &dev->state)) { - prepare_to_wait(&ha->eh_wait_q, &wait, TASK_UNINTERRUPTIBLE); - spin_unlock_irq(&ha->lock); - schedule(); - spin_lock_irq(&ha->lock); - } - finish_wait(&ha->eh_wait_q, &wait); - - spin_unlock_irq(&ha->lock); - - /* make sure SCSI EH is complete */ - if (scsi_host_in_recovery(ha->core.shost)) { - msleep(10); - goto retry; - } -} - -static int sas_queue_reset(struct domain_device *dev, int reset_type, - u64 lun, int wait) +static int sas_queue_reset(struct domain_device *dev, int reset_type, u64 lun) { struct sas_ha_struct *ha = dev->port->ha; int scheduled = 0, tries = 100; @@ -425,8 +395,6 @@ static int sas_queue_reset(struct domain_device *dev, int reset_type, /* ata: promote lun reset to bus reset */ if (dev_is_sata(dev)) { sas_ata_schedule_reset(dev); - if (wait) - sas_ata_wait_eh(dev); return SUCCESS; } @@ -444,9 +412,6 @@ static int sas_queue_reset(struct domain_device *dev, int reset_type, } spin_unlock_irq(&ha->lock); - if (wait) - sas_wait_eh(dev); - if (scheduled) return SUCCESS; } @@ -499,7 +464,7 @@ int sas_eh_device_reset_handler(struct scsi_cmnd *cmd) struct sas_internal *i = to_sas_internal(host->transportt); if (current != host->ehandler) - return sas_queue_reset(dev, SAS_DEV_LU_RESET, cmd->device->lun, 0); + return sas_queue_reset(dev, SAS_DEV_LU_RESET, cmd->device->lun); int_to_scsilun(cmd->device->lun, &lun); @@ -522,7 +487,7 @@ int sas_eh_target_reset_handler(struct scsi_cmnd *cmd) struct sas_internal *i = to_sas_internal(host->transportt); if (current != host->ehandler) - return sas_queue_reset(dev, SAS_DEV_RESET, 0, 0); + return sas_queue_reset(dev, SAS_DEV_RESET, 0); if (!i->dft->lldd_I_T_nexus_reset) return FAILED;
sas_queue_reset is always called with param "wait" set to 0, so remove it from this function's param list. And remove unused function sas_wait_eh. Signed-off-by: Wenchao Hao <haowenchao2@huawei.com> --- drivers/scsi/libsas/sas_scsi_host.c | 41 +++-------------------------- 1 file changed, 3 insertions(+), 38 deletions(-)