[PATCH] scsi: ufs: core: Fix link_startup_again on success

[PATCH] scsi: ufs: core: Fix link_startup_again on success

[Vamshi Gajjela]

Set link_startup_again to false after a successful
ufshcd_dme_link_startup operation and confirmation of device presence.
Prevents unnecessary link startup attempts when the previous operation
has succeeded.

Signed-off-by: Vamshi Gajjela <vamshigajjela@google.com>
Fixes: 7caf489b99a4 ("scsi: ufs: issue link starup 2 times if device isn't active")
Cc: stable@vger.kernel.org
---
 drivers/ufs/core/ufshcd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index abbe7135a977..cc1d15002ab5 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4994,6 +4994,10 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
 			goto out;
 		}
 
+		/* link_startup success and device is present */
+		if (!ret && ufshcd_is_device_present(hba))
+			link_startup_again = false;
+
 		/*
 		 * DME link lost indication is only received when link is up,
 		 * but we can't be sure if the link is up until link startup
-- 
2.47.0.371.ga323438b13-goog

Re: [PATCH] scsi: ufs: core: Fix link_startup_again on success

[Bart Van Assche]

On 11/25/24 4:53 AM, Vamshi Gajjela wrote:
> Set link_startup_again to false after a successful
> ufshcd_dme_link_startup operation and confirmation of device presence.
> Prevents unnecessary link startup attempts when the previous operation
> has succeeded.
> 
> Signed-off-by: Vamshi Gajjela <vamshigajjela@google.com>
> Fixes: 7caf489b99a4 ("scsi: ufs: issue link starup 2 times if device isn't active")
> Cc: stable@vger.kernel.org

Shouldn't your Signed-off-by come after the Fixes: and Cc: stable tags? 
Anyway, since this patch looks good to me:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH] scsi: ufs: core: Fix link_startup_again on success

[Manivannan Sadhasivam]

On Mon, Nov 25, 2024 at 06:23:37PM +0530, Vamshi Gajjela wrote:
> Set link_startup_again to false after a successful
> ufshcd_dme_link_startup operation and confirmation of device presence.
> Prevents unnecessary link startup attempts when the previous operation
> has succeeded.
> 
> Signed-off-by: Vamshi Gajjela <vamshigajjela@google.com>
> Fixes: 7caf489b99a4 ("scsi: ufs: issue link starup 2 times if device isn't active")
> Cc: stable@vger.kernel.org
> ---
>  drivers/ufs/core/ufshcd.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index abbe7135a977..cc1d15002ab5 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -4994,6 +4994,10 @@ static int ufshcd_link_startup(struct ufs_hba *hba)
>  			goto out;
>  		}
>  
> +		/* link_startup success and device is present */
> +		if (!ret && ufshcd_is_device_present(hba))
> +			link_startup_again = false;
> +

Using 'link_startup_again' flag looks messy. Can't we just check the device
state after first link startup and if it is still not active, then try link
startup one more time? (Assuming that the device state won't be active after
first link startup).

Also, we should move the link startup and its associated check to a helper
function and call them instead of (ab)using the goto label.

Like,

	ret = __ufshcd_link_startup()
	if (ret)
		goto fail

	/* try link startup one more time if the device is not active */
	if (!ufshcd_is_ufs_dev_active()) {
		ret = __ufshcd_link_startup()
		if (ret)
			goto fail
	}

- Mani

>  		/*
>  		 * DME link lost indication is only received when link is up,
>  		 * but we can't be sure if the link is up until link startup
> -- 
> 2.47.0.371.ga323438b13-goog
> 

-- 
மணிவண்ணன் சதாசிவம்