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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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
> 

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