> > When you turn on CONFIG_SCSI_UFS_EXYNOS_CMD_LOG,
> I guess you meant CONFIG_ SCSI_UFS_EXYNOS_DBG
Okay
>
>
> > +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
>
> > +
> > +#ifdef CONFIG_SCSI_UFS_EXYNOS_DBG
> Not needed inside exynos-dbg
>
> > +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");
> > + dev_err(dev, ":\t\tSCSI CMD(%u)\n", total - 1);
> > + dev_err(dev, ":--------------------------------------------------
> -\n");
> > + 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.
>
> > + 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;
> > + }
> > +}
>
>
> > 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
