quoted

When you turn on CONFIG_SCSI_UFS_EXYNOS_CMD_LOG,

I guess you meant CONFIG_ SCSI_UFS_EXYNOS_DBG

Okay 

quoted

+struct ufs_cmd_info {
+       u32 total;
+       u32 last;
+       struct cmd_data data[MAX_CMD_LOGS];
+       struct cmd_data *pdata[MAX_CMD_LOGS];

What is the use of pdata?
Looks like it is just a copy of data, is it?

Yes

quoted

+
+#ifdef CONFIG_SCSI_UFS_EXYNOS_DBG

Not needed inside exynos-dbg

quoted

+static void ufs_s_print_cmd_log(struct ufs_s_dbg_mgr *mgr, struct
+device
*dev)
+{
+       struct ufs_cmd_info *cmd_info = &mgr->cmd_info;
+       struct cmd_data *data;
+       u32 i, idx;
+       u32 last;
+       u32 max = MAX_CMD_LOGS;
+       unsigned long flags;
+       u32 total;
+
+       spin_lock_irqsave(&mgr->cmd_lock, flags);
+       total = cmd_info->total;
+       if (cmd_info->total < max)
+               max = cmd_info->total;
+       last = (cmd_info->last + MAX_CMD_LOGS - 1) % MAX_CMD_LOGS;
+       spin_unlock_irqrestore(&mgr->cmd_lock, flags);
+
+       dev_err(dev, ":--------------------------------------------------

-\n");

quoted

+       dev_err(dev, ":\t\tSCSI CMD(%u)\n", total - 1);
+       dev_err(dev, ":--------------------------------------------------

-\n");

quoted

+       dev_err(dev, ":OP, TAG, LBA, SCT, RETRIES, STIME, ETIME,
+ REQS\n\n");
+
+       idx = (last == max - 1) ? 0 : last + 1;

cmd_info->last points to the current index which is the oldest data in
your circular buffer.
Why not just idx = cmd_info->last ?
And then a better name for cmd_info->last would be cmd_info->pos or
cmd_info->current, Or did I get it wrong?

The member 'last' means a next available slot.
I agree this name is a little bit confusing and will change it.

I would also not worried about max and total - so the first round after
platform boot time it might print zeros.
Really don't worth the unnecessary compilation.

quoted

+       data = &cmd_info->data[idx];
+       for (i = 0 ; i < max ; i++, data = &cmd_info->data[idx]) {
+               dev_err(dev, ": 0x%02x, %02d, 0x%08llx, 0x%04x, %d,
+ %llu, %llu,
0x%llx",
+                       data->op, data->tag, data->lba, data->sct, data-
retries,
+                       data->start_time, data->end_time, data-
outstanding_reqs);
+               idx = (idx == max - 1) ? 0 : idx + 1;
+       }
+}

quoted

diff --git a/drivers/scsi/ufs/ufs-exynos-if.h

b/drivers/scsi/ufs/ufs-exynos-if.h
new file mode 100644
index 0000000..c746f59

--- /dev/null
+++ b/drivers/scsi/ufs/ufs-exynos-if.h

@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * UFS Exynos debugging functions

So this header contains a single struct with an opaque pointer.
If it is just for debugging, why  not ufs-exynos-dbg.h?

Okay. 

Thanks,
Avri