[PATCH] scsi:libsas: Simplify sas_queue_reset and remove unused code

[PATCH] scsi:libsas: Simplify sas_queue_reset and remove unused code

[Wenchao Hao]

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(-)

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

Re: [PATCH] scsi:libsas: Simplify sas_queue_reset and remove unused code

[Jason Yan]

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>

Re: [PATCH] scsi:libsas: Simplify sas_queue_reset and remove unused code

[Damien Le Moal]

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.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] scsi:libsas: Simplify sas_queue_reset and remove unused code

[Jason Yan]

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

Re: [PATCH] scsi:libsas: Simplify sas_queue_reset and remove unused code

[Damien Le Moal]

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 <haowenchao2@huawei.com>

Looks OK.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] scsi:libsas: Simplify sas_queue_reset and remove unused code

[haowenchao (C)]

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;

Re: [PATCH] scsi:libsas: Simplify sas_queue_reset and remove unused code

[Martin K. Petersen]

>> 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!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi:libsas: Simplify sas_queue_reset and remove unused code

[Martin K. Petersen]

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

-- 
Martin K. Petersen	Oracle Linux Engineering