diff mbox series

[v1,2/3] scsi: ufs: Let command trace only for the cmd != null case

Message ID 20210523211409.210304-3-huobean@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series Three minor changes for UFS | expand

Commit Message

Bean Huo May 23, 2021, 9:14 p.m. UTC 
From: Bean Huo <beanhuo@micron.com>

For the query request, we already have query_trace, but in
ufshcd_send_command (), there will add two more redundant
traces. Since lrbp->cmd is null in the query request, the
below these two trace events provide nothing except the tag
and DB. Instead of letting them take up the limited trace
ring buffer, it’s better not to print these traces in case
of cmd == null.

ufshcd_command: send_req: ff3b0000.ufs: tag: 28, DB: 0x0, size: -1, IS: 0, LBA: 18446744073709551615, opcode: 0x0 (0x0), group_id: 0x0
ufshcd_command: dev_complete: ff3b0000.ufs: tag: 28, DB: 0x0, size: -1, IS: 0, LBA: 18446744073709551615, opcode: 0x0 (0x0), group_id: 0x0

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/scsi/ufs/ufshcd.c | 49 ++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 26 deletions(-)

Comments

Bart Van Assche May 24, 2021, 1:32 a.m. UTC | #1 
On 5/23/21 2:14 PM, Bean Huo wrote:
> +	opcode = cmd->cmnd[0];
> +	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;

Why does the lba assignment occur inside the if-statement for the
READ_10 and WRITE_10 cases? Has it been considered to move that
assignment before this if-statement?

Does 'lba' represent an offset in units of 512 bytes (sector_t) or an
LBA (logical block address)? In the former case, please rename the
variable 'lba' into 'sector' or 'start_sector'. In the latter case,
please use sectors_to_logical().

Why are READ_16 and WRITE_16 ignored?

Please remove the 'if (cmd->request)' checks since these are not necessary.

> +	} else if (opcode == UNMAP) {
> +		if (cmd->request) {
> +			lba = scsi_get_lba(cmd);
> +			transfer_len = blk_rq_bytes(cmd->request);
>  		}
>  	}

The name of the variable 'transfer_len' is wrong since blk_rq_bytes()
returns the number of bytes affected on the storage medium instead of
the number of bytes transferred from the host to the storage controller.

>  /**
> - * Describes the ufs rpmb wlun.
> - * Used only to send uac.
> + * Describes the ufs rpmb wlun. Used only to send uac.
>   */

Is this change related to the rest of this patch?

Thanks,

Bart.
Avri Altman May 24, 2021, 6:50 a.m. UTC | #2 
> 
> Why are READ_16 and WRITE_16 ignored?
For practical reasons - u32 is large enough to carry addresses for all foreseen devices.
So READ16 is practically not used in ufs, although per spec it is supported.
Bean Huo May 25, 2021, 8:02 p.m. UTC | #3 
Bart,
Thanks for your review, appreciated it.

On Sun, 2021-05-23 at 18:32 -0700, Bart Van Assche wrote:
> On 5/23/21 2:14 PM, Bean Huo wrote:
> > +	opcode = cmd->cmnd[0];
> > +	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;
> 
> Why does the lba assignment occur inside the if-statement for the
> READ_10 and WRITE_10 cases? Has it been considered to move that
> assignment before this if-statement?

yes, this lba assignment can be moved before if-statement:


      if (cmd->request && cmd->request->bio)
                        lba
= cmd->request->bio->bi_iter.bi_sector;

       if ((opcode == READ_10) || (opcode == WRITE_10)) {
                /*
                 * Currently we only fully trace read(10) and write(10)
                 * commands
                 */
          


> 
> Does 'lba' represent an offset in units of 512 bytes (sector_t) or an
> LBA (logical block address)? In the former case, please rename the
> variable 'lba' into 'sector' or 'start_sector'. In the latter case,
> please use sectors_to_logical().

apparently it is in 512 bytes. ok, sector is more readable.
> 
> Why are READ_16 and WRITE_16 ignored?

READ_16 and WRITE_16 are optimal for the UFS. not mandatory.
> 
> Please remove the 'if (cmd->request)' checks since these are not
> necessary.
> 
> > +	} else if (opcode == UNMAP) {
> > +		if (cmd->request) {
> > +			lba = scsi_get_lba(cmd);
> > +			transfer_len = blk_rq_bytes(cmd->request);
> >  		}
> >  	}
> 
> The name of the variable 'transfer_len' is wrong since blk_rq_bytes()
> returns the number of bytes affected on the storage medium instead of
> the number of bytes transferred from the host to the storage
> controller.
> 
ok, I will remove them, and they will be a additional cleanup patch.

> >  /**
> > - * Describes the ufs rpmb wlun.
> > - * Used only to send uac.
> > + * Describes the ufs rpmb wlun. Used only to send uac.
> >   */
> 
> Is this change related to the rest of this patch?
> 

It might be a cleanup.


Bean

> Thanks,
> 
> Bart.
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index dc5f13ccf176..ed9059b3e63d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -376,33 +376,31 @@  static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
 	struct scsi_cmnd *cmd = lrbp->cmd;
 	int transfer_len = -1;
 
-	if (!trace_ufshcd_command_enabled()) {
-		/* trace UPIU W/O tracing command */
-		if (cmd)
-			ufshcd_add_cmd_upiu_trace(hba, tag, str_t);
+	if (!cmd)
 		return;
-	}
 
-	if (cmd) { /* data phase exists */
-		/* trace UPIU also */
-		ufshcd_add_cmd_upiu_trace(hba, tag, str_t);
-		opcode = cmd->cmnd[0];
-		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(
+	/* trace UPIU W/O tracing command */
+	ufshcd_add_cmd_upiu_trace(hba, tag, str_t);
+
+	if (!trace_ufshcd_command_enabled())
+		return;
+
+	opcode = cmd->cmnd[0];
+	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);
-			}
+		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);
 		}
 	}
 
@@ -9774,8 +9772,7 @@  static const struct dev_pm_ops ufs_rpmb_pm_ops = {
 };
 
 /**
- * Describes the ufs rpmb wlun.
- * Used only to send uac.
+ * Describes the ufs rpmb wlun. Used only to send uac.
  */
 static struct scsi_driver ufs_rpmb_wlun_template = {
 	.gendrv = {