[PATCH v6 00/11] Combine the two UFS driver scsi_add_host() calls

[PATCH v6 00/11] Combine the two UFS driver scsi_add_host() calls

[Bart Van Assche]

Hi Martin,

In the UFS driver 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.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Changes compared to v5:
 - Rebased this patch series on top of the v6.12-rc kernel.
 - Removed the probe_start member variable again from struct ufs_hba.
 - Renamed ufshcd_process_device_init_result() into
   ufshcd_process_probe_result().
 - Converted a comment about UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH into an
   explicit test.

Changes compared to v4:
 - Left out the changes that remove the 'init_dev_params' argument.
 - Added a few new patches.

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 (11):
  scsi: ufs: core: Introduce ufshcd_add_scsi_host()
  scsi: ufs: core: Introduce ufshcd_post_device_init()
  scsi: ufs: core: Call ufshcd_add_scsi_host() later
  scsi: ufs: core: Introduce ufshcd_process_probe_result()
  scsi: core: ufs: Convert a comment into an explicit check
  scsi: ufs: core: Move the ufshcd_device_init() calls
  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: Remove code that is no longer needed
  scsi: ufs: core: Move the MCQ scsi_add_host() call
  scsi: ufs: core: Move code out of an if-statement

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

[PATCH v6 01/11] scsi: ufs: core: Introduce ufshcd_add_scsi_host()

[Bart Van Assche]

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: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 96 ++++++++++++++++++++++++---------------
 1 file changed, 59 insertions(+), 37 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 9e6d008f4ea4..959509d9445e 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10377,6 +10377,62 @@ static const struct blk_mq_ops ufshcd_tmf_ops = {
 	.queue_rq = ufshcd_queue_tmf,
 };
 
+static int ufshcd_add_scsi_host(struct ufs_hba *hba)
+{
+	int err;
+
+	if (!is_mcq_supported(hba)) {
+		if (!hba->lsdb_sup) {
+			dev_err(hba->dev,
+				"%s: failed to initialize (legacy doorbell mode not supported)\n",
+				__func__);
+			return -EINVAL;
+		}
+		err = scsi_add_host(hba->host, hba->dev);
+		if (err) {
+			dev_err(hba->dev, "scsi_add_host failed\n");
+			return err;
+		}
+		hba->scsi_host_added = true;
+	}
+
+	hba->tmf_tag_set = (struct blk_mq_tag_set) {
+		.nr_hw_queues	= 1,
+		.queue_depth	= hba->nutmrs,
+		.ops		= &ufshcd_tmf_ops,
+		.flags		= BLK_MQ_F_NO_SCHED,
+	};
+	err = blk_mq_alloc_tag_set(&hba->tmf_tag_set);
+	if (err < 0)
+		goto remove_scsi_host;
+	hba->tmf_queue = blk_mq_alloc_queue(&hba->tmf_tag_set, NULL, NULL);
+	if (IS_ERR(hba->tmf_queue)) {
+		err = PTR_ERR(hba->tmf_queue);
+		goto free_tmf_tag_set;
+	}
+	hba->tmf_rqs = devm_kcalloc(hba->dev, hba->nutmrs,
+				    sizeof(*hba->tmf_rqs), GFP_KERNEL);
+	if (!hba->tmf_rqs) {
+		err = -ENOMEM;
+		goto free_tmf_queue;
+	}
+
+	return 0;
+
+free_tmf_queue:
+	blk_mq_destroy_queue(hba->tmf_queue);
+	blk_put_queue(hba->tmf_queue);
+
+free_tmf_tag_set:
+	blk_mq_free_tag_set(&hba->tmf_tag_set);
+
+remove_scsi_host:
+	if (hba->scsi_host_added)
+		scsi_remove_host(hba->host);
+
+	return err;
+}
+
 /**
  * ufshcd_init - Driver initialization routine
  * @hba: per-adapter instance
@@ -10508,41 +10564,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 		hba->is_irq_enabled = true;
 	}
 
-	if (!is_mcq_supported(hba)) {
-		if (!hba->lsdb_sup) {
-			dev_err(hba->dev, "%s: failed to initialize (legacy doorbell mode not supported)\n",
-				__func__);
-			err = -EINVAL;
-			goto out_disable;
-		}
-		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;
-	}
-
-	hba->tmf_tag_set = (struct blk_mq_tag_set) {
-		.nr_hw_queues	= 1,
-		.queue_depth	= hba->nutmrs,
-		.ops		= &ufshcd_tmf_ops,
-		.flags		= BLK_MQ_F_NO_SCHED,
-	};
-	err = blk_mq_alloc_tag_set(&hba->tmf_tag_set);
-	if (err < 0)
-		goto out_remove_scsi_host;
-	hba->tmf_queue = blk_mq_alloc_queue(&hba->tmf_tag_set, NULL, NULL);
-	if (IS_ERR(hba->tmf_queue)) {
-		err = PTR_ERR(hba->tmf_queue);
-		goto free_tmf_tag_set;
-	}
-	hba->tmf_rqs = devm_kcalloc(hba->dev, hba->nutmrs,
-				    sizeof(*hba->tmf_rqs), GFP_KERNEL);
-	if (!hba->tmf_rqs) {
-		err = -ENOMEM;
-		goto free_tmf_queue;
-	}
+	err = ufshcd_add_scsi_host(hba);
+	if (err)
+		goto out_disable;
 
 	/* Reset the attached device */
 	ufshcd_device_reset(hba);
@@ -10600,9 +10624,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 free_tmf_queue:
 	blk_mq_destroy_queue(hba->tmf_queue);
 	blk_put_queue(hba->tmf_queue);
-free_tmf_tag_set:
 	blk_mq_free_tag_set(&hba->tmf_tag_set);
-out_remove_scsi_host:
 	if (hba->scsi_host_added)
 		scsi_remove_host(hba->host);
 out_disable:

[PATCH v6 02/11] scsi: ufs: core: Introduce ufshcd_post_device_init()

[Bart Van Assche]

Prepare for inlining one ufshcd_device_init() call by introducing the new
function ufshcd_post_device_init(). No functionality has been changed.

Reviewed-by: Avri Altman <avri.altman@wdc.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 62 ++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 27 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 959509d9445e..17c22e880cef 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8732,6 +8732,40 @@ static void ufshcd_config_mcq(struct ufs_hba *hba)
 		 hba->nutrs);
 }
 
+static int ufshcd_post_device_init(struct ufs_hba *hba)
+{
+	int ret;
+
+	ufshcd_tune_unipro_params(hba);
+
+	/* UFS device is also active now */
+	ufshcd_set_ufs_dev_active(hba);
+	ufshcd_force_reset_auto_bkops(hba);
+
+	ufshcd_set_timestamp_attr(hba);
+	schedule_delayed_work(&hba->ufs_rtc_update_work,
+			      msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
+
+	if (!hba->max_pwr_info.is_valid)
+		return 0;
+
+	/*
+	 * Set the right value to bRefClkFreq before attempting to
+	 * switch to HS gears.
+	 */
+	if (hba->dev_ref_clk_freq != REF_CLK_FREQ_INVAL)
+		ufshcd_set_dev_ref_clk(hba);
+	/* Gear up to HS gear. */
+	ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
+	if (ret) {
+		dev_err(hba->dev, "%s: Failed setting power mode, err = %d\n",
+			__func__, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 {
 	int ret;
@@ -8801,33 +8835,7 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 		}
 	}
 
-	ufshcd_tune_unipro_params(hba);
-
-	/* UFS device is also active now */
-	ufshcd_set_ufs_dev_active(hba);
-	ufshcd_force_reset_auto_bkops(hba);
-
-	ufshcd_set_timestamp_attr(hba);
-	schedule_delayed_work(&hba->ufs_rtc_update_work,
-			      msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
-
-	/* Gear up to HS gear if supported */
-	if (hba->max_pwr_info.is_valid) {
-		/*
-		 * Set the right value to bRefClkFreq before attempting to
-		 * switch to HS gears.
-		 */
-		if (hba->dev_ref_clk_freq != REF_CLK_FREQ_INVAL)
-			ufshcd_set_dev_ref_clk(hba);
-		ret = ufshcd_config_pwr_mode(hba, &hba->max_pwr_info.info);
-		if (ret) {
-			dev_err(hba->dev, "%s: Failed setting power mode, err = %d\n",
-					__func__, ret);
-			return ret;
-		}
-	}
-
-	return 0;
+	return ufshcd_post_device_init(hba);
 }
 
 /**

[PATCH v6 03/11] scsi: ufs: core: Call ufshcd_add_scsi_host() later

[Bart Van Assche]

Call ufshcd_add_scsi_host() after host controller initialization has
completed. This is safe because no code between the old and new
ufshcd_add_scsi_host() call site depends on the scsi_add_host() call.

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Bean Huo <beanhuo@micron.com>
Reviewed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 17c22e880cef..17604397aaba 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10572,10 +10572,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 		hba->is_irq_enabled = true;
 	}
 
-	err = ufshcd_add_scsi_host(hba);
-	if (err)
-		goto out_disable;
-
 	/* Reset the attached device */
 	ufshcd_device_reset(hba);
 
@@ -10587,7 +10583,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 		dev_err(hba->dev, "Host controller enable failed\n");
 		ufshcd_print_evt_hist(hba);
 		ufshcd_print_host_state(hba);
-		goto free_tmf_queue;
+		goto out_disable;
 	}
 
 	/*
@@ -10622,6 +10618,10 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	 */
 	ufshcd_set_ufs_dev_active(hba);
 
+	err = ufshcd_add_scsi_host(hba);
+	if (err)
+		goto out_disable;
+
 	async_schedule(ufshcd_async_scan, hba);
 	ufs_sysfs_add_nodes(hba->dev);
 
@@ -10629,12 +10629,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	ufshcd_pm_qos_init(hba);
 	return 0;
 
-free_tmf_queue:
-	blk_mq_destroy_queue(hba->tmf_queue);
-	blk_put_queue(hba->tmf_queue);
-	blk_mq_free_tag_set(&hba->tmf_tag_set);
-	if (hba->scsi_host_added)
-		scsi_remove_host(hba->host);
 out_disable:
 	hba->is_irq_enabled = false;
 	ufshcd_hba_exit(hba);

[PATCH v6 04/11] scsi: ufs: core: Introduce ufshcd_process_probe_result()

[Bart Van Assche]

Prepare for moving a ufshcd_device_init() call from inside
ufshcd_probe_hba() into the ufshcd_probe_hba() callers by introducing
the function ufshcd_process_probe_result(). No functionality has been
changed.

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

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 17604397aaba..724060a9eae7 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -7684,6 +7684,29 @@ static int ufshcd_abort(struct scsi_cmnd *cmd)
 	return err;
 }
 
+/**
+ * ufshcd_process_probe_result - Process the ufshcd_probe_hba() result.
+ * @hba: UFS host controller instance.
+ * @probe_start: time when the ufshcd_probe_hba() call started.
+ * @ret: ufshcd_probe_hba() return value.
+ */
+static void ufshcd_process_probe_result(struct ufs_hba *hba,
+					ktime_t probe_start, int ret)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(hba->host->host_lock, flags);
+	if (ret)
+		hba->ufshcd_state = UFSHCD_STATE_ERROR;
+	else if (hba->ufshcd_state == UFSHCD_STATE_RESET)
+		hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
+	spin_unlock_irqrestore(hba->host->host_lock, flags);
+
+	trace_ufshcd_init(dev_name(hba->dev), ret,
+			  ktime_to_us(ktime_sub(ktime_get(), probe_start)),
+			  hba->curr_dev_pwr_mode, hba->uic_link_state);
+}
+
 /**
  * ufshcd_host_reset_and_restore - reset and restore host controller
  * @hba: per-adapter instance
@@ -8850,7 +8873,6 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 {
 	ktime_t start = ktime_get();
-	unsigned long flags;
 	int ret;
 
 	ret = ufshcd_device_init(hba, init_dev_params);
@@ -8896,16 +8918,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 	ufshcd_configure_auto_hibern8(hba);
 
 out:
-	spin_lock_irqsave(hba->host->host_lock, flags);
-	if (ret)
-		hba->ufshcd_state = UFSHCD_STATE_ERROR;
-	else if (hba->ufshcd_state == UFSHCD_STATE_RESET)
-		hba->ufshcd_state = UFSHCD_STATE_OPERATIONAL;
-	spin_unlock_irqrestore(hba->host->host_lock, flags);
-
-	trace_ufshcd_init(dev_name(hba->dev), ret,
-		ktime_to_us(ktime_sub(ktime_get(), start)),
-		hba->curr_dev_pwr_mode, hba->uic_link_state);
+	ufshcd_process_probe_result(hba, start, ret);
 	return ret;
 }
 

[PATCH v6 05/11] scsi: ufs: core: Convert a comment into an explicit check

[Bart Van Assche]

The comment /* UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH is set */ is
only correct if ufshcd_device_init() is only called by ufshcd_probe_hba().
Convert the comment into an explicit check. This patch prepares for moving
the ufshcd_device_init() calls.

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

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 724060a9eae7..7b8eaf2b9ce2 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8851,8 +8851,9 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 				return ret;
 			}
 			hba->scsi_host_added = true;
-		} else if (is_mcq_supported(hba)) {
-			/* UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH is set */
+		} else if (is_mcq_supported(hba) &&
+			   hba->quirks &
+				   UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH) {
 			ufshcd_config_mcq(hba);
 			ufshcd_mcq_enable(hba);
 		}

[PATCH v6 06/11] scsi: ufs: core: Move the ufshcd_device_init() calls

[Bart Van Assche]

Move the ufshcd_device_init() and ufshcd_process_hba_result() calls to
the ufshcd_probe_hba() callers. 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 | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 7b8eaf2b9ce2..8803031f1694 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -298,6 +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_probe_hba(struct ufs_hba *hba, bool init_dev_params);
 static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on);
 static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba);
@@ -7736,8 +7737,14 @@ 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);
+	if (!err) {
+		ktime_t probe_start = ktime_get();
+
+		err = ufshcd_device_init(hba, /*init_dev_params=*/false);
+		if (!err)
+			err = ufshcd_probe_hba(hba, false);
+		ufshcd_process_probe_result(hba, probe_start, err);
+	}
 
 	if (err)
 		dev_err(hba->dev, "%s: Host init failed %d\n", __func__, err);
@@ -8873,13 +8880,8 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
  */
 static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 {
-	ktime_t start = ktime_get();
 	int ret;
 
-	ret = ufshcd_device_init(hba, init_dev_params);
-	if (ret)
-		goto out;
-
 	if (!hba->pm_op_in_progress &&
 	    (hba->quirks & UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH)) {
 		/* Reset the device and controller before doing reinit */
@@ -8892,13 +8894,13 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 			dev_err(hba->dev, "Host controller enable failed\n");
 			ufshcd_print_evt_hist(hba);
 			ufshcd_print_host_state(hba);
-			goto out;
+			return ret;
 		}
 
 		/* Reinit the device */
 		ret = ufshcd_device_init(hba, init_dev_params);
 		if (ret)
-			goto out;
+			return ret;
 	}
 
 	ufshcd_print_pwr_info(hba);
@@ -8918,9 +8920,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 		ufshcd_write_ee_control(hba);
 	ufshcd_configure_auto_hibern8(hba);
 
-out:
-	ufshcd_process_probe_result(hba, start, ret);
-	return ret;
+	return 0;
 }
 
 /**
@@ -8931,11 +8931,16 @@ static int ufshcd_probe_hba(struct ufs_hba *hba, bool init_dev_params)
 static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 {
 	struct ufs_hba *hba = (struct ufs_hba *)data;
+	ktime_t probe_start;
 	int ret;
 
 	down(&hba->host_sem);
 	/* Initialize hba, detect and initialize UFS device */
-	ret = ufshcd_probe_hba(hba, true);
+	probe_start = ktime_get();
+	ret = ufshcd_device_init(hba, /*init_dev_params=*/true);
+	if (ret == 0)
+		ret = ufshcd_probe_hba(hba, true);
+	ufshcd_process_probe_result(hba, probe_start, ret);
 	up(&hba->host_sem);
 	if (ret)
 		goto out;

[PATCH v6 07/11] scsi: ufs: core: Move the ufshcd_device_init(hba, true) call

[Bart Van Assche]

ufshcd_async_scan() is called (asynchronously) only by ufshcd_init().
Move the ufshcd_device_init(hba, true) call from ufshcd_async_scan()
into ufshcd_init(). This patch prepares for moving both scsi_add_host()
calls into ufshcd_add_scsi_host(). Calling ufshcd_device_init() from
ufshcd_init() without holding hba->host_sem is safe. This is safe because
hba->host_sem serializes core code and sysfs callbacks. The
ufshcd_device_init() call is moved before the scsi_add_host() call and
hence happens before any SCSI sysfs attributes are created.

Since ufshcd_device_init() may call scsi_add_host(), only call
scsi_add_host() from ufshcd_add_scsi_host() if the SCSI host has not yet
been added by ufshcd_device_init().

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

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 8803031f1694..063b87be23e9 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8937,9 +8937,7 @@ static void ufshcd_async_scan(void *data, async_cookie_t cookie)
 	down(&hba->host_sem);
 	/* Initialize hba, detect and initialize UFS device */
 	probe_start = ktime_get();
-	ret = ufshcd_device_init(hba, /*init_dev_params=*/true);
-	if (ret == 0)
-		ret = ufshcd_probe_hba(hba, true);
+	ret = ufshcd_probe_hba(hba, true);
 	ufshcd_process_probe_result(hba, probe_start, ret);
 	up(&hba->host_sem);
 	if (ret)
@@ -10408,7 +10406,8 @@ static int ufshcd_add_scsi_host(struct ufs_hba *hba)
 {
 	int err;
 
-	if (!is_mcq_supported(hba)) {
+	if (!hba->scsi_host_added) {
+		WARN_ON_ONCE(is_mcq_supported(hba));
 		if (!hba->lsdb_sup) {
 			dev_err(hba->dev,
 				"%s: failed to initialize (legacy doorbell mode not supported)\n",
@@ -10637,6 +10636,11 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	 */
 	ufshcd_set_ufs_dev_active(hba);
 
+	/* Initialize hba, detect and initialize UFS device */
+	err = ufshcd_device_init(hba, /*init_dev_params=*/true);
+	if (err)
+		goto out_disable;
+
 	err = ufshcd_add_scsi_host(hba);
 	if (err)
 		goto out_disable;

[PATCH v6 08/11] scsi: ufs: core: Expand the ufshcd_device_init(hba, true) call

[Bart Van Assche]

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 | 56 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 063b87be23e9..d09aa3763f88 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10637,7 +10637,61 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	ufshcd_set_ufs_dev_active(hba);
 
 	/* Initialize hba, detect and initialize UFS device */
-	err = ufshcd_device_init(hba, /*init_dev_params=*/true);
+	ktime_t probe_start = ktime_get();
+
+	hba->ufshcd_state = UFSHCD_STATE_RESET;
+
+	err = ufshcd_link_startup(hba);
+	if (err)
+		goto out_disable;
+
+	if (hba->quirks & UFSHCD_QUIRK_SKIP_PH_CONFIGURATION)
+		goto initialized;
+
+	/* Debug counters initialization */
+	ufshcd_clear_dbg_ufs_stats(hba);
+
+	/* UniPro link is active now */
+	ufshcd_set_link_active(hba);
+
+	/* Verify device initialization by sending NOP OUT UPIU */
+	err = ufshcd_verify_dev_init(hba);
+	if (err)
+		goto out_disable;
+
+	/* Initiate UFS initialization, and waiting until completion */
+	err = ufshcd_complete_dev_init(hba);
+	if (err)
+		goto out_disable;
+
+	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;
+			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);
+
+initialized:
+	ufshcd_process_probe_result(hba, probe_start, err);
 	if (err)
 		goto out_disable;
 

[PATCH v6 09/11] scsi: ufs: core: Remove code that is no longer needed

[Bart Van Assche]

Previous changes guarantee that hba->scsi_host_added is true before
ufshcd_device_init() is called. Hence, remove the code from
ufshcd_device_init() that depends on hba->scsi_host_added being false.

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

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d09aa3763f88..e3f433f221fc 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8799,7 +8799,8 @@ static int ufshcd_post_device_init(struct ufs_hba *hba)
 static int ufshcd_device_init(struct ufs_hba *hba, bool init_dev_params)
 {
 	int ret;
-	struct Scsi_Host *host = hba->host;
+
+	WARN_ON_ONCE(!hba->scsi_host_added);
 
 	hba->ufshcd_state = UFSHCD_STATE_RESET;
 
@@ -8840,27 +8841,8 @@ static int ufshcd_device_init(struct ufs_hba *hba, bool 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) &&
-			   hba->quirks &
-				   UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH) {
+		if (is_mcq_supported(hba) &&
+		    hba->quirks & UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH) {
 			ufshcd_config_mcq(hba);
 			ufshcd_mcq_enable(hba);
 		}

[PATCH v6 10/11] scsi: ufs: core: Move the MCQ scsi_add_host() call

[Bart Van Assche]

Whether or not MCQ is used, call scsi_add_host from ufshcd_add_scsi_host().
For MCQ this patch swaps the order of the scsi_add_host() and UFS device
initialization. This patch prepares for combining the two scsi_add_host()
calls.

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

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index e3f433f221fc..f34563e3a51d 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10388,8 +10388,25 @@ static int ufshcd_add_scsi_host(struct ufs_hba *hba)
 {
 	int err;
 
-	if (!hba->scsi_host_added) {
-		WARN_ON_ONCE(is_mcq_supported(hba));
+	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;
+			dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
+				err);
+		}
+		err = scsi_add_host(hba->host, hba->dev);
+		if (err) {
+			dev_err(hba->dev, "scsi_add_host failed\n");
+			return err;
+		}
+		hba->scsi_host_added = true;
+	} else {
 		if (!hba->lsdb_sup) {
 			dev_err(hba->dev,
 				"%s: failed to initialize (legacy doorbell mode not supported)\n",
@@ -10650,26 +10667,6 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	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;
-			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);
 
 initialized:

[PATCH v6 11/11] scsi: ufs: core: Move code out of an if-statement

[Bart Van Assche]

The previous patch in this series introduced identical code in both
branches of an if-statement. Move that code outside the if-statement.

Reviewed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index f34563e3a51d..70d89e154c4f 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10400,26 +10400,20 @@ static int ufshcd_add_scsi_host(struct ufs_hba *hba)
 			dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
 				err);
 		}
-		err = scsi_add_host(hba->host, hba->dev);
-		if (err) {
-			dev_err(hba->dev, "scsi_add_host failed\n");
-			return err;
-		}
-		hba->scsi_host_added = true;
-	} else {
-		if (!hba->lsdb_sup) {
-			dev_err(hba->dev,
-				"%s: failed to initialize (legacy doorbell mode not supported)\n",
-				__func__);
-			return -EINVAL;
-		}
-		err = scsi_add_host(hba->host, hba->dev);
-		if (err) {
-			dev_err(hba->dev, "scsi_add_host failed\n");
-			return err;
-		}
-		hba->scsi_host_added = true;
 	}
+	if (!is_mcq_supported(hba) && !hba->lsdb_sup) {
+		dev_err(hba->dev,
+			"%s: failed to initialize (legacy doorbell mode not supported)\n",
+			__func__);
+		return -EINVAL;
+	}
+
+	err = scsi_add_host(hba->host, hba->dev);
+	if (err) {
+		dev_err(hba->dev, "scsi_add_host failed\n");
+		return err;
+	}
+	hba->scsi_host_added = true;
 
 	hba->tmf_tag_set = (struct blk_mq_tag_set) {
 		.nr_hw_queues	= 1,

Re: [PATCH v6 11/11] scsi: ufs: core: Move code out of an if-statement

[Neil Armstrong]

Hi,

On 16/10/2024 22:12, 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.
> 
> Reviewed-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ufs/core/ufshcd.c | 32 +++++++++++++-------------------
>   1 file changed, 13 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index f34563e3a51d..70d89e154c4f 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -10400,26 +10400,20 @@ static int ufshcd_add_scsi_host(struct ufs_hba *hba)
>   			dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
>   				err);
>   		}
> -		err = scsi_add_host(hba->host, hba->dev);
> -		if (err) {
> -			dev_err(hba->dev, "scsi_add_host failed\n");
> -			return err;
> -		}
> -		hba->scsi_host_added = true;
> -	} else {
> -		if (!hba->lsdb_sup) {
> -			dev_err(hba->dev,
> -				"%s: failed to initialize (legacy doorbell mode not supported)\n",
> -				__func__);
> -			return -EINVAL;
> -		}
> -		err = scsi_add_host(hba->host, hba->dev);
> -		if (err) {
> -			dev_err(hba->dev, "scsi_add_host failed\n");
> -			return err;
> -		}
> -		hba->scsi_host_added = true;
>   	}
> +	if (!is_mcq_supported(hba) && !hba->lsdb_sup) {
> +		dev_err(hba->dev,
> +			"%s: failed to initialize (legacy doorbell mode not supported)\n",
> +			__func__);
> +		return -EINVAL;
> +	}
> +
> +	err = scsi_add_host(hba->host, hba->dev);
> +	if (err) {
> +		dev_err(hba->dev, "scsi_add_host failed\n");
> +		return err;
> +	}
> +	hba->scsi_host_added = true;
>   
>   	hba->tmf_tag_set = (struct blk_mq_tag_set) {
>   		.nr_hw_queues	= 1,
> 

This change regresses the Qualcomm SM8650 Platforms, QRD and HDK boards fails to boot:
https://git.codelinaro.org/linaro/qcomlt/ci/staging/cdba-tester/-/jobs/182758#L1200

[    5.155432] ufshcd-qcom 1d84000.ufshc: Resource ufs_mem not provided
[    5.155439] ufshcd-qcom 1d84000.ufshc: MCQ mode is disabled, err=-19
[    5.155443] ufshcd-qcom 1d84000.ufshc: ufshcd_add_scsi_host: failed to initialize (legacy doorbell mode not supported)
[    5.155874] ufshcd-qcom 1d84000.ufshc: error -EINVAL: Initialization failed with error -22

then causes system crash:
[   15.400948] Internal error: Oops: 0000000096000006 [#1] PREEMPT SMP
<snip>
[   15.544573] CPU: 1 UID: 0 PID: 87 Comm: kworker/1:1 Tainted: G S                 6.12.0-rc5-next-20241030 #1
[   15.554938] Tainted: [S]=CPU_OUT_OF_SPEC
[   15.559098] Hardware name: Qualcomm Technologies, Inc. SM8650 QRD (DT)
[   15.565984] Workqueue: events ufshcd_rtc_work
[   15.570628] pstate: 234000c5 (nzCv daIF +PAN -UAO +TCO +DIT -SSBS BTYPE=--)
[   15.577986] pc : _raw_spin_lock_irqsave+0x34/0x8c
[   15.582981] lr : pm_runtime_get_if_active+0x24/0x9c
[   15.588159] sp : ffff800080df3d10
[   15.591679] x29: ffff800080df3d10 x28: 0000000000000000 x27: 0000000000000000
[   15.599235] x26: 0000000000000000 x25: ffff713a408fa1c0 x24: ffff713a40020205
[   15.606788] x23: 0000000000000000 x22: 0000000000000001 x21: ffff713a427ca848
[   15.614344] x20: 00000000000002a4 x19: 0000000000000000 x18: 0000000000000000
[   15.621896] x17: 0000000000000000 x16: 0000000000000000 x15: 00003a474f4572ec
[   15.629449] x14: 0000476e937c7726 x13: ffff713a40b64580 x12: 0000000000000001
[   15.637005] x11: 000000038cd4b97b x10: 3fb907ccb9c2fb76 x9 : 1eb87ffdfb6b34c8
[   15.644559] x8 : ffff713a40b655e8 x7 : ffff713a408fa200 x6 : 0000000000000020
[   15.652113] x5 : 000073746e657665 x4 : 0000000000000000 x3 : 000000003b9ac9ff
[   15.659665] x2 : 0000000000000001 x1 : 0000000000000000 x0 : 00000000000002a4
[   15.667218] Call trace:
[   15.669833]  _raw_spin_lock_irqsave+0x34/0x8c (P)
[   15.674829]  pm_runtime_get_if_active+0x24/0x9c (L)
[   15.679998]  pm_runtime_get_if_active+0x24/0x9c
[   15.684811]  ufshcd_rtc_work+0x138/0x1b4
[   15.688991]  process_one_work+0x148/0x288
[   15.693258]  worker_thread+0x2cc/0x3d4
[   15.697248]  kthread+0x110/0x114
[   15.700703]  ret_from_fork+0x10/0x20
[   15.704516] Code: b9000841 d503201f 52800001 52800022 (88e17c02)
[   15.710956] ---[ end trace 0000000000000000 ]---

Since next-20241028, bisect log on next-20241030:
# bad: [86e3904dcdc7e70e3257fc1de294a1b75f3d8d04] Add linux-next specific files for 20241030
# good: [81983758430957d9a5cb3333fe324fd70cf63e7e] Linux 6.12-rc5
git bisect start 'HEAD' 'v6.12-rc5'
# good: [9bebee868a9f662d3829e7529b99d63f14245673] Merge branch 'nand/next' of git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
git bisect good 9bebee868a9f662d3829e7529b99d63f14245673
# good: [64f1d5c3ad7542ea8f979988d2af75fd4e18148e] Merge branch 'for-backlight-next' of git://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git
git bisect good 64f1d5c3ad7542ea8f979988d2af75fd4e18148e
# good: [38754c326c1fa76b673652b406d06d10004bf372] Merge branch 'char-misc-next' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
git bisect good 38754c326c1fa76b673652b406d06d10004bf372
# bad: [08b6666d1a597b0ac3753ed4228707e5067db303] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git
git bisect bad 08b6666d1a597b0ac3753ed4228707e5067db303
# good: [27709a220e05ec666c65b2266162bdd51a1f9b58] Merge branch 'spmi-next' of git://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
git bisect good 27709a220e05ec666c65b2266162bdd51a1f9b58
# good: [0edaa545afbbcd7e8ff162f9fd8852c3589d2fa6] staging: gpib: fmh_gpib: Fix typo
git bisect good 0edaa545afbbcd7e8ff162f9fd8852c3589d2fa6
# bad: [b92e5937e3523b0b7d41373681256bec78d7e134] scsi: ufs: core: Move code out of an if-statement
git bisect bad b92e5937e3523b0b7d41373681256bec78d7e134
# good: [a3517717c3c0dbad771f5e491191b4b7b69808fb] Merge patch series "scsi: hisi_sas: Some fixes for hisi_sas"
git bisect good a3517717c3c0dbad771f5e491191b4b7b69808fb
# good: [5824e18b3db468e6eb5e9ef226eed80db26f581a] scsi: ufs: core: Remove redundant host_lock calls around UTMRLCLR
git bisect good 5824e18b3db468e6eb5e9ef226eed80db26f581a
# good: [0936001322646a15d7091f61232e5ded9bf1883f] scsi: ufs: core: Convert a comment into an explicit check
git bisect good 0936001322646a15d7091f61232e5ded9bf1883f
# good: [a390e6677f4119e3b9e6364ac2c5cbe3ef1321a2] scsi: ufs: core: Expand the ufshcd_device_init(hba, true) call
git bisect good a390e6677f4119e3b9e6364ac2c5cbe3ef1321a2
# good: [72e979225ed2e9427396e317d33050bcf50ad899] scsi: ufs: core: Move the MCQ scsi_add_host() call
git bisect good 72e979225ed2e9427396e317d33050bcf50ad899
# first bad commit: [b92e5937e3523b0b7d41373681256bec78d7e134] scsi: ufs: core: Move code out of an if-statement

#regzbot introduced b92e5937e3523b0b7d41373681256bec78d7e134

Thanks,
Neil

Re: [PATCH v6 11/11] scsi: ufs: core: Move code out of an if-statement

[Bart Van Assche]

On 10/31/24 7:46 AM, Neil Armstrong wrote:
> This change regresses the Qualcomm SM8650 Platforms, QRD and HDK boards 
> fails to boot:
> https://git.codelinaro.org/linaro/qcomlt/ci/staging/cdba-tester/-/jobs/182758#L1200
> 
> [    5.155432] ufshcd-qcom 1d84000.ufshc: Resource ufs_mem not provided
> [    5.155439] ufshcd-qcom 1d84000.ufshc: MCQ mode is disabled, err=-19
> [    5.155443] ufshcd-qcom 1d84000.ufshc: ufshcd_add_scsi_host: failed 
> to initialize (legacy doorbell mode not supported)
> [    5.155874] ufshcd-qcom 1d84000.ufshc: error -EINVAL: Initialization 
> failed with error -22
> 
> then causes system crash:
> [   15.400948] Internal error: Oops: 0000000096000006 [#1] PREEMPT SMP
> [   15.667218] Call trace:
> [   15.669833]  _raw_spin_lock_irqsave+0x34/0x8c (P)
> [   15.674829]  pm_runtime_get_if_active+0x24/0x9c (L)
> [   15.679998]  pm_runtime_get_if_active+0x24/0x9c
> [   15.684811]  ufshcd_rtc_work+0x138/0x1b4
> [   15.688991]  process_one_work+0x148/0x288
> [   15.693258]  worker_thread+0x2cc/0x3d4
> [   15.697248]  kthread+0x110/0x114
> [   15.700703]  ret_from_fork+0x10/0x20
> [   15.704516] Code: b9000841 d503201f 52800001 52800022 (88e17c02)
> [   15.710956] ---[ end trace 0000000000000000 ]---

Hi Neil,

Thank you for the very detailed report. I think that two bugs are being
reported:
* Support for non-MCQ UFSHCI 4.0 controllers is broken.
* The RTC update code is activated too early.

Is the patch below sufficient to fix both issues?

Thanks,

Bart.


diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 0787387b7ce1..0b6b0cd4af33 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -8620,6 +8620,13 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
  		ufshcd_init_clk_scaling_sysfs(hba);
  	}

+	/*
+	 * The RTC update code accesses the hba->ufs_device_wlun->sdev_gendev
+	 * pointer.
+	 */
+	schedule_delayed_work(&hba->ufs_rtc_update_work,
+			      msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
+
  	ufs_bsg_probe(hba);
  	scsi_scan_host(hba->host);

@@ -8714,8 +8721,6 @@ static int ufshcd_post_device_init(struct ufs_hba 
*hba)
  	ufshcd_force_reset_auto_bkops(hba);

  	ufshcd_set_timestamp_attr(hba);
-	schedule_delayed_work(&hba->ufs_rtc_update_work,
-			      msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));

  	if (!hba->max_pwr_info.is_valid)
  		return 0;
@@ -10345,8 +10350,7 @@ static int ufshcd_add_scsi_host(struct ufs_hba *hba)
  			dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
  				err);
  		}
-	}
-	if (!is_mcq_supported(hba) && !hba->lsdb_sup) {
+	} else if (!hba->lsdb_sup) {
  		dev_err(hba->dev,
  			"%s: failed to initialize (legacy doorbell mode not supported)\n",
  			__func__);

Re: [PATCH v6 11/11] scsi: ufs: core: Move code out of an if-statement

[Neil Armstrong]

Le 31/10/2024 à 18:51, Bart Van Assche a écrit :
> On 10/31/24 7:46 AM, Neil Armstrong wrote:
>> This change regresses the Qualcomm SM8650 Platforms, QRD and HDK boards fails to boot:
>> https://git.codelinaro.org/linaro/qcomlt/ci/staging/cdba-tester/-/jobs/182758#L1200
>>
>> [    5.155432] ufshcd-qcom 1d84000.ufshc: Resource ufs_mem not provided
>> [    5.155439] ufshcd-qcom 1d84000.ufshc: MCQ mode is disabled, err=-19
>> [    5.155443] ufshcd-qcom 1d84000.ufshc: ufshcd_add_scsi_host: failed to initialize (legacy doorbell mode not supported)
>> [    5.155874] ufshcd-qcom 1d84000.ufshc: error -EINVAL: Initialization failed with error -22
>>
>> then causes system crash:
>> [   15.400948] Internal error: Oops: 0000000096000006 [#1] PREEMPT SMP
>> [   15.667218] Call trace:
>> [   15.669833]  _raw_spin_lock_irqsave+0x34/0x8c (P)
>> [   15.674829]  pm_runtime_get_if_active+0x24/0x9c (L)
>> [   15.679998]  pm_runtime_get_if_active+0x24/0x9c
>> [   15.684811]  ufshcd_rtc_work+0x138/0x1b4
>> [   15.688991]  process_one_work+0x148/0x288
>> [   15.693258]  worker_thread+0x2cc/0x3d4
>> [   15.697248]  kthread+0x110/0x114
>> [   15.700703]  ret_from_fork+0x10/0x20
>> [   15.704516] Code: b9000841 d503201f 52800001 52800022 (88e17c02)
>> [   15.710956] ---[ end trace 0000000000000000 ]---
> 
> Hi Neil,
> 
> Thank you for the very detailed report. I think that two bugs are being
> reported:
> * Support for non-MCQ UFSHCI 4.0 controllers is broken.
> * The RTC update code is activated too early.
> 
> Is the patch below sufficient to fix both issues?

Yes it does!

Please add my:
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # SM8650-QRD

Thanks,
Neil

> 
> Thanks,
> 
> Bart.
> 
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 0787387b7ce1..0b6b0cd4af33 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -8620,6 +8620,13 @@ static int ufshcd_add_lus(struct ufs_hba *hba)
>           ufshcd_init_clk_scaling_sysfs(hba);
>       }
> 
> +    /*
> +     * The RTC update code accesses the hba->ufs_device_wlun->sdev_gendev
> +     * pointer.
> +     */
> +    schedule_delayed_work(&hba->ufs_rtc_update_work,
> +                  msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
> +
>       ufs_bsg_probe(hba);
>       scsi_scan_host(hba->host);
> 
> @@ -8714,8 +8721,6 @@ static int ufshcd_post_device_init(struct ufs_hba *hba)
>       ufshcd_force_reset_auto_bkops(hba);
> 
>       ufshcd_set_timestamp_attr(hba);
> -    schedule_delayed_work(&hba->ufs_rtc_update_work,
> -                  msecs_to_jiffies(UFS_RTC_UPDATE_INTERVAL_MS));
> 
>       if (!hba->max_pwr_info.is_valid)
>           return 0;
> @@ -10345,8 +10350,7 @@ static int ufshcd_add_scsi_host(struct ufs_hba *hba)
>               dev_err(hba->dev, "MCQ mode is disabled, err=%d\n",
>                   err);
>           }
> -    }
> -    if (!is_mcq_supported(hba) && !hba->lsdb_sup) {
> +    } else if (!hba->lsdb_sup) {
>           dev_err(hba->dev,
>               "%s: failed to initialize (legacy doorbell mode not supported)\n",
>               __func__);
> 

Re: [PATCH v6 11/11] scsi: ufs: core: Move code out of an if-statement

[Bart Van Assche]

On 10/31/24 12:55 PM, Neil Armstrong wrote:
> Le 31/10/2024 à 18:51, Bart Van Assche a écrit :
>> Is the patch below sufficient to fix both issues?
> 
> Yes it does!

Thank you for having tested this patch quickly. Would it be possible
to test whether the patch below also fixes the reported boot failure?
I think the patch below is a better fix.

Thanks,

Bart.


diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index a5a0646bb80a..3b592492e152 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -874,7 +874,8 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba 
*hba)
  	if (host->hw_ver.major > 0x3)
  		hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH;

-	if (of_device_is_compatible(hba->dev->of_node, "qcom,sm8550-ufshc"))
+	if (of_device_is_compatible(hba->dev->of_node, "qcom,sm8550-ufshc") ||
+	    of_device_is_compatible(hba->dev->of_node, "qcom,sm8650-ufshc"))
  		hba->quirks |= UFSHCD_QUIRK_BROKEN_LSDBS_CAP;
  }

Re: [PATCH v6 11/11] scsi: ufs: core: Move code out of an if-statement

[Bart Van Assche]

On 10/31/24 2:15 PM, Bart Van Assche wrote:
> On 10/31/24 12:55 PM, Neil Armstrong wrote:
>> Le 31/10/2024 à 18:51, Bart Van Assche a écrit :
>>> Is the patch below sufficient to fix both issues?
>>
>> Yes it does!
> 
> Thank you for having tested this patch quickly. Would it be possible
> to test whether the patch below also fixes the reported boot failure?
> I think the patch below is a better fix.
> 
> Thanks,
> 
> Bart.
> 
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index a5a0646bb80a..3b592492e152 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -874,7 +874,8 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba 
> *hba)
>       if (host->hw_ver.major > 0x3)
>           hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH;
> 
> -    if (of_device_is_compatible(hba->dev->of_node, "qcom,sm8550-ufshc"))
> +    if (of_device_is_compatible(hba->dev->of_node, "qcom,sm8550-ufshc") ||
> +        of_device_is_compatible(hba->dev->of_node, "qcom,sm8650-ufshc"))
>           hba->quirks |= UFSHCD_QUIRK_BROKEN_LSDBS_CAP;
>   }

(replying to my own email)

Can anyone who has access to a Qualcomm SM8650 Platform please help with
testing the above patch on top of linux-next?

Thanks,

Bart.

Re: [PATCH v6 11/11] scsi: ufs: core: Move code out of an if-statement

[Neil Armstrong]

Hi,

On 05/11/2024 23:01, Bart Van Assche wrote:
> On 10/31/24 2:15 PM, Bart Van Assche wrote:
>> On 10/31/24 12:55 PM, Neil Armstrong wrote:
>>> Le 31/10/2024 à 18:51, Bart Van Assche a écrit :
>>>> Is the patch below sufficient to fix both issues?
>>>
>>> Yes it does!
>>
>> Thank you for having tested this patch quickly. Would it be possible
>> to test whether the patch below also fixes the reported boot failure?
>> I think the patch below is a better fix.
>>
>> Thanks,
>>
>> Bart.
>>
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index a5a0646bb80a..3b592492e152 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -874,7 +874,8 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba)
>>       if (host->hw_ver.major > 0x3)
>>           hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH;
>>
>> -    if (of_device_is_compatible(hba->dev->of_node, "qcom,sm8550-ufshc"))
>> +    if (of_device_is_compatible(hba->dev->of_node, "qcom,sm8550-ufshc") ||
>> +        of_device_is_compatible(hba->dev->of_node, "qcom,sm8650-ufshc"))
>>           hba->quirks |= UFSHCD_QUIRK_BROKEN_LSDBS_CAP;
>>   }
> 
> (replying to my own email)
> 
> Can anyone who has access to a Qualcomm SM8650 Platform please help with
> testing the above patch on top of linux-next?

Sorry I was traveling and I forgot about it, I'll try to test this today.

Neil

> 
> Thanks,
> 
> Bart.

Re: [PATCH v6 11/11] scsi: ufs: core: Move code out of an if-statement

[Neil Armstrong]

+ Mani

On 05/11/2024 23:01, Bart Van Assche wrote:
> On 10/31/24 2:15 PM, Bart Van Assche wrote:
>> On 10/31/24 12:55 PM, Neil Armstrong wrote:
>>> Le 31/10/2024 à 18:51, Bart Van Assche a écrit :
>>>> Is the patch below sufficient to fix both issues?
>>>
>>> Yes it does!
>>
>> Thank you for having tested this patch quickly. Would it be possible
>> to test whether the patch below also fixes the reported boot failure?
>> I think the patch below is a better fix.
>>
>> Thanks,
>>
>> Bart.
>>
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index a5a0646bb80a..3b592492e152 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -874,7 +874,8 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba)
>>       if (host->hw_ver.major > 0x3)
>>           hba->quirks |= UFSHCD_QUIRK_REINIT_AFTER_MAX_GEAR_SWITCH;
>>
>> -    if (of_device_is_compatible(hba->dev->of_node, "qcom,sm8550-ufshc"))
>> +    if (of_device_is_compatible(hba->dev->of_node, "qcom,sm8550-ufshc") ||
>> +        of_device_is_compatible(hba->dev->of_node, "qcom,sm8650-ufshc"))
>>           hba->quirks |= UFSHCD_QUIRK_BROKEN_LSDBS_CAP;
>>   }
> 
> (replying to my own email)
> 
> Can anyone who has access to a Qualcomm SM8650 Platform please help with
> testing the above patch on top of linux-next?
> 
> Thanks,
> 
> Bart.

Thanks, it does fix the issue. But it won't scale since the next platforms will
probably need the same tweak in the future.

Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-HDK
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD

Thanks,
Neil

Re: [PATCH v6 11/11] scsi: ufs: core: Move code out of an if-statement

[Bart Van Assche]

On 11/6/24 1:57 AM, Neil Armstrong wrote:
> Thanks, it does fix the issue. But it won't scale since the next 
> platforms will probably need the same tweak in the future.
> 
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-HDK
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD

Hi Neil,

Thanks for testing!

The comment about future platforms is not clear to me. This patch fixes
support for a non-standard controller (reports UFSHCI version 4 but
supports UFSHCI 3 register set). Shouldn't all future platforms support
the UFSHCI 4 register set?

Thanks,

Bart.

Re: [PATCH v6 11/11] scsi: ufs: core: Move code out of an if-statement

[Manivannan Sadhasivam]

On Wed, Nov 06, 2024 at 09:51:06AM -0800, Bart Van Assche wrote:
> On 11/6/24 1:57 AM, Neil Armstrong wrote:
> > Thanks, it does fix the issue. But it won't scale since the next
> > platforms will probably need the same tweak in the future.
> > 
> > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-HDK
> > Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
> 
> Hi Neil,
> 
> Thanks for testing!
> 
> The comment about future platforms is not clear to me. This patch fixes
> support for a non-standard controller (reports UFSHCI version 4 but
> supports UFSHCI 3 register set). Shouldn't all future platforms support
> the UFSHCI 4 register set?
> 

Yeah, the future platforms should (hopefully) no longer have this issue.

- Mani

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

Re: [PATCH v6 00/11] Combine the two UFS driver scsi_add_host() calls

[Martin K. Petersen]

Bart,

> In the UFS driver 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.

Applied to 6.13/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v6 00/11] Combine the two UFS driver scsi_add_host() calls

[Martin K. Petersen]

On Wed, 16 Oct 2024 13:11:56 -0700, Bart Van Assche wrote:

> In the UFS driver 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.
> 
> Please consider this patch series for the next merge window.
> 
> Thanks,
> 
> [...]

Applied to 6.13/scsi-queue, thanks!

[01/11] scsi: ufs: core: Introduce ufshcd_add_scsi_host()
        https://git.kernel.org/mkp/scsi/c/17a973970397
[02/11] scsi: ufs: core: Introduce ufshcd_post_device_init()
        https://git.kernel.org/mkp/scsi/c/3192d28ec660
[03/11] scsi: ufs: core: Call ufshcd_add_scsi_host() later
        https://git.kernel.org/mkp/scsi/c/7702c7f64f2d
[04/11] scsi: ufs: core: Introduce ufshcd_process_probe_result()
        https://git.kernel.org/mkp/scsi/c/18ec23b60822
[05/11] scsi: ufs: core: Convert a comment into an explicit check
        https://git.kernel.org/mkp/scsi/c/093600132264
[06/11] scsi: ufs: core: Move the ufshcd_device_init() calls
        https://git.kernel.org/mkp/scsi/c/639e2043b589
[07/11] scsi: ufs: core: Move the ufshcd_device_init(hba, true) call
        https://git.kernel.org/mkp/scsi/c/69f5eb78d4b0
[08/11] scsi: ufs: core: Expand the ufshcd_device_init(hba, true) call
        https://git.kernel.org/mkp/scsi/c/a390e6677f41
[09/11] scsi: ufs: core: Remove code that is no longer needed
        https://git.kernel.org/mkp/scsi/c/b6195d02b914
[10/11] scsi: ufs: core: Move the MCQ scsi_add_host() call
        https://git.kernel.org/mkp/scsi/c/72e979225ed2
[11/11] scsi: ufs: core: Move code out of an if-statement
        https://git.kernel.org/mkp/scsi/c/b92e5937e352

-- 
Martin K. Petersen	Oracle Linux Engineering