[v3,9/9] ufs: core: Remove the second argument of ufshcd_device_init()

Both ufshcd_device_init() callers pass 'false' as second argument. Hence,
remove that second argument.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 44 +++++----------------------------------
 1 file changed, 5 insertions(+), 39 deletions(-)

It's inconvenient to review them one by one, so until the last one. I
understand the main purpose is to remove init_dev_params. Why not merge
all the preparation patches into the last one?


On Wed, 2024-08-28 at 10:44 -0700, Bart Van Assche wrote:
> Both ufshcd_device_init() callers pass 'false' as second argument.
> Hence,
> remove that second argument.
> 
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/ufs/core/ufshcd.c | 44 +++++--------------------------------
> --
>  1 file changed, 5 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 16638974b34f..5239caf3fc65 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -298,7 +298,7 @@ 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_device_init(struct ufs_hba *hba, bool
> init_dev_params);
> +static int ufshcd_device_init(struct ufs_hba *hba);
>  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);
> @@ -7713,7 +7713,7 @@ static int ufshcd_host_reset_and_restore(struct
> ufs_hba *hba)
>  
>  	/* Establish the link again and restore the device */
>  	if (!err) {
> -		err = ufshcd_device_init(hba,
> /*init_dev_params=*/false);
> +		err = ufshcd_device_init(hba);
>  		if (!err)
>  			err = ufshcd_probe_hba(hba);
>  	}
> @@ -8788,9 +8788,8 @@ static int ufshcd_post_device_init(struct
> ufs_hba *hba)
>  	return 0;
>  }
>  
> -static int ufshcd_device_init(struct ufs_hba *hba, bool
> init_dev_params)
> +static int ufshcd_device_init(struct ufs_hba *hba)
>  {
> -	struct Scsi_Host *host = hba->host;
>  	int ret;
>  
>  	ret = ufshcd_activate_link(hba);
> @@ -8798,7 +8797,7 @@ static int ufshcd_device_init(struct ufs_hba
> *hba, bool init_dev_params)
>  		return ret;
>  
>  	/* Reconfigure MCQ upon reset */
> -	if (hba->mcq_enabled && !init_dev_params) {
> +	if (hba->mcq_enabled) {
>  		ufshcd_config_mcq(hba);
>  		ufshcd_mcq_enable(hba);
>  	}
> @@ -8813,39 +8812,6 @@ static int ufshcd_device_init(struct ufs_hba
> *hba, bool init_dev_params)
>  	if (ret)
>  		return ret;
>  
> -	/*
> -	 * Initialize UFS device parameters used by driver, these
> -	 * parameters are associated with UFS descriptors.
> -	 */
> -	if (init_dev_params) {
> -		ret = ufshcd_device_params_init(hba);
> -		if (ret)
> -			return ret;
> -		if (is_mcq_supported(hba) && !hba->scsi_host_added)
> {
> -			ufshcd_mcq_enable(hba);
> -			ret = ufshcd_alloc_mcq(hba);
> -			if (!ret) {
> -				ufshcd_config_mcq(hba);
> -			} else {
> -				/* Continue with SDB mode */
> -				ufshcd_mcq_disable(hba);
> -				use_mcq_mode = false;
> -				dev_err(hba->dev, "MCQ mode is
> disabled, err=%d\n",
> -					 ret);
> -			}
> -			ret = scsi_add_host(host, hba->dev);
> -			if (ret) {
> -				dev_err(hba->dev, "scsi_add_host
> failed\n");
> -				return ret;
> -			}
> -			hba->scsi_host_added = true;
> -		} else if (is_mcq_supported(hba)) {
> -			/* UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH
> is set */
> -			ufshcd_config_mcq(hba);
> -			ufshcd_mcq_enable(hba);
> -		}
> -	}
> -
>  	return ufshcd_post_device_init(hba);
>  }
>  
> @@ -8879,7 +8845,7 @@ static int ufshcd_probe_hba(struct ufs_hba
> *hba)
>  		}
>  
>  		/* Reinit the device */
> -		ret = ufshcd_device_init(hba,
> /*init_dev_params=*/false);
> +		ret = ufshcd_device_init(hba);
>  		if (ret)
>  			goto out;
>  	}
>

On 9/1/24 12:25 PM, Bean Huo wrote:
> It's inconvenient to review them one by one, so until the last one. I
> understand the main purpose is to remove init_dev_params. Why not merge
> all the preparation patches into the last one?

The main purpose of this patch series is to combine the two
scsi_add_host() calls into a single call (see also the cover letter).
Something unfortunate about the current MCQ implementation is that
adding MCQ support introduced a new scsi_add_host() call far away from
the original scsi_add_host() call. Hence, there is a risk that the two 
code paths for adding a SCSI host in the UFS driver would start to
diverge. This patch series eliminates that risk by combining the two 
scsi_add_host() calls into a single call.

Removing the 'init_dev_params' argument is only a secondary goal of this
patch series.

I'm afraid if I would combine all nine patches into a single patch that
it would be very hard to review that single patch. Additionally, the
patch description would become very long. The kernel documentation says
the following about long patch descriptions: "If your description starts
to get long, that's a sign that you probably need to split up your
patch. See :ref:`split_changes`."

Thanks,

Bart.