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 <[email protected]>
---
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;
--
2.32.0
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 <[email protected]>
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 <[email protected]>
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.
--
Damien Le Moal
Western Digital Research
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 <[email protected]>
>
> 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 7/29/23 19: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.
>
> Signed-off-by: Wenchao Hao <[email protected]>
Looks OK.
Reviewed-by: Damien Le Moal <[email protected]>
--
Damien Le Moal
Western Digital Research
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 <[email protected]>
> ---
> 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;