>
> When you turn on CONFIG_SCSI_UFS_EXYNOS_CMD_LOG,
I guess you meant CONFIG_ SCSI_UFS_EXYNOS_DBG
> +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?
> +
> +#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?
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?
Thanks,
Avri
