mbox series

[v4,00/10] Simplify the UFS driver initialization code

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

Message

Bart Van Assche Sept. 5, 2024, 10:01 p.m. UTC 
Hi Martin,

This patch series addresses the following issues in the UFS driver
initialization code:
* The legacy and MCQ scsi_add_host() calls occur in different functions. This
  patch series reduces the number of scsi_add_host() calls from two to one
  and hence makes the UFS driver easier to maintain.
* Two functions have a boolean 'init_dev_params' argument. This patch series
  removes that argument from both functions by splitting functions and by
  pushing some function calls from caller into callee.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Changes compared to v3:
 - Split patch "Move the MCQ scsi_add_host() call" into two patches to make
   it easier for reviewers.

Changes compared to v2:
 - Improved several patch descriptions.
 - Moved one source code comment.

Changes compared to v1:
 - Fixed a compiler warning reported by the kernel build robot.
 - Improved patch descriptions.

Bart Van Assche (10):
  scsi: ufs: core: Introduce ufshcd_add_scsi_host()
  scsi: ufs: core: Introduce ufshcd_activate_link()
  scsi: ufs: core: Introduce ufshcd_post_device_init()
  scsi: ufs: core: Call ufshcd_add_scsi_host() later
  scsi: ufs: core: Move the ufshcd_device_init() call
  scsi: ufs: core: Move the ufshcd_device_init(hba, true) call
  scsi: ufs: core: Expand the ufshcd_device_init(hba, true) call
  scsi: ufs: core: Move the MCQ scsi_add_host() call
  scsi: ufs: core: Move code out of an if-statement
  scsi: ufs: core: Remove the second argument of ufshcd_device_init()

 drivers/ufs/core/ufshcd.c | 268 ++++++++++++++++++++++----------------
 1 file changed, 153 insertions(+), 115 deletions(-)

Comments

Bao D. Nguyen Sept. 6, 2024, 11:51 p.m. UTC | #1 
On 9/5/2024 3:01 PM, Bart Van Assche wrote:
> Move the code for adding a SCSI host and also the code for managing
> TMF tags from ufshcd_init() into a new function called
> ufshcd_add_scsi_host(). This patch prepares for combining the two
> scsi_add_host() calls into a single call. No functionality has been
> changed.
> 

Reviewed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>

Thanks, Bao
Bao D. Nguyen Sept. 7, 2024, 12:24 a.m. UTC | #2 
On 9/5/2024 3:01 PM, Bart Van Assche wrote:

> +	ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
> +	if (ret) {
> +		dev_err(hba->dev,

Nit picking. A line break here is not necessary. The original code fits 
the message fine.
> +			"%s: Failed setting power mode, err = %d\n",
> +			__func__, ret);
> +		return ret;
> +	}

Reviewed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
Bao D. Nguyen Sept. 7, 2024, 11:12 a.m. UTC | #3 
On 9/5/2024 3:01 PM, Bart Van Assche wrote:
> Move the ufshcd_device_init() call to the ufshcd_probe_hba() callers and
> remove the 'init_dev_params' argument of ufshcd_probe_hba(). This change
> refactors the code without modifying the behavior of the UFSHCI driver.
> This change prepares for moving one ufshcd_device_init() call into
> ufshcd_init().
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ufs/core/ufshcd.c | 25 +++++++++++++------------
>   1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index e2137bcf3de7..6e3cffcdf9a6 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -298,7 +298,8 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba);
>   static int ufshcd_eh_host_reset_handler(struct scsi_cmnd *cmd);
>   static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag);
>   static void ufshcd_hba_exit(struct ufs_hba *hba);
> -static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params);
> +static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params);
> +static int ufshcd_probe_hba(struct ufs_hba *hba);
>   static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on);
>   static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba);
>   static int ufshcd_host_reset_and_restore(struct ufs_hba *hba);
> @@ -7693,8 +7694,11 @@ static int ufshcd_host_reset_and_restore(struct ufs_hba *hba)
>   	err = ufshcd_hba_enable(hba);
>   
>   	/* Establish the link again and restore the device */
> -	if (!err)
> -		err = ufshcd_probe_hba(hba, false);

In the original code, if ufshcd_probe_hba()->ufshcd_device_init() fails, 
the hba->ufshcd_state is updated to UFSHCD_STATE_ERROR;

> +	if (!err) {
> +		err = ufshcd_device_init(hba, /*init_dev_params=*/false);

Calling ufshcd_device_init() here changes the behavior of the code 
slightly. If the ufshcd_device_init() fails, it exits without updating 
the hba->ufshcd_state to UFSHCD_STATE_ERROR.


> +		if (!err)
> +			err = ufshcd_probe_hba(hba);
> +	}
>   
>   	if (err)
>   		dev_err(hba->dev, "%s: Host init failed %d\n", __func__, err);
> @@ -8830,21 +8834,16 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
>   /**
>    * ufshcd_probe_hba - probe hba to detect device and initialize it
>    * @hba: per-adapter instance
> - * @init_dev_params: whether or not to call ufshcd_device_params_init().
>    *
>    * Execute link-startup and verify device initialization
>    *
>    * Return: 0 upon success; < 0 upon failure.
>    */
> -static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
> +static int ufshcd_probe_hba(struct ufs_hba *hba)
>   {
>   	ktime_t start = ktime_get();
>   	unsigned long flags;
> -	int ret;
> -
> -	ret = ufshcd_device_init(hba, init_dev_params);
> -	if (ret)
> -		goto out;
> +	int ret = 0;
>   
>   	if (!hba->pm_op_in_progress &&
>   	    (hba->quirks & UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH)) {
> @@ -8862,7 +8861,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
>   		}
>   
>   		/* Reinit the device */
> -		ret = ufshcd_device_init(hba, init_dev_params);
> +		ret = ufshcd_device_init(hba, /*init_dev_params=*/false);

Originally, the ufshcd_async_scan()->ufshcd_probe_hba() has the 
init_dev_params = true. However the init_dev_params is updated to false 
here for the ufshcd_async_scan()->ufshcd_probe_hba() path.

>   		if (ret)
>   			goto out;
>   	}
> @@ -8910,7 +8909,9 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
>   
>   	down(&hba->host_sem);
>   	/* Initialize hba, detect and initialize UFS device */
> -	ret = ufshcd_probe_hba(hba, true);
> +	ret = ufshcd_device_init(hba, /*init_dev_params=*/true);

Same issue as mentioned earlier. If ufshcd_device_init() fails here, the 
hba->ufshcd_state is not updated to UFSHCD_STATE_ERROR. Compared to the 
original ufshcd_probe_hba()->ufshcd_device_init() failure would update 
hba->ufshcd_state.

Thanks, Bao

> +	if (ret == 0)
> +		ret = ufshcd_probe_hba(hba);
>   	up(&hba->host_sem);
>   	if (ret)
>   		goto out;
>
Bao D. Nguyen Sept. 7, 2024, 8:15 p.m. UTC | #4 
On 9/5/2024 3:01 PM, Bart Van Assche wrote:
> Expand the ufshcd_device_init(hba, true) call and remove all code that
> depends on init_dev_params == false. This change prepares for combining
> the two scsi_add_host() calls.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ufs/core/ufshcd.c | 43 ++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 843566720afa..014bc74390af 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10602,7 +10602,48 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
>   	 */
>   	ufshcd_set_ufs_dev_active(hba);
>   
> -	err = ufshcd_device_init(hba, /*init_dev_params=*/true);
> +	err = ufshcd_activate_link(hba);
> +	if (err)
> +		goto out_disable;
> +
> +	/* Verify device initialization by sending NOP OUT UPIU. */
> +	err = ufshcd_verify_dev_init(hba);
> +	if (err)
> +		goto out_disable;
> +
> +	/* Initiate UFS initialization and waiting for completion. */
> +	err = ufshcd_complete_dev_init(hba);
> +	if (err)
> +		goto out_disable;
> +
> +	/*
> +	 * Initialize UFS device parameters used by driver, these
> +	 * parameters are associated with UFS descriptors.
> +	 */
> +	err = ufshcd_device_params_init(hba);
> +	if (err)
> +		goto out_disable;
> +	if (is_mcq_supported(hba)) {
> +		ufshcd_mcq_enable(hba);
> +		err = ufshcd_alloc_mcq(hba);
> +		if (!err) {
> +			ufshcd_config_mcq(hba);
> +		} else {
> +			/* Continue with SDB mode */
> +			ufshcd_mcq_disable(hba);
> +			use_mcq_mode = false;

If ufshcd_alloc_mcq() fails here, sdb mode is used. The scsi_add_host() 
is also called for sdb mode. Later on, when the ufshcd_add_scsi_host() 
function is called, we will call scsi_add_host() again for sdb mode. 
Therefore, scsi_add_host() would be called twice.

> +			dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
> +				err);
> +		}
> +		err = scsi_add_host(host, hba->dev);
> +		if (err) {
> +			dev_err(hba->dev, "scsi_add_host failed\n");
> +			goto out_disable;
> +		}
> +		hba->scsi_host_added = true;
> +	}
> +
> +	err = ufshcd_post_device_init(hba);
>   	if (err)
>   		goto out_disable;
>   
>
Bao D. Nguyen Sept. 7, 2024, 8:36 p.m. UTC | #5 
On 9/5/2024 3:01 PM, Bart Van Assche wrote:
> The previous patch in this series introduced identical code in both
> branches of an if-statement. Move that code outside the if-statement.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Reviewed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>