From: Bean Huo <[email protected]>
Changelog:
v1--v2:
1. Add a new cleanup patch 1/4
2. Make the patch 3/4 much readable by initializing a variable
'header'.
Bean Huo (4):
scsi: ufs: Cleanup ufshcd_add_command_trace()
scsi: ufs: Let UPIU completion trace print RSP UPIU header
scsi: ufs: Let command trace only for the cmd != null case
scsi: ufs: Use UPIU query trace in devman_upiu_cmd
drivers/scsi/ufs/ufshcd.c | 59 ++++++++++++++++++++++-----------------
1 file changed, 34 insertions(+), 25 deletions(-)
--
2.25.1
From: Bean Huo <[email protected]>
The current UPIU completion event trace still prints the COMMAND UPIU
header, rather than the RSP UPIU header. This makes UPIU command trace
useless in problem shooting in case we receive a trace log from the
customer/field.
There are two important fields in RSP UPIU:
1. The response field, which indicates the UFS defined overall success
or failure of the series of Command, Data and RESPONSE UPIU’s that
make up the execution of a task.
2. The Status field, which contains the command set specific status for
a specific command issued by the initiator device.
Before this patch, the UPIU paired trace events:
ufshcd_upiu: send_req: fe3b0000.ufs: HDR:01 20 00 1c 00 00 00 00 00 00 00 00, CDB:3b e1 00 00 00 00 00 00 30 00 00 00 00 00 00 00
ufshcd_upiu: complete_rsp: fe3b0000.ufs: HDR:01 20 00 1c 00 00 00 00 00 00 00 00, CDB:3b e1 00 00 00 00 00 00 30 00 00 00 00 00 00 00
After the patch:
ufshcd_upiu: send_req: fe3b0000.ufs: HDR:01 20 00 1c 00 00 00 00 00 00 00 00, CDB:3b e1 00 00 00 00 00 00 30 00 00 00 00 00 00 00
ufshcd_upiu: complete_rsp: fe3b0000.ufs: HDR:21 00 00 1c 00 00 00 00 00 00 00 00, CDB:3b e1 00 00 00 00 00 00 30 00 00 00 00 00 00 00
Signed-off-by: Bean Huo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 85590d3a719e..c5754d5486c9 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -302,11 +302,17 @@ static void ufshcd_add_cmd_upiu_trace(struct ufs_hba *hba, unsigned int tag,
enum ufs_trace_str_t str_t)
{
struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
+ struct utp_upiu_header *header;
if (!trace_ufshcd_upiu_enabled())
return;
- trace_ufshcd_upiu(dev_name(hba->dev), str_t, &rq->header, &rq->sc.cdb,
+ if (str_t == UFS_CMD_SEND)
+ header = &rq->header;
+ else
+ header = &hba->lrb[tag].ucd_rsp_ptr->header;
+
+ trace_ufshcd_upiu(dev_name(hba->dev), str_t, header, &rq->sc.cdb,
UFS_TSF_CDB);
}
--
2.25.1
From: Bean Huo <[email protected]>
Since devman_upiu_cmd is not COMMAND UPIU, and doesn't have
CDB, it is better to use UPIU query trace, which provides more
helpful information for issue shooting.
Signed-off-by: Bean Huo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c84bd8e045f6..deb9e232b349 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6701,6 +6701,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
hba->dev_cmd.complete = &wait;
+ ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);
/* Make sure descriptors are ready before ringing the doorbell */
wmb();
spin_lock_irqsave(hba->host->host_lock, flags);
@@ -6732,6 +6733,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
err = -EINVAL;
}
}
+ ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP,
+ (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
out:
blk_put_request(req);
--
2.25.1
From: Bean Huo <[email protected]>
To consistent with trace event print, convert the value of the
variable 'lba' in the unit of LBA (logical block address).
Suggested-by: Bart Van Assche <[email protected]>
Signed-off-by: Bean Huo <[email protected]>
---
drivers/scsi/ufs/ufshcd.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 02267b090729..85590d3a719e 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -25,6 +25,7 @@
#include "ufs_bsg.h"
#include "ufshcd-crypto.h"
#include <asm/unaligned.h>
+#include "../sd.h"
#define CREATE_TRACE_POINTS
#include <trace/events/ufs.h>
@@ -364,7 +365,7 @@ static void ufshcd_add_uic_command_trace(struct ufs_hba *hba,
static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
enum ufs_trace_str_t str_t)
{
- sector_t lba = -1;
+ u64 lba = -1;
u8 opcode = 0, group_id = 0;
u32 intr, doorbell;
struct ufshcd_lrb *lrbp = &hba->lrb[tag];
@@ -382,22 +383,23 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
/* trace UPIU also */
ufshcd_add_cmd_upiu_trace(hba, tag, str_t);
opcode = cmd->cmnd[0];
+ lba = sectors_to_logical(cmd->device, blk_rq_pos(cmd->request));
+
if ((opcode == READ_10) || (opcode == WRITE_10)) {
/*
* Currently we only fully trace read(10) and write(10)
* commands
*/
- if (cmd->request && cmd->request->bio)
- lba = cmd->request->bio->bi_iter.bi_sector;
transfer_len = be32_to_cpu(
lrbp->ucd_req_ptr->sc.exp_data_transfer_len);
if (opcode == WRITE_10)
group_id = lrbp->cmd->cmnd[6];
} else if (opcode == UNMAP) {
- if (cmd->request) {
- lba = scsi_get_lba(cmd);
- transfer_len = blk_rq_bytes(cmd->request);
- }
+ /*
+ * The number of Bytes to be unmapped beginning with the
+ * lba.
+ */
+ transfer_len = blk_rq_bytes(cmd->request);
}
}
--
2.25.1
Hi Bean,
On 2021-05-31 18:43, Bean Huo wrote:
> From: Bean Huo <[email protected]>
>
> Since devman_upiu_cmd is not COMMAND UPIU, and doesn't have
> CDB, it is better to use UPIU query trace, which provides more
> helpful information for issue shooting.
>
> Signed-off-by: Bean Huo <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c84bd8e045f6..deb9e232b349 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -6701,6 +6701,7 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
>
> hba->dev_cmd.complete = &wait;
>
> + ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);
> /* Make sure descriptors are ready before ringing the doorbell */
> wmb();
> spin_lock_irqsave(hba->host->host_lock, flags);
> @@ -6732,6 +6733,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
> err = -EINVAL;
> }
> }
> + ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR :
> UFS_QUERY_COMP,
> + (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
>
> out:
> blk_put_request(req);
What about ufshcd_exec_dev_cmd()?
Thanks,
Can Guo.
On 2021-05-31 18:43, Bean Huo wrote:
> From: Bean Huo <[email protected]>
>
> The current UPIU completion event trace still prints the COMMAND UPIU
> header, rather than the RSP UPIU header. This makes UPIU command trace
> useless in problem shooting in case we receive a trace log from the
> customer/field.
>
> There are two important fields in RSP UPIU:
> 1. The response field, which indicates the UFS defined overall success
> or failure of the series of Command, Data and RESPONSE UPIU’s that
> make up the execution of a task.
> 2. The Status field, which contains the command set specific status
> for
> a specific command issued by the initiator device.
>
> Before this patch, the UPIU paired trace events:
>
> ufshcd_upiu: send_req: fe3b0000.ufs: HDR:01 20 00 1c 00 00 00 00 00 00
> 00 00, CDB:3b e1 00 00 00 00 00 00 30 00 00 00 00 00 00 00
> ufshcd_upiu: complete_rsp: fe3b0000.ufs: HDR:01 20 00 1c 00 00 00 00
> 00 00 00 00, CDB:3b e1 00 00 00 00 00 00 30 00 00 00 00 00 00 00
>
> After the patch:
>
> ufshcd_upiu: send_req: fe3b0000.ufs: HDR:01 20 00 1c 00 00 00 00 00 00
> 00 00, CDB:3b e1 00 00 00 00 00 00 30 00 00 00 00 00 00 00
> ufshcd_upiu: complete_rsp: fe3b0000.ufs: HDR:21 00 00 1c 00 00 00 00
> 00 00 00 00, CDB:3b e1 00 00 00 00 00 00 30 00 00 00 00 00 00 00
>
> Signed-off-by: Bean Huo <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 85590d3a719e..c5754d5486c9 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -302,11 +302,17 @@ static void ufshcd_add_cmd_upiu_trace(struct
> ufs_hba *hba, unsigned int tag,
> enum ufs_trace_str_t str_t)
> {
> struct utp_upiu_req *rq = hba->lrb[tag].ucd_req_ptr;
> + struct utp_upiu_header *header;
>
> if (!trace_ufshcd_upiu_enabled())
> return;
>
> - trace_ufshcd_upiu(dev_name(hba->dev), str_t, &rq->header,
> &rq->sc.cdb,
> + if (str_t == UFS_CMD_SEND)
> + header = &rq->header;
> + else
> + header = &hba->lrb[tag].ucd_rsp_ptr->header;
> +
> + trace_ufshcd_upiu(dev_name(hba->dev), str_t, header, &rq->sc.cdb,
> UFS_TSF_CDB);
> }
Reviewed-by: Can Guo <[email protected]>
On 2021-05-31 18:43, Bean Huo wrote:
> From: Bean Huo <[email protected]>
>
> To consistent with trace event print, convert the value of the
> variable 'lba' in the unit of LBA (logical block address).
>
> Suggested-by: Bart Van Assche <[email protected]>
> Signed-off-by: Bean Huo <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 02267b090729..85590d3a719e 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -25,6 +25,7 @@
> #include "ufs_bsg.h"
> #include "ufshcd-crypto.h"
> #include <asm/unaligned.h>
> +#include "../sd.h"
>
> #define CREATE_TRACE_POINTS
> #include <trace/events/ufs.h>
> @@ -364,7 +365,7 @@ static void ufshcd_add_uic_command_trace(struct
> ufs_hba *hba,
> static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int
> tag,
> enum ufs_trace_str_t str_t)
> {
> - sector_t lba = -1;
> + u64 lba = -1;
> u8 opcode = 0, group_id = 0;
> u32 intr, doorbell;
> struct ufshcd_lrb *lrbp = &hba->lrb[tag];
> @@ -382,22 +383,23 @@ static void ufshcd_add_command_trace(struct
> ufs_hba *hba, unsigned int tag,
> /* trace UPIU also */
> ufshcd_add_cmd_upiu_trace(hba, tag, str_t);
> opcode = cmd->cmnd[0];
> + lba = sectors_to_logical(cmd->device, blk_rq_pos(cmd->request));
> +
> if ((opcode == READ_10) || (opcode == WRITE_10)) {
> /*
> * Currently we only fully trace read(10) and write(10)
> * commands
> */
> - if (cmd->request && cmd->request->bio)
> - lba = cmd->request->bio->bi_iter.bi_sector;
> transfer_len = be32_to_cpu(
> lrbp->ucd_req_ptr->sc.exp_data_transfer_len);
> if (opcode == WRITE_10)
> group_id = lrbp->cmd->cmnd[6];
> } else if (opcode == UNMAP) {
> - if (cmd->request) {
> - lba = scsi_get_lba(cmd);
> - transfer_len = blk_rq_bytes(cmd->request);
> - }
> + /*
> + * The number of Bytes to be unmapped beginning with the
> + * lba.
> + */
> + transfer_len = blk_rq_bytes(cmd->request);
> }
> }
Reviewed-by: Can Guo <[email protected]>
On Wed, 2021-06-02 at 10:29 +0800, Can Guo wrote:
> > spin_lock_irqsave(hba->host->host_lock, flags);
> > @@ -6732,6 +6733,8 @@ static int
> > ufshcd_issue_devman_upiu_cmd(struct
> > ufs_hba *hba,
> > err = -EINVAL;
> > }
> > }
> > + ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR :
> > UFS_QUERY_COMP,
> > + (struct utp_upiu_req *)lrbp-
> > >ucd_rsp_ptr);
> > out:
> > blk_put_request(req);
>
>
> What about ufshcd_exec_dev_cmd()?
>
Can,
thanks for your veiew.
yes, ufshcd_exec_dev_cmd() is only to send
UPIU command frame, and doesn't include CDB. It already uses
ufshcd_add_query_upiu_trace() to trace UPIU frame.
Kind regards,
Bean
> Thanks,
>
> Can Guo.
On 2021-06-03 05:05, Bean Huo wrote:
> On Wed, 2021-06-02 at 10:29 +0800, Can Guo wrote:
>> > spin_lock_irqsave(hba->host->host_lock, flags);
>> > @@ -6732,6 +6733,8 @@ static int
>> > ufshcd_issue_devman_upiu_cmd(struct
>> > ufs_hba *hba,
>> > err = -EINVAL;
>> > }
>> > }
>> > + ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR :
>> > UFS_QUERY_COMP,
>> > + (struct utp_upiu_req *)lrbp-
>> > >ucd_rsp_ptr);
>> > out:
>> > blk_put_request(req);
>>
>>
>> What about ufshcd_exec_dev_cmd()?
>>
>
>
> Can,
> thanks for your veiew.
> yes, ufshcd_exec_dev_cmd() is only to send
> UPIU command frame, and doesn't include CDB. It already uses
> ufshcd_add_query_upiu_trace() to trace UPIU frame.
>
Oh, sorry, I missed it.
Reviewed-by: Can Guo <[email protected]>
> Kind regards,
> Bean
>
>> Thanks,
>>
>> Can Guo.
Bean,
> 1. Add a new cleanup patch 1/4 2. Make the patch 3/4 much readable
> by initializing a variable 'header'.
Applied to 5.14/scsi-staging, thanks!
--
Martin K. Petersen Oracle Linux Engineering
On Mon, 31 May 2021 12:43:04 +0200, Bean Huo wrote:
> Changelog:
> v1--v2:
> 1. Add a new cleanup patch 1/4
> 2. Make the patch 3/4 much readable by initializing a variable
> 'header'.
>
>
> [...]
Applied to 5.14/scsi-queue, thanks!
[1/4] scsi: ufs: Cleanup ufshcd_add_command_trace()
https://git.kernel.org/mkp/scsi/c/04c073feb1d7
[2/4] scsi: ufs: Let UPIU completion trace print RSP UPIU header
https://git.kernel.org/mkp/scsi/c/89ac2c3b2835
[3/4] scsi: ufs: Let command trace only for the cmd != null case
https://git.kernel.org/mkp/scsi/c/44b5de363524
[4/4] scsi: ufs: Use UPIU query trace in devman_upiu_cmd
https://git.kernel.org/mkp/scsi/c/105424895c02
--
Martin K. Petersen Oracle Linux Engineering