[PATCH v1] ufs: core: fix hwq_id type and value

[PATCH v1] ufs: core: fix hwq_id type and value

[peter.wang]

From: Peter Wang <peter.wang@mediatek.com>

Because the member id of struct ufs_hw_queue is u32 (hwq->id) and
the trace entry hwq_id is also u32, the type should be changed to u32.
If mcq is not supported, SDB mode only supports one hardware queue,
for which setting the hwq_id to 0 is more suitable.

Fixes: 4a52338bf288 ("scsi: ufs: core: Add trace event for MCQ")
Cc: <stable@vger.kernel.org>
Signed-off-by: Peter Wang <peter.wang@mediatek.com>
---
 drivers/ufs/core/ufshcd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 7735421e3991..14e4cfbcb9eb 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -432,7 +432,7 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
 	u8 opcode = 0, group_id = 0;
 	u32 doorbell = 0;
 	u32 intr;
-	int hwq_id = -1;
+	u32 hwq_id = 0;
 	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
 	struct scsi_cmnd *cmd = lrbp->cmd;
 	struct request *rq = scsi_cmd_to_rq(cmd);
-- 
2.45.2

Re: [PATCH v1] ufs: core: fix hwq_id type and value

[Bart Van Assche]

On 5/6/25 5:39 AM, peter.wang@mediatek.com wrote:
> From: Peter Wang <peter.wang@mediatek.com>
> 
> Because the member id of struct ufs_hw_queue is u32 (hwq->id) and
> the trace entry hwq_id is also u32, the type should be changed to u32.
> If mcq is not supported, SDB mode only supports one hardware queue,
> for which setting the hwq_id to 0 is more suitable.
> 
> Fixes: 4a52338bf288 ("scsi: ufs: core: Add trace event for MCQ")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Peter Wang <peter.wang@mediatek.com>
> ---
>   drivers/ufs/core/ufshcd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 7735421e3991..14e4cfbcb9eb 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -432,7 +432,7 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
>   	u8 opcode = 0, group_id = 0;
>   	u32 doorbell = 0;
>   	u32 intr;
> -	int hwq_id = -1;
> +	u32 hwq_id = 0;
>   	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
>   	struct scsi_cmnd *cmd = lrbp->cmd;
>   	struct request *rq = scsi_cmd_to_rq(cmd);

Is this change really necessary? I like the current behavior because it
makes it easy to figure out whether or not MCQ has been enabled. Even if
others would agree with this change, I think that the "Fixes:" and "Cc:
stable" tags are overkill because I don't see this as a bug fix but
rather as a behavior change that is not a bug fix.

Thanks,

Bart.

Re: [PATCH v1] ufs: core: fix hwq_id type and value

[Peter Wang (王信友)]

On Tue, 2025-05-06 at 09:15 -0700, Bart Van Assche wrote:
> 
> Is this change really necessary? I like the current behavior because
> it
> makes it easy to figure out whether or not MCQ has been enabled. Even
> if
> others would agree with this change, I think that the "Fixes:" and
> "Cc:
> stable" tags are overkill because I don't see this as a bug fix but
> rather as a behavior change that is not a bug fix.
> 
> Thanks,
> 
> Bart.


Hi Bart,

Whether it is necessary or not depends on how we define 'necessary.' 
If the criterion is simply to avoid errors, then indeed, this patch 
is not necessary. However, if we are addressing the warning caused 
by incorrect behavior (assigning int to u32), then it is necessary 
to fix it. After all, we shouldn't just be satisfied with avoiding 
errors, we should strive to make the Linux kernel as perfect as 
possible, shouldn't we?

Additionally, there are many ways to determine whether MCQ is enabled,
including reading the host capability or checking hba->mcq_enabled,
etc. 
Moreover, MCQ is not a feature that trun on and off at runtime.
It is at the end of the UFS initialization that the status of MCQ 
is determined, so it shouldn't be necessary to rely on this to 
determine whether MCQ is enabled, right?

Thanks
Peter

Re: [PATCH v1] ufs: core: fix hwq_id type and value

[Bart Van Assche]

On 5/6/25 9:03 PM, Peter Wang (王信友) wrote:
> Whether it is necessary or not depends on how we define 'necessary.'
> If the criterion is simply to avoid errors, then indeed, this patch
> is not necessary. However, if we are addressing the warning caused
> by incorrect behavior (assigning int to u32), then it is necessary
> to fix it. After all, we shouldn't just be satisfied with avoiding
> errors, we should strive to make the Linux kernel as perfect as
> possible, shouldn't we?

Errors? Which errors? Using -1 instead of UINT_MAX is common in C code.
Assigning variables of signed integer type to unsigned variables is also
widespread. Using %d to format a negative number, although dubious, is
also common in C code. Several years ago gcc warned about using %d to
format unsigned integers. That warning was disabled again because there
is too much existing code that follows this practice.

> Additionally, there are many ways to determine whether MCQ is enabled,
> including reading the host capability or checking hba->mcq_enabled,
> etc.
> Moreover, MCQ is not a feature that trun on and off at runtime.
> It is at the end of the UFS initialization that the status of MCQ
> is determined, so it shouldn't be necessary to rely on this to
> determine whether MCQ is enabled, right?

If you want to proceed with this patch, please make it clear in the
patch description that this patch is a behavior change and not a bug
fix.

Thanks,

Bart.

Re: [PATCH v1] ufs: core: fix hwq_id type and value

[Peter Wang (王信友)]

On Wed, 2025-05-07 at 12:26 -0700, Bart Van Assche wrote:
> 
> If you want to proceed with this patch, please make it clear in the
> patch description that this patch is a behavior change and not a bug
> fix.
> 
> Thanks,
> 
> Bart.

Hi Bart,

Okay, the commit message for the bug fix will be 
corrected in the next version.

Thanks
Peter