| Message ID | 20240828174435.2469498-10-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Simplify the UFS driver initialization code | expand |
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.
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; }
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(-)