[PATCH for rc] ufs: core: Configure MCQ after link startup

[PATCH for rc] ufs: core: Configure MCQ after link startup

[Bart Van Assche]

Commit f46b9a595fa9 ("scsi: ufs: core: Allocate the SCSI host earlier")
did not only cause scsi_add_host() to be called earlier. It also swapped
the order of link startup and enabling and configuring MCQ mode. Before
that commit, the call chains for link startup and enabling MCQ were as
follows:

ufshcd_init()
  ufshcd_link_startup()
  ufshcd_add_scsi_host()
    ufshcd_mcq_enable()

Apparently this change causes link startup to fail. Fix this by configuring
MCQ after link startup has completed.

Reported-by: Nitin Rawat <nitin.rawat@oss.qualcomm.com>
Fixes: f46b9a595fa9 ("scsi: ufs: core: Allocate the SCSI host earlier")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 80c0b49f30b0..bc395434c3a2 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -10733,9 +10733,7 @@ static int ufshcd_add_scsi_host(struct ufs_hba *hba)
 	if (is_mcq_supported(hba)) {
 		ufshcd_mcq_enable(hba);
 		err = ufshcd_alloc_mcq(hba);
-		if (!err) {
-			ufshcd_config_mcq(hba);
-		} else {
+		if (err) {
 			/* Continue with SDB mode */
 			ufshcd_mcq_disable(hba);
 			use_mcq_mode = false;
@@ -11008,6 +11006,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
 	if (err)
 		goto out_disable;
 
+	if (hba->mcq_enabled)
+		ufshcd_config_mcq(hba);
+
 	if (hba->quirks & UFSHCD_QUIRK_SKIP_PH_CONFIGURATION)
 		goto initialized;
 

Re: [PATCH for rc] ufs: core: Configure MCQ after link startup

[Peter Wang (王信友)]

On Thu, 2025-12-18 at 15:07 -0800, Bart Van Assche wrote:
> Commit f46b9a595fa9 ("scsi: ufs: core: Allocate the SCSI host
> earlier")
> did not only cause scsi_add_host() to be called earlier. It also
> swapped
> the order of link startup and enabling and configuring MCQ mode.
> Before
> that commit, the call chains for link startup and enabling MCQ were
> as
> follows:
> 
> ufshcd_init()
>   ufshcd_link_startup()
>   ufshcd_add_scsi_host()
>     ufshcd_mcq_enable()
> 
> Apparently this change causes link startup to fail. Fix this by
> configuring
> MCQ after link startup has completed.
> 
> Reported-by: Nitin Rawat <nitin.rawat@oss.qualcomm.com>
> Fixes: f46b9a595fa9 ("scsi: ufs: core: Allocate the SCSI host
> earlier")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>

Hi Bart,

It seems strange to me that ufshcd_mcq_enable is called before
ufshcd_link_startup.

In driver development, configuration (config) should generally
be done before enabling (enable) the hardware.
Reason:
Configuring first ensures the hardware operates correctly and
avoids unexpected behavior or safety issues.

Typical steps:
1. Set registers/parameters
2. Initialize resources
3. Configure interrupts/DMA
4. Enable the hardware

So, do you also consider moving ufshcd_mcq_enable after 
ufshcd_config_mcq?

Thanks.
Peter

Re: [PATCH for rc] ufs: core: Configure MCQ after link startup

[Bart Van Assche]

On 12/18/25 11:32 PM, Peter Wang (王信友) wrote:
> It seems strange to me that ufshcd_mcq_enable is called before
> ufshcd_link_startup.
> 
> In driver development, configuration (config) should generally
> be done before enabling (enable) the hardware.
> Reason:
> Configuring first ensures the hardware operates correctly and
> avoids unexpected behavior or safety issues.
> 
> Typical steps:
> 1. Set registers/parameters
> 2. Initialize resources
> 3. Configure interrupts/DMA
> 4. Enable the hardware
> 
> So, do you also consider moving ufshcd_mcq_enable after
> ufshcd_config_mcq?

This has been considered but unfortunately the UFSHCI register set
definition makes this impossible. hba->host->can_queue must be set
before scsi_add_host() is called. hba->host->can_queue is derived
from the controller capabilities register (CAP). MCQ must be enabled
before NUTRS is extracted from the CAP register. Otherwise NUTRS is
reported as 32 - 1 instead of 64 - 1 (assuming that the host
controller supports 64 outstanding commands in MCQ mode).

Thanks,

Bart.

Re: [PATCH for rc] ufs: core: Configure MCQ after link startup

[Peter Wang (王信友)]

On Fri, 2025-12-19 at 08:32 -0800, Bart Van Assche wrote:
> 
> This has been considered but unfortunately the UFSHCI register set
> definition makes this impossible. hba->host->can_queue must be set
> before scsi_add_host() is called. hba->host->can_queue is derived
> from the controller capabilities register (CAP). MCQ must be enabled
> before NUTRS is extracted from the CAP register. Otherwise NUTRS is
> reported as 32 - 1 instead of 64 - 1 (assuming that the host
> controller supports 64 outstanding commands in MCQ mode).
> 
> Thanks,
> 
> Bart.

Hi Bart,

Yes, this is indeed a specification limitation.
Thank you for your explanation.

Reviewed-by: Peter Wang <peter.wang@mediatek.com>

Re: [PATCH for rc] ufs: core: Configure MCQ after link startup

[Martin K. Petersen]

On Thu, 18 Dec 2025 15:07:37 -0800, Bart Van Assche wrote:

> Commit f46b9a595fa9 ("scsi: ufs: core: Allocate the SCSI host earlier")
> did not only cause scsi_add_host() to be called earlier. It also swapped
> the order of link startup and enabling and configuring MCQ mode. Before
> that commit, the call chains for link startup and enabling MCQ were as
> follows:
> 
> ufshcd_init()
>   ufshcd_link_startup()
>   ufshcd_add_scsi_host()
>     ufshcd_mcq_enable()
> 
> [...]

Applied to 6.19/scsi-fixes, thanks!

[1/1] ufs: core: Configure MCQ after link startup
      https://git.kernel.org/mkp/scsi/c/ee229e7c256a

-- 
Martin K. Petersen