> > +#define EN_PRINT_UFS_LOG 1
>
> Since this macro controls debug code, please make this configurable at
> runtime, e.g. as a kernel module parameter or by using the dynamic debug
> mechanism.
Got it.
>
> > +/* Structure for ufs cmd logging */
> > +#define MAX_CMD_LOGS 32
>
> Please clarify in the comment above this constant how this constant has
> been chosen. Is this constant e.g. related to the queue depth? Is this
> constant per UFS device or is it a maximum for all UFS devices?
>
> > +struct cmd_data {
> > + unsigned int tag;
> > + unsigned int sct;
> > + unsigned long lba;
> > + u64 start_time;
> > + u64 end_time;
> > + u64 outstanding_reqs;
> > + int retries;
> > + unsigned char op;
> > +};
>
> Please use data types that explicitly specify the width for members like
> e.g. the lba (u64 instead of unsigned long). Please also use u8 instead of
> unsigned char for 'op' since 'op' is not used to store any kind of ASCII
> character.
Got it.
>
> > +struct ufs_cmd_info {
> > + u32 total;
> > + u32 last;
> > + struct cmd_data data[MAX_CMD_LOGS];
> > + struct cmd_data *pdata[32]; /* Currently, 32 slots */
> > +};
>
> What are 'slots'? Why 32?
I meant tag number and assumed that CAP.NUTRS be 32.
With 32, you can see all the command context with full outstanding situations.
Of course, there is a possibility that the value increases in the next UFS versions.
So, to run automatically, yes, I should have made this get CAP.NUTRS.
However, to do that, I have to add another call after enabling host.
>
> > +#define DBG_NUM_OF_HOSTS 1
> > +struct ufs_s_dbg_mgr {
> > + struct ufs_exynos_handle *handle;
> > + int active;
> > + u64 first_time;
> > + u64 time;
> > +
> > + /* cmd log */
> > + struct ufs_cmd_info cmd_info;
> > + struct cmd_data cmd_log; /* temp buffer to put */
> > + spinlock_t cmd_lock;
> > +};
>
> Please add a comment above this structure that explains the role of this
> data structure.
Got it.
>
> > +static struct ufs_s_dbg_mgr ufs_s_dbg[DBG_NUM_OF_HOSTS];
>
> A static array? That's suspicious. Should that array perhaps be a member
> of another UFS data structure, e.g. the UFS host or device data structure?
>
> > +static int ufs_s_dbg_mgr_idx;
I considered it again and will remove.
>
> What does this variable represent?
>
> > + for (i = 0 ; i < max ; i++, data++) {
> > + dev_err(dev, ": 0x%02x, %02d, 0x%08lx,
> 0x%04x, %d, %llu, %llu, 0x%llx %s",
> > + data->op,
> > + data->tag,
> > + data->lba,
> > + data->sct,
> > + data->retries,
> > + data->start_time,
> > + data->end_time,
> > + data->outstanding_reqs,
> > + ((last == i) ? "<--" : " "));
>
> Please follow the same coding style as elsewhere in the Linux kernel and
> specify multiple arguments per line (up to the current column limit).
> Please also align the arguments either with the opening parentheses or
> indent these by one tab as requested in the Linux kernel coding style
> document.
>
Got it.
> > +/*
> > + * EXTERNAL FUNCTIONS
> > + *
> > + * There are two classes that are to initialize data structures for
> > +debug
> > + * and to define actual behavior.
> > + */
> > +void exynos_ufs_dump_info(struct ufs_exynos_handle *handle, struct
> > +device *dev) {
> > + struct ufs_s_dbg_mgr *mgr = (struct ufs_s_dbg_mgr *)handle->private;
> > +
> > + if (mgr->active == 0)
> > + goto out;
> > +
> > + mgr->time = cpu_clock(raw_smp_processor_id());
> > +
> > +#if EN_PRINT_UFS_LOG
> > + ufs_s_print_cmd_log(mgr, dev);
> > +#endif
> > +
> > + if (mgr->first_time == 0ULL)
> > + mgr->first_time = mgr->time;
> > +out:
> > + return;
> > +}
>
> Using cpu_clock() without storing the CPU on which it has been sampled
> seems wrong to me. Is higher accuracy than a single jiffy required? If not,
> how about using 'jiffies' instead? From clock.h:
I think jiffies isn't proper for the original purpose.
>
> /*
> * As outlined in clock.c, provides a fast, high resolution, nanosecond
> * time source that is monotonic per cpu argument and has bounded drift
> * between cpus.
> *
> * ######################### BIG FAT WARNING ##########################
> * # when comparing cpu_clock(i) to cpu_clock(j) for i != j, time can #
> * # go backwards !! #
> * ####################################################################
> */
>
> > +void exynos_ufs_cmd_log_start(struct ufs_exynos_handle *handle,
> > + struct ufs_hba *hba, struct scsi_cmnd *cmd) {
> > + struct ufs_s_dbg_mgr *mgr = (struct ufs_s_dbg_mgr *)handle->private;
> > + int cpu = raw_smp_processor_id();
> > + struct cmd_data *cmd_log = &mgr->cmd_log; /* temp buffer to put
> */
> > + unsigned long lba = (cmd->cmnd[2] << 24) |
> > + (cmd->cmnd[3] << 16) |
> > + (cmd->cmnd[4] << 8) |
> > + (cmd->cmnd[5] << 0);
> > + unsigned int sct = (cmd->cmnd[7] << 8) |
> > + (cmd->cmnd[8] << 0);
>
> Aargh! SCSI command parsing ... Has it been considered to use
> blk_rq_pos(cmd->req) and blk_rq_bytes(cmd->req) instead?
Sure and I have recognized those things for years.
UFS depends on SCSI and regarding to actual usages.
Thus, I don't feel that sort of parsing unless there is another macro to parse SCSI commands.
As I know, nothing exits.
And blk_rq_pos doesn't represent actual lba. That represents in-partition addressing.
With this, I have to refer to the start address of partitions and I don’t think that isn't suitable in UFS driver.
>
> > +int exynos_ufs_init_dbg(struct ufs_exynos_handle *handle) {
> > + struct ufs_s_dbg_mgr *mgr;
> > +
> > + if (ufs_s_dbg_mgr_idx >= DBG_NUM_OF_HOSTS)
> > + return -1;
> > +
> > + mgr = &ufs_s_dbg[ufs_s_dbg_mgr_idx++];
> > + handle->private = (void *)mgr;
> > + mgr->handle = handle;
> > + mgr->active = 1;
>
> Can the '(void *)' cast above be left out?
I didn't want to share the details of this structure with ufs-exynos.c.
I don’t feel that it's better.
>
> > +#define UFS_VS_MMIO_VER 1
>
> What does this constant represent? Please add a comment.
I considered it again and will remove.
>
> Thanks,
>
> Bart.
Thank you for your comments even if this is a RFS patch.
Thanks.
Kiwoong Kim
