RE: [RFC PATCH v1 2/2] exynos-ufs: support command history

["Kiwoong Kim" <kwmad.kim@samsung.com>]

> > +#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

Got it.

> 
> 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? 

Got it.

> 
> > +static int ufs_s_dbg_mgr_idx;
> 
> What does this variable represent? 

I considered it again and will remove.

> 
> > +	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