| Message ID | 1592635992-35619-2-git-send-email-kwmad.kim@samsung.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,v1,1/2] ufs: introduce callbacks to get command information | expand |
> +#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.
> > +#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
> > +#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. Got it. > > > +/* 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. 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? > > > +static int ufs_s_dbg_mgr_idx; I considered it again and will remove. > > 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. > 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
diff --git a/drivers/scsi/ufs/Makefile b/drivers/scsi/ufs/Makefile index f0c5b95..d9e4da7 100644 --- a/drivers/scsi/ufs/Makefile +++ b/drivers/scsi/ufs/Makefile @@ -4,7 +4,7 @@ obj-$(CONFIG_SCSI_UFS_DWC_TC_PCI) += tc-dwc-g210-pci.o ufshcd-dwc.o tc-dwc-g210. obj-$(CONFIG_SCSI_UFS_DWC_TC_PLATFORM) += tc-dwc-g210-pltfrm.o ufshcd-dwc.o tc-dwc-g210.o obj-$(CONFIG_SCSI_UFS_CDNS_PLATFORM) += cdns-pltfrm.o obj-$(CONFIG_SCSI_UFS_QCOM) += ufs-qcom.o -obj-$(CONFIG_SCSI_UFS_EXYNOS) += ufs-exynos.o +obj-$(CONFIG_SCSI_UFS_EXYNOS) += ufs-exynos.o ufs-exynos-dbg.o obj-$(CONFIG_SCSI_UFSHCD) += ufshcd-core.o ufshcd-core-y += ufshcd.o ufs-sysfs.o ufshcd-core-$(CONFIG_SCSI_UFS_BSG) += ufs_bsg.o diff --git a/drivers/scsi/ufs/ufs-exynos-dbg.c b/drivers/scsi/ufs/ufs-exynos-dbg.c new file mode 100644 index 0000000..2265af7 --- /dev/null +++ b/drivers/scsi/ufs/ufs-exynos-dbg.c @@ -0,0 +1,208 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * UFS Exynos debugging functions + * + * Copyright (C) 2020 Samsung Electronics Co., Ltd. + * Author: Kiwoong Kim <kwmad.kim@samsung.com> + * + */ +#include <linux/platform_device.h> +#include <linux/module.h> +#include "ufshcd.h" +#include "ufs-exynos.h" +#include "ufs-exynos-if.h" + +#define EN_PRINT_UFS_LOG 1 + +/* Structure for ufs cmd logging */ +#define MAX_CMD_LOGS 32 + +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; +}; + +struct ufs_cmd_info { + u32 total; + u32 last; + struct cmd_data data[MAX_CMD_LOGS]; + struct cmd_data *pdata[32]; /* Currently, 32 slots */ +}; + +#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; +}; +static struct ufs_s_dbg_mgr ufs_s_dbg[DBG_NUM_OF_HOSTS]; +static int ufs_s_dbg_mgr_idx; + +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 = cmd_info->data; + u32 i; + 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"); + + 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) ? "<--" : " ")); + if (last == i) + dev_err(dev, "\n"); + } +} + +static void ufs_s_put_cmd_log(struct ufs_s_dbg_mgr *mgr, + struct cmd_data *cmd_data) +{ + struct ufs_cmd_info *cmd_info = &mgr->cmd_info; + unsigned long flags; + struct cmd_data *pdata; + + spin_lock_irqsave(&mgr->cmd_lock, flags); + pdata = &cmd_info->data[cmd_info->last]; + ++cmd_info->total; + ++cmd_info->last; + cmd_info->last = cmd_info->last % MAX_CMD_LOGS; + spin_unlock_irqrestore(&mgr->cmd_lock, flags); + + pdata->op = cmd_data->op; + pdata->tag = cmd_data->tag; + pdata->lba = cmd_data->lba; + pdata->sct = cmd_data->sct; + pdata->retries = cmd_data->retries; + pdata->start_time = cmd_data->start_time; + pdata->end_time = 0; + pdata->outstanding_reqs = cmd_data->outstanding_reqs; + cmd_info->pdata[cmd_data->tag] = pdata; +} + +/* + * 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; +} + +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); + + if (mgr->active == 0) + return; + + cmd_log->start_time = cpu_clock(cpu); + cmd_log->op = cmd->cmnd[0]; + cmd_log->tag = cmd->request->tag; + + /* This function runtime is protected by spinlock from outside */ + cmd_log->outstanding_reqs = hba->outstanding_reqs; + + /* unmap */ + if (cmd->cmnd[0] != UNMAP) + cmd_log->lba = lba; + + cmd_log->sct = sct; + cmd_log->retries = cmd->allowed; + + ufs_s_put_cmd_log(mgr, cmd_log); +} + +void exynos_ufs_cmd_log_end(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; + struct ufs_cmd_info *cmd_info = &mgr->cmd_info; + int cpu = raw_smp_processor_id(); + int tag = cmd->request->tag; + + if (mgr->active == 0) + return; + + cmd_info->pdata[tag]->end_time = cpu_clock(cpu); +} + +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; + + /* cmd log */ + spin_lock_init(&mgr->cmd_lock); + + return 0; +} +MODULE_AUTHOR("Kiwoong Kim <kwmad.kim@samsung.com>"); +MODULE_DESCRIPTION("Exynos UFS debug information"); +MODULE_LICENSE("GPL"); +MODULE_VERSION("0.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..33f7187 --- /dev/null +++ b/drivers/scsi/ufs/ufs-exynos-if.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * UFS Exynos debugging functions + * + * Copyright (C) 2020 Samsung Electronics Co., Ltd. + * Author: Kiwoong Kim <kwmad.kim@samsung.com> + * + */ +#ifndef _UFS_EXYNOS_IF_H_ +#define _UFS_EXYNOS_IF_H_ + +#define UFS_VS_MMIO_VER 1 + +/* more members would be added in the future */ +struct ufs_exynos_handle { + void *private; +}; + +#endif /* _UFS_EXYNOS_IF_H_ */ diff --git a/drivers/scsi/ufs/ufs-exynos.c b/drivers/scsi/ufs/ufs-exynos.c index 440f2af..50437db 100644 --- a/drivers/scsi/ufs/ufs-exynos.c +++ b/drivers/scsi/ufs/ufs-exynos.c @@ -1008,6 +1008,12 @@ static int exynos_ufs_init(struct ufs_hba *hba) goto out; exynos_ufs_specify_phy_time_attr(ufs); exynos_ufs_config_smu(ufs); + + /* init dbg */ + ret = exynos_ufs_init_dbg(ufs->handle); + if (ret) + return ret; + spin_lock_init(&ufs->dbg_lock); return 0; phy_off: @@ -1210,6 +1216,41 @@ static int exynos_ufs_resume(struct ufs_hba *hba, enum ufs_pm_op pm_op) return 0; } +static void exynos_ufs_dbg_register_dump(struct ufs_hba *hba) +{ + struct exynos_ufs *ufs = ufshcd_get_variant(hba); + unsigned long flags; + + spin_lock_irqsave(&ufs->dbg_lock, flags); + if (ufs->under_dump == 0) + ufs->under_dump = 1; + else { + spin_unlock_irqrestore(&ufs->dbg_lock, flags); + goto out; + } + spin_unlock_irqrestore(&ufs->dbg_lock, flags); + + exynos_ufs_dump_info(ufs->handle, hba->dev); + + spin_lock_irqsave(&ufs->dbg_lock, flags); + ufs->under_dump = 0; + spin_unlock_irqrestore(&ufs->dbg_lock, flags); +out: + return; +} + +static void exynos_ufs_cmd_log(struct ufs_hba *hba, + struct scsi_cmnd *cmd, int enter) +{ + struct exynos_ufs *ufs = ufshcd_get_variant(hba); + struct ufs_exynos_handle *handle = ufs->handle; + + if (enter == 1) + exynos_ufs_cmd_log_start(handle, hba, cmd); + else if (enter == 2) + exynos_ufs_cmd_log_end(handle, hba, cmd); +} + static struct ufs_hba_variant_ops ufs_hba_exynos_ops = { .name = "exynos_ufs", .init = exynos_ufs_init, @@ -1221,6 +1262,8 @@ static struct ufs_hba_variant_ops ufs_hba_exynos_ops = { .hibern8_notify = exynos_ufs_hibern8_notify, .suspend = exynos_ufs_suspend, .resume = exynos_ufs_resume, + .dbg_register_dump = exynos_ufs_dbg_register_dump, + .cmd_log = exynos_ufs_cmd_log, }; static int exynos_ufs_probe(struct platform_device *pdev) diff --git a/drivers/scsi/ufs/ufs-exynos.h b/drivers/scsi/ufs/ufs-exynos.h index 76d6e39..14347fb 100644 --- a/drivers/scsi/ufs/ufs-exynos.h +++ b/drivers/scsi/ufs/ufs-exynos.h @@ -8,6 +8,7 @@ #ifndef _UFS_EXYNOS_H_ #define _UFS_EXYNOS_H_ +#include "ufs-exynos-if.h" /* * UNIPRO registers @@ -212,6 +213,10 @@ struct exynos_ufs { #define EXYNOS_UFS_OPT_BROKEN_AUTO_CLK_CTRL BIT(2) #define EXYNOS_UFS_OPT_BROKEN_RX_SEL_IDX BIT(3) #define EXYNOS_UFS_OPT_USE_SW_HIBERN8_TIMER BIT(4) + + struct ufs_exynos_handle *handle; + spinlock_t dbg_lock; + int under_dump; }; #define for_each_ufs_rx_lane(ufs, i) \ @@ -284,4 +289,12 @@ struct exynos_ufs_uic_attr exynos7_uic_attr = { .rx_hs_g3_prep_sync_len_cap = PREP_LEN(0xf), .pa_dbg_option_suite = 0x30103, }; + +/* public function declarations */ +void exynos_ufs_dump_info(struct ufs_exynos_handle *handle, struct device *dev); +void exynos_ufs_cmd_log_start(struct ufs_exynos_handle *handle, + struct ufs_hba *hba, struct scsi_cmnd *cmd); +void exynos_ufs_cmd_log_end(struct ufs_exynos_handle *handle, + struct ufs_hba *hba, struct scsi_cmnd *cmd); +int exynos_ufs_init_dbg(struct ufs_exynos_handle *handle); #endif /* _UFS_EXYNOS_H_ */
This patch is to record contexts of incoming commands in a circular queue. ufshcd.c has already some function tracer calls to get command history but ftrace would be gone when system dies before you get the information, such as panic cases. Setting EN_PRINT_UFS_LOG to one enables to print information in the circular queue whenever ufshcd.c invokes a dbg_register_dump callback. Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> --- drivers/scsi/ufs/Makefile | 2 +- drivers/scsi/ufs/ufs-exynos-dbg.c | 208 ++++++++++++++++++++++++++++++++++++++ drivers/scsi/ufs/ufs-exynos-if.h | 19 ++++ drivers/scsi/ufs/ufs-exynos.c | 43 ++++++++ drivers/scsi/ufs/ufs-exynos.h | 13 +++ 5 files changed, 284 insertions(+), 1 deletion(-) create mode 100644 drivers/scsi/ufs/ufs-exynos-dbg.c create mode 100644 drivers/scsi/ufs/ufs-exynos-if.h