diff mbox series

[v4,02/10] scsi: ufs: core: Introduce ufshcd_activate_link()

Message ID 20240905220214.738506-3-bvanassche@acm.org
State New
Headers show
Series Simplify the UFS driver initialization code | expand

Commit Message

Bart Van Assche Sept. 5, 2024, 10:01 p.m. UTC 
Prepare for introducing a second caller by moving the code for
activating the link between UFS controller and device into a new
function. No functionality has been changed.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Bao D. Nguyen Sept. 6, 2024, 11:59 p.m. UTC | #1 
On 9/5/2024 3:01 PM, Bart Van Assche wrote:
> Prepare for introducing a second caller by moving the code for
> activating the link between UFS controller and device into a new
> function. No functionality has been changed.
> 
> Reviewed-by: Avri Altman <avri.altman@wdc.com>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Reviewed-by: Bean Huo <beanhuo@micron.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ufs/core/ufshcd.c | 15 +++++++++++++--
>   1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ecf6da2efed1..4cfa8dd5500a 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8709,10 +8709,9 @@ static void ufshcd_config_mcq(struct ufs_hba *hba)
>   		 hba->nutrs);
>   }
>   
> -static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
> +static int ufshcd_activate_link(struct ufs_hba *hba)
>   {
>   	int ret;
> -	struct Scsi_Host *host = hba->host;
>   
>   	hba->ufshcd_state = UFSHCD_STATE_RESET;
>   
> @@ -8729,6 +8728,18 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
>   	/* UniPro link is active now */
>   	ufshcd_set_link_active(hba);
>   
> +	return 0;
> +}
> +
> +static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
> +{
> +	struct Scsi_Host *host = hba->host;
> +	int ret;
> +
> +	ret = ufshcd_activate_link(hba);
> +	if (ret)
> +		return ret;

Hi Bart,
There may be a problem in this patch.
In the original code, if UFSHCD_QUIRK_SKIP_PH_CONFIGURATION is set, 
ufshcd_device_init() would exit early. However, in this patch, if 
UFSHCD_QUIRK_SKIP_PH_CONFIGURATION is set, the new function 
ufshcd_activate_link() would return 0 which would cause the 
ufshcd_device_init() to continue further down. As a result, the 
ufshcd_device_init() would fail to handle the 
UFSHCD_QUIRK_SKIP_PH_CONFIGURATION flag as its original intention, right?

Thanks, Bao

> +
>   	/* Reconfigure MCQ upon reset */
>   	if (hba->mcq_enabled && !init_dev_params) {
>   		ufshcd_config_mcq(hba);
>
Bart Van Assche Sept. 9, 2024, 5:08 p.m. UTC | #2 
On 9/6/24 4:59 PM, Bao D. Nguyen wrote:
> In the original code, if UFSHCD_QUIRK_SKIP_PH_CONFIGURATION is set, 
> ufshcd_device_init() would exit early. However, in this patch, if 
> UFSHCD_QUIRK_SKIP_PH_CONFIGURATION is set, the new function 
> ufshcd_activate_link() would return 0 which would cause the 
> ufshcd_device_init() to continue further down. As a result, the 
> ufshcd_device_init() would fail to handle the 
> UFSHCD_QUIRK_SKIP_PH_CONFIGURATION flag as its original intention, right?

Hi Bao,

I was assuming that UFSHCD_STATE_RESET != 0 but apparently it is zero. I
will fix this patch before I repost this patch series.

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index ecf6da2efed1..4cfa8dd5500a 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8709,10 +8709,9 @@  static void ufshcd_config_mcq(struct ufs_hba *hba)
 		 hba->nutrs);
 }
 
-static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
+static int ufshcd_activate_link(struct ufs_hba *hba)
 {
 	int ret;
-	struct Scsi_Host *host = hba->host;
 
 	hba->ufshcd_state = UFSHCD_STATE_RESET;
 
@@ -8729,6 +8728,18 @@  static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 	/* UniPro link is active now */
 	ufshcd_set_link_active(hba);
 
+	return 0;
+}
+
+static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
+{
+	struct Scsi_Host *host = hba->host;
+	int ret;
+
+	ret = ufshcd_activate_link(hba);
+	if (ret)
+		return ret;
+
 	/* Reconfigure MCQ upon reset */
 	if (hba->mcq_enabled && !init_dev_params) {
 		ufshcd_config_mcq(hba);