> +#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.
> +/* 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.
> +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?
> +#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.
> +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;
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.
> +/*
> + * 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:
/*
* 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?
> +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?
> +#define UFS_VS_MMIO_VER 1
What does this constant represent? Please add a comment.
Thanks,
Bart.
