Messages in this thread |  | | Subject | Re: [PATCH v1 2/3] scsi: ufs: Let command trace only for the cmd != null case | From | Bart Van Assche <> | Date | Sun, 23 May 2021 18:32:08 -0700 |
| |
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.
|  |