[PATCH 0/2] scsi: ufs: Add an enum for ufs_trace_str_t to check uic cmd error

[PATCH 0/2] scsi: ufs: Add an enum for ufs_trace_str_t to check uic cmd error

[DooHyun Hwang]

There is no trace when a ufs uic command error occurs.

So, add "UFS_CMD_ERR" enum to ufs_trace_str_t and add trace function calls when a uic
command error happens.

DooHyun Hwang (2):
  scsi: ufs: Add an enum for ufs_trace to check ufs cmd error
  scsi: ufs: core: Add a trace function calling when uic command error
    occurs

 drivers/ufs/core/ufs_trace.h | 1 +
 drivers/ufs/core/ufshcd.c    | 5 +++++
 include/ufs/ufs.h            | 2 +-
 3 files changed, 7 insertions(+), 1 deletion(-)

-- 
2.48.1

[PATCH 1/2] scsi: ufs: Add an enum for ufs_trace to check ufs cmd error

[DooHyun Hwang]

There is no trace when a ufs uic cmd error occurs.
So, add "UFS_CMD_ERR" enumeration to ufs_trace_str_t.

Signed-off-by: DooHyun Hwang <dh0421.hwang@samsung.com>
---
 drivers/ufs/core/ufs_trace.h | 1 +
 include/ufs/ufs.h            | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufs_trace.h b/drivers/ufs/core/ufs_trace.h
index caa32e23ffa5..43830a092637 100644
--- a/drivers/ufs/core/ufs_trace.h
+++ b/drivers/ufs/core/ufs_trace.h
@@ -41,6 +41,7 @@
 #define UFS_CMD_TRACE_STRINGS					\
 	EM(UFS_CMD_SEND,	"send_req")			\
 	EM(UFS_CMD_COMP,	"complete_rsp")			\
+	EM(UFS_CMD_ERR,		"req_complete_err")		\
 	EM(UFS_DEV_COMP,	"dev_complete")			\
 	EM(UFS_QUERY_SEND,	"query_send")			\
 	EM(UFS_QUERY_COMP,	"query_complete")		\
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index c0c59a8f7256..7f2d418bdd86 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -631,7 +631,7 @@ struct ufs_dev_info {
  * This enum is used in string mapping in ufs_trace.h.
  */
 enum ufs_trace_str_t {
-	UFS_CMD_SEND, UFS_CMD_COMP, UFS_DEV_COMP,
+	UFS_CMD_SEND, UFS_CMD_COMP, UFS_CMD_ERR, UFS_DEV_COMP,
 	UFS_QUERY_SEND, UFS_QUERY_COMP, UFS_QUERY_ERR,
 	UFS_TM_SEND, UFS_TM_COMP, UFS_TM_ERR
 };
-- 
2.48.1

RE: [PATCH 1/2] scsi: ufs: Add an enum for ufs_trace to check ufs cmd error

[Avri Altman]

> There is no trace when a ufs uic cmd error occurs.
> So, add "UFS_CMD_ERR" enumeration to ufs_trace_str_t.
> 
> Signed-off-by: DooHyun Hwang <dh0421.hwang@samsung.com>
> ---
>  drivers/ufs/core/ufs_trace.h | 1 +
>  include/ufs/ufs.h            | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufs_trace.h b/drivers/ufs/core/ufs_trace.h index
> caa32e23ffa5..43830a092637 100644
> --- a/drivers/ufs/core/ufs_trace.h
> +++ b/drivers/ufs/core/ufs_trace.h
> @@ -41,6 +41,7 @@
>  #define UFS_CMD_TRACE_STRINGS                                  \
>         EM(UFS_CMD_SEND,        "send_req")                     \
>         EM(UFS_CMD_COMP,        "complete_rsp")                 \
> +       EM(UFS_CMD_ERR,         "req_complete_err")             \
>         EM(UFS_DEV_COMP,        "dev_complete")                 \
>         EM(UFS_QUERY_SEND,      "query_send")                   \
>         EM(UFS_QUERY_COMP,      "query_complete")               \
> diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h index
> c0c59a8f7256..7f2d418bdd86 100644
> --- a/include/ufs/ufs.h
> +++ b/include/ufs/ufs.h
> @@ -631,7 +631,7 @@ struct ufs_dev_info {
>   * This enum is used in string mapping in ufs_trace.h.
>   */
>  enum ufs_trace_str_t {
> -       UFS_CMD_SEND, UFS_CMD_COMP, UFS_DEV_COMP,
> +       UFS_CMD_SEND, UFS_CMD_COMP, UFS_CMD_ERR, UFS_DEV_COMP,
>         UFS_QUERY_SEND, UFS_QUERY_COMP, UFS_QUERY_ERR,
>         UFS_TM_SEND, UFS_TM_COMP, UFS_TM_ERR  };
It seems strange to me that scsi & uic commands are designated by the same enum.
Has it been considered to add UFS_UIC_SEND, UFS_UIC_COMP, UFS_UIC_ERR to enum ufs_trace_str_t ?
Also looks like UFS_DEV_COMP is unused ?

Thanks,
Avri


> --
> 2.48.1

RE: [PATCH 1/2] scsi: ufs: Add an enum for ufs_trace to check ufs cmd error

[DooHyun Hwang]

> > There is no trace when a ufs uic cmd error occurs.
> > So, add "UFS_CMD_ERR" enumeration to ufs_trace_str_t.
> >
> > Signed-off-by: DooHyun Hwang <dh0421.hwang@samsung.com>
> > ---
> >  drivers/ufs/core/ufs_trace.h | 1 +
> >  include/ufs/ufs.h            | 2 +-
> >  2 files changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/ufs/core/ufs_trace.h b/drivers/ufs/core/ufs_trace.h
> index
> > caa32e23ffa5..43830a092637 100644
> > --- a/drivers/ufs/core/ufs_trace.h
> > +++ b/drivers/ufs/core/ufs_trace.h
> > @@ -41,6 +41,7 @@
> >  #define UFS_CMD_TRACE_STRINGS                                  \
> >         EM(UFS_CMD_SEND,        "send_req")                     \
> >         EM(UFS_CMD_COMP,        "complete_rsp")                 \
> > +       EM(UFS_CMD_ERR,         "req_complete_err")             \
> >         EM(UFS_DEV_COMP,        "dev_complete")                 \
> >         EM(UFS_QUERY_SEND,      "query_send")                   \
> >         EM(UFS_QUERY_COMP,      "query_complete")               \
> > diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h index
> > c0c59a8f7256..7f2d418bdd86 100644
> > --- a/include/ufs/ufs.h
> > +++ b/include/ufs/ufs.h
> > @@ -631,7 +631,7 @@ struct ufs_dev_info {
> >   * This enum is used in string mapping in ufs_trace.h.
> >   */
> >  enum ufs_trace_str_t {
> > -       UFS_CMD_SEND, UFS_CMD_COMP, UFS_DEV_COMP,
> > +       UFS_CMD_SEND, UFS_CMD_COMP, UFS_CMD_ERR, UFS_DEV_COMP,
> >         UFS_QUERY_SEND, UFS_QUERY_COMP, UFS_QUERY_ERR,
> >         UFS_TM_SEND, UFS_TM_COMP, UFS_TM_ERR  };
> It seems strange to me that scsi & uic commands are designated by the same
> enum.
> Has it been considered to add UFS_UIC_SEND, UFS_UIC_COMP, UFS_UIC_ERR to
> enum ufs_trace_str_t ?
> Also looks like UFS_DEV_COMP is unused ?
> 
> Thanks,
> Avri
It is correct that the same enumeration is used for both SCSI and UIC
commands.
However, the trace function differs for SCSI and UIC commands.
The enum is solely for handling the sending and completion of each command
within the respective trace functions.

Therefore, I think there is no need to add new enums like "UFS_UIC_SEND".

It seems better to discuss "UFS_DEV_COMP" in another commit.


Thank you.
DooHyun Hwang.
> > --
> > 2.48.1
> 

Re: [PATCH 1/2] scsi: ufs: Add an enum for ufs_trace to check ufs cmd error

[Bart Van Assche]

On 4/16/25 7:34 PM, DooHyun Hwang wrote:
> +	EM(UFS_CMD_ERR,		"req_complete_err")		\

Does UFS_CMD_ERR stand for "command error" or "completion error"? Please
make the enum label and the text that is displayed in error messages
consistent.

Bart.

RE: [PATCH 1/2] scsi: ufs: Add an enum for ufs_trace to check ufs cmd error

[DooHyun Hwang]

On 8/29/25 8:15 PM, Bart Van Assche wrote:
> On 4/16/25 7:34 PM, DooHyun Hwang wrote:
> > +	EM(UFS_CMD_ERR,		"req_complete_err")		\
> 
> Does UFS_CMD_ERR stand for "command error" or "completion error"? Please
> make the enum label and the text that is displayed in error messages
> consistent.
> 
> Bart.

UFS_CMD_ERR stands for "completion error".
The enum is converted to a string in the trace log according
to the definition below.

in include/trace/events/ufs.h
#define UFS_CMD_TRACE_STRINGS					\
	EM(UFS_CMD_SEND,	"send_req")			\
	EM(UFS_CMD_COMP,	"complete_rsp")			\
+	EM(UFS_CMD_ERR,		"req_complete_err")		\
	EM(UFS_DEV_COMP,	"dev_complete")			\
	EM(UFS_QUERY_SEND,	"query_send")			\
	EM(UFS_QUERY_COMP,	"query_complete")		\
	EM(UFS_QUERY_ERR,	"query_complete_err")		\
	EM(UFS_TM_SEND,		"tm_send")			\
	EM(UFS_TM_COMP,		"tm_complete")			\
	EMe(UFS_TM_ERR,		"tm_complete_err")

Thank you.
DooHyun Hwang.

Re: [PATCH 1/2] scsi: ufs: Add an enum for ufs_trace to check ufs cmd error

[Bart Van Assche]

On 8/31/25 6:31 PM, DooHyun Hwang wrote:
> UFS_CMD_ERR stands for "completion error".

No it doesn't. In the UFS driver and also in many other driver "cmd" 
stands for "command" and not for "completion". Please change the
"UFS_CMD_ERR" enum label.

Bart.

RE: [PATCH 1/2] scsi: ufs: Add an enum for ufs_trace to check ufs cmd error

[DooHyun Hwang]

On 9/1/25 7:4 AM, Bart Van Assche wrote:
> On 8/31/25 6:31 PM, DooHyun Hwang wrote:
> > UFS_CMD_ERR stands for "completion error".
> 
> No it doesn't. In the UFS driver and also in many other driver "cmd"
> stands for "command" and not for "completion". Please change the
> "UFS_CMD_ERR" enum label.
> 
> Bart.

I'm sorry. There was a misunderstanding in my previous comment.

The UFS_CMD_ERR stands for "command completion error."
I wanted to make it consistent with the other enums.
For example, query errors are defined as UFS_QUERY_ERR,
and Task Management errors are defined as UFS_TM_ERR.

I hope to use UFS_CMD_ERR for the same classification purposes
as UFS_CMD_SEND or UFS_CMD_COMP.
Therefore, UFS_CMD_ERR can be used for logging SCSI or UIC commands.

Thank you.
DooHyun Hwang.

Re: [PATCH 1/2] scsi: ufs: Add an enum for ufs_trace to check ufs cmd error

[Bart Van Assche]

On 9/1/25 6:09 PM, DooHyun Hwang wrote:
> The UFS_CMD_ERR stands for "command completion error."

I've never before seen anyone abbreviating "completion" as "CMD".
Please choose a better enumeration label name.

Bart.

RE: [PATCH 1/2] scsi: ufs: Add an enum for ufs_trace to check ufs cmd error

[DooHyun Hwang]

On 9/2/25 9:35 AM, Bart Van Assche wrote:
> On 9/1/25 6:09 PM, DooHyun Hwang wrote:
> > The UFS_CMD_ERR stands for "command completion error."
> 
> I've never before seen anyone abbreviating "completion" as "CMD".
> Please choose a better enumeration label name.
> 
> Bart.

When I reviewed the UFS_CMD_TRACE_STRINGS definition,
I noticed that "query_complete_err" is referred to as UFS_QUERY_ERR,
and "tm_complete_err" is referred to as UFS_TM_ERR.

I followed the naming convention specified in this STRINGS definition,
but I’m not entirely clear on what changes are being requested.

Could you please provide some guidance or suggestions?

Thank you.
DooHyun Hwang.

Re: [PATCH 1/2] scsi: ufs: Add an enum for ufs_trace to check ufs cmd error

[Bart Van Assche]

On 9/2/25 11:39 PM, DooHyun Hwang wrote:
> Could you please provide some guidance or suggestions?

How about this approach?
* In a separate patch, remove UFS_DEV_COMP because it is not used.
* Call the new enumeration label UFS_UIC_ERR (UFS UIC command error)
   since "UIC" is how the UFS standard refers to UIC commands (these are
   called "device commands" in the UFS driver).

If anyone else has a different opinion, please speak up.

Thanks,

Bart.

RE: [PATCH 1/2] scsi: ufs: Add an enum for ufs_trace to check ufs cmd error

[DooHyun Hwang]

On 9/3/25 8:57 AM, Bart Van Assche wrote:
> On 9/2/25 11:39 PM, DooHyun Hwang wrote:
> > Could you please provide some guidance or suggestions?
> 
> How about this approach?
> * In a separate patch, remove UFS_DEV_COMP because it is not used.
> * Call the new enumeration label UFS_UIC_ERR (UFS UIC command error)
>    since "UIC" is how the UFS standard refers to UIC commands (these are
>    called "device commands" in the UFS driver).
> 
> If anyone else has a different opinion, please speak up.
> 
> Thanks,
> 
> Bart.

Thank you for your review. Your opinion is truly valuable to me.

Based on your suggestion, I understand that the change involves
renaming UFS_DEV_COMP to UFS_UIC_ERR.
Would it be acceptable to add UFS_UIC_SEND, UFS_UIC_COMP,
and UFS_UIC_ERR at the end of the enum?

I am aware that UFS_DEV_COMP is currently not used in the code,
but I am concerned about whether it is appropriate to modify
or remove existing enum labels.
Therefore, I thought it would be better to discuss the removal
of UFS_DEV_COMP in a separate commit.
Additionally, I considered adding new enums as needed.

I decided to name the new enum UFS_CMD_ERR and add it
in the same type's position, which led to a change in the enum order.
I realized this could also potentially alter existing enum values,
creating room for controversy.
Furthermore, I acknowledged that the label UFS_CMD_ERR may not accurately
represent the purpose of tracing UIC errors.

Thank you.
DooHyun Hwang.

Re: [PATCH 1/2] scsi: ufs: Add an enum for ufs_trace to check ufs cmd error

[Bart Van Assche]

On 9/8/25 12:43 AM, DooHyun Hwang wrote:
> Would it be acceptable to add UFS_UIC_SEND, UFS_UIC_COMP,
> and UFS_UIC_ERR at the end of the enum?
> 
> I am aware that UFS_DEV_COMP is currently not used in the code,
> but I am concerned about whether it is appropriate to modify
> or remove existing enum labels.

Please take a look at Documentation/process/stable-api-nonsense.rst.
While the Android GKI kernels have a stable ABI, the upstream kernel
does not promise a stable ABI. Hence, the concerns that apply to GKI
kernels regarding changing the numerical value of enum labels do not
apply to the upstream kernel. It is the responsibility of anyone who
backports an upstream patch to a GKI kernel branch to make sure that
the GKI ABI is not modified.

Thanks,

Bart.

[PATCH 2/2] scsi: ufs: core: Add a trace function calling when uic command error occurs

[DooHyun Hwang]

When a uic command error occurs, there is no trace function calling.
Therefore, trace function calls are added when a uic command error happens.

Signed-off-by: DooHyun Hwang <dh0421.hwang@samsung.com>
---
 drivers/ufs/core/ufshcd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index ab98acd982b5..baac1ae94efc 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2534,6 +2534,9 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
 	hba->active_uic_cmd = NULL;
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
+	if (ret)
+		ufshcd_add_uic_command_trace(hba, uic_cmd, UFS_CMD_ERR);
+
 	return ret;
 }
 
@@ -4306,6 +4309,8 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
 	}
 out:
 	if (ret) {
+		ufshcd_add_uic_command_trace(hba, hba->active_uic_cmd,
+					     UFS_CMD_ERR);
 		ufshcd_print_host_state(hba);
 		ufshcd_print_pwr_info(hba);
 		ufshcd_print_evt_hist(hba);
-- 
2.48.1

Re: [PATCH 2/2] scsi: ufs: core: Add a trace function calling when uic command error occurs

[Bart Van Assche]

On 4/16/25 7:34 PM, DooHyun Hwang wrote:
> When a uic command error occurs, there is no trace function calling.
> Therefore, trace function calls are added when a uic command error happens.
> 
> Signed-off-by: DooHyun Hwang <dh0421.hwang@samsung.com>
> ---
>   drivers/ufs/core/ufshcd.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ab98acd982b5..baac1ae94efc 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2534,6 +2534,9 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba, struct uic_command *uic_cmd)
>   	hba->active_uic_cmd = NULL;
>   	spin_unlock_irqrestore(hba->host->host_lock, flags);
>   
> +	if (ret)
> +		ufshcd_add_uic_command_trace(hba, uic_cmd, UFS_CMD_ERR);
> +
>   	return ret;
>   }
>   
> @@ -4306,6 +4309,8 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd)
>   	}
>   out:
>   	if (ret) {
> +		ufshcd_add_uic_command_trace(hba, hba->active_uic_cmd,
> +					     UFS_CMD_ERR);
>   		ufshcd_print_host_state(hba);
>   		ufshcd_print_pwr_info(hba);
>   		ufshcd_print_evt_hist(hba);

Shouldn't the value of 'ret' be included in the UFS_CMD_ERR trace output?

Thanks,

Bart.

RE: [PATCH 2/2] scsi: ufs: core: Add a trace function calling when uic command error occurs

[DooHyun Hwang]

> On 4/16/25 7:34 PM, DooHyun Hwang wrote:
> > When a uic command error occurs, there is no trace function calling.
> > Therefore, trace function calls are added when a uic command error
> happens.
> >
> > Signed-off-by: DooHyun Hwang <dh0421.hwang@samsung.com>
> > ---
> >   drivers/ufs/core/ufshcd.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index ab98acd982b5..baac1ae94efc 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -2534,6 +2534,9 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba,
> struct uic_command *uic_cmd)
> >   	hba->active_uic_cmd = NULL;
> >   	spin_unlock_irqrestore(hba->host->host_lock, flags);
> >
> > +	if (ret)
> > +		ufshcd_add_uic_command_trace(hba, uic_cmd, UFS_CMD_ERR);
> > +
> >   	return ret;
> >   }
> >
> > @@ -4306,6 +4309,8 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba,
> struct uic_command *cmd)
> >   	}
> >   out:
> >   	if (ret) {
> > +		ufshcd_add_uic_command_trace(hba, hba->active_uic_cmd,
> > +					     UFS_CMD_ERR);
> >   		ufshcd_print_host_state(hba);
> >   		ufshcd_print_pwr_info(hba);
> >   		ufshcd_print_evt_hist(hba);
> 
> Shouldn't the value of 'ret' be included in the UFS_CMD_ERR trace output?
> 
> Thanks,
> 
> Bart.
Thank you for your insightful feedback.

Upon further consideration, it seems that the existing trace, which captures detailed information through the UICCMDARG1 to UICCMDARG3 fields.
That already provides sufficient context for analyzing command-related issues.
The UFS_CMD_ERR trace output, combined with UICCMDARG* values, should be adequate for diagnosing errors and understanding the command's execution status.

BR,
Thank you.
DooHyun Hwang.

Re: [PATCH 2/2] scsi: ufs: core: Add a trace function calling when uic command error occurs

[Peter Wang (王信友)]

On Thu, 2025-04-17 at 11:34 +0900, DooHyun Hwang wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> When a uic command error occurs, there is no trace function calling.
> Therefore, trace function calls are added when a uic command error
> happens.
> 
> Signed-off-by: DooHyun Hwang <dh0421.hwang@samsung.com>
> ---
>  drivers/ufs/core/ufshcd.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ab98acd982b5..baac1ae94efc 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2534,6 +2534,9 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba,
> struct uic_command *uic_cmd)
>         hba->active_uic_cmd = NULL;
>         spin_unlock_irqrestore(hba->host->host_lock, flags);
> 
> +       if (ret)
> +               ufshcd_add_uic_command_trace(hba, uic_cmd,
> UFS_CMD_ERR);
> +
>         return ret;
>  }
> 
> @@ -4306,6 +4309,8 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba
> *hba, struct uic_command *cmd)
>         }
>  out:
>         if (ret) {
> +               ufshcd_add_uic_command_trace(hba, hba-
> >active_uic_cmd,
> +                                            UFS_CMD_ERR);
>                 ufshcd_print_host_state(hba);
>                 ufshcd_print_pwr_info(hba);
>                 ufshcd_print_evt_hist(hba);
> --
> 2.48.1
> 


Hi DooHyun,

Is it possible to receive UFS_CMD_COMP and UFS_CMD_ERR at the same
time? 
Wouldn't it be a bit strange for a command to receive two completions? 
Additionally, should we also add tracing for general SCSI commands 
that receive errors?

Thanks
Peter

RE: [PATCH 2/2] scsi: ufs: core: Add a trace function calling when uic command error occurs

[DooHyun Hwang]

> On Thu, 2025-04-17 at 11:34 +0900, DooHyun Hwang wrote:
> >
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >
> >
> > When a uic command error occurs, there is no trace function calling.
> > Therefore, trace function calls are added when a uic command error
> > happens.
> >
> > Signed-off-by: DooHyun Hwang <dh0421.hwang@samsung.com>
> > ---
> >  drivers/ufs/core/ufshcd.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index ab98acd982b5..baac1ae94efc 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -2534,6 +2534,9 @@ ufshcd_wait_for_uic_cmd(struct ufs_hba *hba,
> > struct uic_command *uic_cmd)
> >         hba->active_uic_cmd = NULL;
> >         spin_unlock_irqrestore(hba->host->host_lock, flags);
> >
> > +       if (ret)
> > +               ufshcd_add_uic_command_trace(hba, uic_cmd,
> > UFS_CMD_ERR);
> > +
> >         return ret;
> >  }
> >
> > @@ -4306,6 +4309,8 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba
> > *hba, struct uic_command *cmd)
> >         }
> >  out:
> >         if (ret) {
> > +               ufshcd_add_uic_command_trace(hba, hba-
> > >active_uic_cmd,
> > +                                            UFS_CMD_ERR);
> >                 ufshcd_print_host_state(hba);
> >                 ufshcd_print_pwr_info(hba);
> >                 ufshcd_print_evt_hist(hba);
> > --
> > 2.48.1
> >
> 
> 
> Hi DooHyun,
> 
> Is it possible to receive UFS_CMD_COMP and UFS_CMD_ERR at the same time?
> Wouldn't it be a bit strange for a command to receive two completions?
> Additionally, should we also add tracing for general SCSI commands that
> receive errors?
> 
> Thanks
> Peter
> 
Thank you for your review.

Yes, that’s correct. There is a case where both UFS_CMD_COMP and UFS_CMD_ERR are logged simultaneously.

In the typical case of a UIC command timeout, only UFS_CMD_ERR would be recorded.

However, the scenario you described earlier, where both UFS_CMD_COMP and UFS_CMD_ERR are received in the same UIC command, is distinct from a timeout situation.
In this case, the command completes in time, and UFS_CMD_COMP is received when handling the UIC command completion interrupt in ufshcd_uic_cmd_compl().
However, the UPMCRS status indicates an error because PWR_LOCAL is not in its expected state (when checking upmcrs in the "check_upmcrs:" section of the ufshcd_uic_pwr_ctrl() function).

UFS_CMD_COMP is generated when the system recognizes through interrupt status that the command has completed its execution, while UFS_CMD_ERR is recorded when an error associated with the command is detected.

While it may be cumbersome, it is possible to trace the timeout case and this case separately.

Regarding tracing general SCSI command errors directly through UFS_CMD_ERR, it might be challenging because SCSI command errors are typically handled through various mechanisms such as timeout detection, scsi_status value checks, sense error checks, and OCS (Overall Command Status) value verification.
Additionally, tracing SCSI command errors does not align with the purpose of this commit.

BR,
Thank you.
DooHyun Hwang