Add sysfs interface to read System Manager syslog and system info
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
drivers/firmware/imx/sm-misc.c | 97 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 97 insertions(+)
diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
index fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c615102a377f41025a6911d746770 100644
--- a/drivers/firmware/imx/sm-misc.c
+++ b/drivers/firmware/imx/sm-misc.c
@@ -44,6 +44,100 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
return 0;
}
+static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
+ char *buf)
+{
+ struct scmi_imx_misc_sys_sleep_rec *rec;
+ struct scmi_imx_misc_syslog *syslog;
+ int ret;
+ size_t len = 0;
+
+ if (!ph)
+ return 0;
+
+ syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
+ if (!syslog)
+ return -ENOMEM;
+
+ ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
+ if (ret) {
+ kfree(syslog);
+ return ret;
+ }
+
+ rec = &syslog->syssleeprecord;
+
+ len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
+ len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
+ len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
+ len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
+ len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
+ len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
+ len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
+ len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
+ len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
+
+ kfree(syslog);
+
+ return len;
+}
+
+static DEVICE_ATTR_RO(syslog);
+
+static ssize_t system_info_show(struct device *device, struct device_attribute *attr,
+ char *buf)
+{
+ struct scmi_imx_misc_system_info *info;
+ int len = 0;
+ int ret;
+
+ if (!ph)
+ return 0;
+
+ info = kmalloc(sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ ret = imx_misc_ctrl_ops->misc_discover_build_info(ph, info);
+ if (ret)
+ goto err;
+
+ ret = imx_misc_ctrl_ops->misc_cfg_info(ph, info);
+ if (ret)
+ goto err;
+
+ ret = imx_misc_ctrl_ops->misc_silicon_info(ph, info);
+ if (ret)
+ goto err;
+
+ ret = imx_misc_ctrl_ops->misc_board_info(ph, info);
+ if (ret)
+ goto err;
+
+ len += sysfs_emit_at(buf, len, "SM Version = Build %u, Commit 08%x\n",
+ info->buildnum, info->buildcommit);
+ len += sysfs_emit_at(buf, len, "SM Config = %s, mSel=%u\n",
+ info->cfgname, info->msel);
+ len += sysfs_emit_at(buf, len, "Silicon = %s\n", info->siname);
+ len += sysfs_emit_at(buf, len, "Board = %s, attr=0x%08x\n",
+ info->brdname, info->brd_attributes);
+
+ ret = len;
+err:
+ kfree(info);
+ return ret;
+}
+
+static DEVICE_ATTR_RO(system_info);
+
+static struct attribute *sm_misc_attrs[] = {
+ &dev_attr_syslog.attr,
+ &dev_attr_system_info.attr,
+ NULL,
+};
+
+ATTRIBUTE_GROUPS(sm_misc);
+
static int scmi_imx_misc_ctrl_probe(struct scmi_device *sdev)
{
const struct scmi_handle *handle = sdev->handle;
@@ -108,6 +202,9 @@ static const struct scmi_device_id scmi_id_table[] = {
MODULE_DEVICE_TABLE(scmi, scmi_id_table);
static struct scmi_driver scmi_imx_misc_ctrl_driver = {
+ .driver = {
+ .dev_groups = sm_misc_groups,
+ },
.name = "scmi-imx-misc-ctrl",
.probe = scmi_imx_misc_ctrl_probe,
.id_table = scmi_id_table,
--
2.37.1On Fri, Jun 27, 2025 at 02:03:50PM +0800, Peng Fan wrote:
> Add sysfs interface to read System Manager syslog and system info
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/firmware/imx/sm-misc.c | 97 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 97 insertions(+)
>
> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> index fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c615102a377f41025a6911d746770 100644
> --- a/drivers/firmware/imx/sm-misc.c
> +++ b/drivers/firmware/imx/sm-misc.c
> @@ -44,6 +44,100 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
> return 0;
> }
>
> +static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
> + char *buf)
> +{
> + struct scmi_imx_misc_sys_sleep_rec *rec;
> + struct scmi_imx_misc_syslog *syslog;
> + int ret;
> + size_t len = 0;
> +
> + if (!ph)
> + return 0;
> +
> + syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
> + if (!syslog)
> + return -ENOMEM;
> +
> + ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
> + if (ret) {
> + kfree(syslog);
> + return ret;
> + }
> +
> + rec = &syslog->syssleeprecord;
> +
> + len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
> + len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
> + len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
> + len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
> + len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
> + len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
> + len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
> + len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
> + len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
> +
Why can't this be individual files and values ?
--
Regards,
Sudeep
On Wed, Jul 02, 2025 at 04:22:47PM +0100, Sudeep Holla wrote:
>On Fri, Jun 27, 2025 at 02:03:50PM +0800, Peng Fan wrote:
>> Add sysfs interface to read System Manager syslog and system info
>>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>> drivers/firmware/imx/sm-misc.c | 97 ++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 97 insertions(+)
>>
>> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
>> index fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c615102a377f41025a6911d746770 100644
>> --- a/drivers/firmware/imx/sm-misc.c
>> +++ b/drivers/firmware/imx/sm-misc.c
>> @@ -44,6 +44,100 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
>> return 0;
>> }
>>
>> +static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct scmi_imx_misc_sys_sleep_rec *rec;
>> + struct scmi_imx_misc_syslog *syslog;
>> + int ret;
>> + size_t len = 0;
>> +
>> + if (!ph)
>> + return 0;
>> +
>> + syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
>> + if (!syslog)
>> + return -ENOMEM;
>> +
>> + ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
>> + if (ret) {
>> + kfree(syslog);
>> + return ret;
>> + }
>> +
>> + rec = &syslog->syssleeprecord;
>> +
>> + len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
>> + len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
>> + len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
>> + len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
>> + len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
>> + len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
>> + len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
>> + len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
>> + len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
>> +
>
>Why can't this be individual files and values ?
The System manager firmware provides a command "syslog" to dump
all the information, I just follow same.
With individual files, some values may not be consistent, because
system behaviour may change during reading two files.
Regards,
Peng
>
>--
>Regards,
>Sudeep
On Fri, Jul 04, 2025 at 06:28:02PM +0800, Peng Fan wrote:
> On Wed, Jul 02, 2025 at 04:22:47PM +0100, Sudeep Holla wrote:
> >On Fri, Jun 27, 2025 at 02:03:50PM +0800, Peng Fan wrote:
> >> Add sysfs interface to read System Manager syslog and system info
> >>
> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >> ---
> >> drivers/firmware/imx/sm-misc.c | 97 ++++++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 97 insertions(+)
> >>
> >> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> >> index fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c615102a377f41025a6911d746770 100644
> >> --- a/drivers/firmware/imx/sm-misc.c
> >> +++ b/drivers/firmware/imx/sm-misc.c
> >> @@ -44,6 +44,100 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
> >> return 0;
> >> }
> >>
> >> +static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
> >> + char *buf)
> >> +{
> >> + struct scmi_imx_misc_sys_sleep_rec *rec;
> >> + struct scmi_imx_misc_syslog *syslog;
> >> + int ret;
> >> + size_t len = 0;
> >> +
> >> + if (!ph)
> >> + return 0;
> >> +
> >> + syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
> >> + if (!syslog)
> >> + return -ENOMEM;
> >> +
> >> + ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
> >> + if (ret) {
> >> + kfree(syslog);
> >> + return ret;
> >> + }
> >> +
> >> + rec = &syslog->syssleeprecord;
> >> +
> >> + len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
> >> + len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
> >> + len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
> >> + len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
> >> + len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
> >> + len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
> >> + len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
> >> + len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
> >> + len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
> >> +
> >
> >Why can't this be individual files and values ?
>
> The System manager firmware provides a command "syslog" to dump
> all the information, I just follow same.
>
> With individual files, some values may not be consistent, because
> system behaviour may change during reading two files.
>
You definitely need to convince Greg if you take this approach. I am sure
he prefers single value sysfs files.
--
Regards,
Sudeep
Hi Peng,
kernel test robot noticed the following build warnings:
[auto build test WARNING on ecb259c4f70dd5c83907809f45bf4dc6869961d7]
url: https://github.com/intel-lab-lkp/linux/commits/Peng-Fan/firmware-arm_scmi-imx-Add-documentation-for-MISC_BOARD_INFO/20250627-140736
base: ecb259c4f70dd5c83907809f45bf4dc6869961d7
patch link: https://lore.kernel.org/r/20250627-sm-misc-api-v1-v1-7-2b99481fe825%40nxp.com
patch subject: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info
config: x86_64-buildonly-randconfig-005-20250627 (https://download.01.org/0day-ci/archive/20250628/202506282233.n54Z23oc-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250628/202506282233.n54Z23oc-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506282233.n54Z23oc-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/firmware/imx/sm-misc.c: In function 'syslog_show':
drivers/firmware/imx/sm-misc.c:58:18: error: implicit declaration of function 'kmalloc'; did you mean 'mm_alloc'? [-Werror=implicit-function-declaration]
58 | syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
| ^~~~~~~
| mm_alloc
>> drivers/firmware/imx/sm-misc.c:58:16: warning: assignment to 'struct scmi_imx_misc_syslog *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
58 | syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
| ^
drivers/firmware/imx/sm-misc.c:64:17: error: implicit declaration of function 'kfree' [-Werror=implicit-function-declaration]
64 | kfree(syslog);
| ^~~~~
drivers/firmware/imx/sm-misc.c: In function 'system_info_show':
>> drivers/firmware/imx/sm-misc.c:97:14: warning: assignment to 'struct scmi_imx_misc_system_info *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
97 | info = kmalloc(sizeof(*info), GFP_KERNEL);
| ^
cc1: some warnings being treated as errors
vim +58 drivers/firmware/imx/sm-misc.c
46
47 static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
48 char *buf)
49 {
50 struct scmi_imx_misc_sys_sleep_rec *rec;
51 struct scmi_imx_misc_syslog *syslog;
52 int ret;
53 size_t len = 0;
54
55 if (!ph)
56 return 0;
57
> 58 syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
59 if (!syslog)
60 return -ENOMEM;
61
62 ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
63 if (ret) {
64 kfree(syslog);
65 return ret;
66 }
67
68 rec = &syslog->syssleeprecord;
69
70 len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
71 len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
72 len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
73 len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
74 len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
75 len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
76 len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
77 len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
78 len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
79
80 kfree(syslog);
81
82 return len;
83 }
84
85 static DEVICE_ATTR_RO(syslog);
86
87 static ssize_t system_info_show(struct device *device, struct device_attribute *attr,
88 char *buf)
89 {
90 struct scmi_imx_misc_system_info *info;
91 int len = 0;
92 int ret;
93
94 if (!ph)
95 return 0;
96
> 97 info = kmalloc(sizeof(*info), GFP_KERNEL);
98 if (!info)
99 return -ENOMEM;
100
101 ret = imx_misc_ctrl_ops->misc_discover_build_info(ph, info);
102 if (ret)
103 goto err;
104
105 ret = imx_misc_ctrl_ops->misc_cfg_info(ph, info);
106 if (ret)
107 goto err;
108
109 ret = imx_misc_ctrl_ops->misc_silicon_info(ph, info);
110 if (ret)
111 goto err;
112
113 ret = imx_misc_ctrl_ops->misc_board_info(ph, info);
114 if (ret)
115 goto err;
116
117 len += sysfs_emit_at(buf, len, "SM Version = Build %u, Commit 08%x\n",
118 info->buildnum, info->buildcommit);
119 len += sysfs_emit_at(buf, len, "SM Config = %s, mSel=%u\n",
120 info->cfgname, info->msel);
121 len += sysfs_emit_at(buf, len, "Silicon = %s\n", info->siname);
122 len += sysfs_emit_at(buf, len, "Board = %s, attr=0x%08x\n",
123 info->brdname, info->brd_attributes);
124
125 ret = len;
126 err:
127 kfree(info);
128 return ret;
129 }
130
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Peng,
kernel test robot noticed the following build errors:
[auto build test ERROR on ecb259c4f70dd5c83907809f45bf4dc6869961d7]
url: https://github.com/intel-lab-lkp/linux/commits/Peng-Fan/firmware-arm_scmi-imx-Add-documentation-for-MISC_BOARD_INFO/20250627-140736
base: ecb259c4f70dd5c83907809f45bf4dc6869961d7
patch link: https://lore.kernel.org/r/20250627-sm-misc-api-v1-v1-7-2b99481fe825%40nxp.com
patch subject: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info
config: i386-buildonly-randconfig-001-20250628 (https://download.01.org/0day-ci/archive/20250628/202506280217.aZih1pGT-lkp@intel.com/config)
compiler: clang version 20.1.7 (https://github.com/llvm/llvm-project 6146a88f60492b520a36f8f8f3231e15f3cc6082)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250628/202506280217.aZih1pGT-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506280217.aZih1pGT-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/firmware/imx/sm-misc.c:58:11: error: call to undeclared function 'kmalloc'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
58 | syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
| ^
drivers/firmware/imx/sm-misc.c:58:11: note: did you mean 'mm_alloc'?
include/linux/sched/mm.h:16:26: note: 'mm_alloc' declared here
16 | extern struct mm_struct *mm_alloc(void);
| ^
>> drivers/firmware/imx/sm-misc.c:58:9: error: incompatible integer to pointer conversion assigning to 'struct scmi_imx_misc_syslog *' from 'int' [-Wint-conversion]
58 | syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/firmware/imx/sm-misc.c:64:3: error: call to undeclared function 'kfree'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
64 | kfree(syslog);
| ^
drivers/firmware/imx/sm-misc.c:80:2: error: call to undeclared function 'kfree'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
80 | kfree(syslog);
| ^
drivers/firmware/imx/sm-misc.c:97:9: error: call to undeclared function 'kmalloc'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
97 | info = kmalloc(sizeof(*info), GFP_KERNEL);
| ^
>> drivers/firmware/imx/sm-misc.c:97:7: error: incompatible integer to pointer conversion assigning to 'struct scmi_imx_misc_system_info *' from 'int' [-Wint-conversion]
97 | info = kmalloc(sizeof(*info), GFP_KERNEL);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/firmware/imx/sm-misc.c:127:2: error: call to undeclared function 'kfree'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
127 | kfree(info);
| ^
7 errors generated.
vim +/kmalloc +58 drivers/firmware/imx/sm-misc.c
46
47 static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
48 char *buf)
49 {
50 struct scmi_imx_misc_sys_sleep_rec *rec;
51 struct scmi_imx_misc_syslog *syslog;
52 int ret;
53 size_t len = 0;
54
55 if (!ph)
56 return 0;
57
> 58 syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
59 if (!syslog)
60 return -ENOMEM;
61
62 ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
63 if (ret) {
> 64 kfree(syslog);
65 return ret;
66 }
67
68 rec = &syslog->syssleeprecord;
69
70 len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
71 len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
72 len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
73 len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
74 len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
75 len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
76 len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
77 len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
78 len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
79
80 kfree(syslog);
81
82 return len;
83 }
84
85 static DEVICE_ATTR_RO(syslog);
86
87 static ssize_t system_info_show(struct device *device, struct device_attribute *attr,
88 char *buf)
89 {
90 struct scmi_imx_misc_system_info *info;
91 int len = 0;
92 int ret;
93
94 if (!ph)
95 return 0;
96
> 97 info = kmalloc(sizeof(*info), GFP_KERNEL);
98 if (!info)
99 return -ENOMEM;
100
101 ret = imx_misc_ctrl_ops->misc_discover_build_info(ph, info);
102 if (ret)
103 goto err;
104
105 ret = imx_misc_ctrl_ops->misc_cfg_info(ph, info);
106 if (ret)
107 goto err;
108
109 ret = imx_misc_ctrl_ops->misc_silicon_info(ph, info);
110 if (ret)
111 goto err;
112
113 ret = imx_misc_ctrl_ops->misc_board_info(ph, info);
114 if (ret)
115 goto err;
116
117 len += sysfs_emit_at(buf, len, "SM Version = Build %u, Commit 08%x\n",
118 info->buildnum, info->buildcommit);
119 len += sysfs_emit_at(buf, len, "SM Config = %s, mSel=%u\n",
120 info->cfgname, info->msel);
121 len += sysfs_emit_at(buf, len, "Silicon = %s\n", info->siname);
122 len += sysfs_emit_at(buf, len, "Board = %s, attr=0x%08x\n",
123 info->brdname, info->brd_attributes);
124
125 ret = len;
126 err:
127 kfree(info);
128 return ret;
129 }
130
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Fri, Jun 27, 2025 at 02:03:50PM +0800, Peng Fan wrote:
> Add sysfs interface to read System Manager syslog and system info
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/firmware/imx/sm-misc.c | 97 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 97 insertions(+)
>
> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
> index fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c615102a377f41025a6911d746770 100644
> --- a/drivers/firmware/imx/sm-misc.c
> +++ b/drivers/firmware/imx/sm-misc.c
> @@ -44,6 +44,100 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
> return 0;
> }
>
> +static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
> + char *buf)
> +{
> + struct scmi_imx_misc_sys_sleep_rec *rec;
> + struct scmi_imx_misc_syslog *syslog;
> + int ret;
> + size_t len = 0;
> +
> + if (!ph)
> + return 0;
> +
> + syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
> + if (!syslog)
> + return -ENOMEM;
> +
> + ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
...ah...so you basically cast to void a structure of u32 words and then
expect that the firmware perfectly aligned with how the struct is
defined here....size checks assures no out-of-bounds BUT the meaning of
he blob itself is entirely up to FW and kernel being aligned, since no
type checking is done of any kind and fields are NOT reference by
name...so may I ask why ? ..also because the scmi_imx_misc_syslog seems
pretty tiny to need iterators to parse back the reply...do you have so
tiny transpotr message size ?
..anyway I would suggest at least to add some sort of version-field to
the struct so that at least you can detect if the FW spits out something
that is not what your driver expect or is ready to handle...especially
if you plan to extend or modify the layout of the structs in the future.
> + if (ret) {
> + kfree(syslog);
> + return ret;
> + }
> +
> + rec = &syslog->syssleeprecord;
> +
> + len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
> + len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
> + len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
> + len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
> + len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
> + len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
> + len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
> + len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
> + len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
> +
... how wonder how much is still frowned upon to serve such big multiline
structured information payloads from sysfs ... (asking for a friend :P)
> + kfree(syslog);
> +
> + return len;
> +}
> +
> +static DEVICE_ATTR_RO(syslog);
> +
> +static ssize_t system_info_show(struct device *device, struct device_attribute *attr,
> + char *buf)
> +{
> + struct scmi_imx_misc_system_info *info;
> + int len = 0;
> + int ret;
> +
> + if (!ph)
> + return 0;
> +
> + info = kmalloc(sizeof(*info), GFP_KERNEL);
> + if (!info)
> + return -ENOMEM;
> +
> + ret = imx_misc_ctrl_ops->misc_discover_build_info(ph, info);
> + if (ret)
> + goto err;
> +
> + ret = imx_misc_ctrl_ops->misc_cfg_info(ph, info);
> + if (ret)
> + goto err;
> +
> + ret = imx_misc_ctrl_ops->misc_silicon_info(ph, info);
> + if (ret)
> + goto err;
> +
> + ret = imx_misc_ctrl_ops->misc_board_info(ph, info);
> + if (ret)
> + goto err;
> +
> + len += sysfs_emit_at(buf, len, "SM Version = Build %u, Commit 08%x\n",
> + info->buildnum, info->buildcommit);
> + len += sysfs_emit_at(buf, len, "SM Config = %s, mSel=%u\n",
> + info->cfgname, info->msel);
> + len += sysfs_emit_at(buf, len, "Silicon = %s\n", info->siname);
> + len += sysfs_emit_at(buf, len, "Board = %s, attr=0x%08x\n",
> + info->brdname, info->brd_attributes);
...so some of this stuff Build/Silicon/BoaRD info has pretty much
'forever' lifetime...are you sure you want to query them out of the FW
each time you issue a sysfs show ?
Cannot you simply dump this stuff once for all at probve time and
instead query misc_cfg_info() upon each show to refresh only the data
that will change ?
(this also corroborates my idea that you should somehow partition this
data into distinct structs by their lifetime...
Thanks,
Cristian
On Fri, Jun 27, 2025 at 03:11:03PM +0100, Cristian Marussi wrote:
>On Fri, Jun 27, 2025 at 02:03:50PM +0800, Peng Fan wrote:
>> Add sysfs interface to read System Manager syslog and system info
>>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>> drivers/firmware/imx/sm-misc.c | 97 ++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 97 insertions(+)
>>
>> diff --git a/drivers/firmware/imx/sm-misc.c b/drivers/firmware/imx/sm-misc.c
>> index fc3ee12c2be878e0285183e3381c9514a63d5142..55485a3c4a5c615102a377f41025a6911d746770 100644
>> --- a/drivers/firmware/imx/sm-misc.c
>> +++ b/drivers/firmware/imx/sm-misc.c
>> @@ -44,6 +44,100 @@ static int scmi_imx_misc_ctrl_notifier(struct notifier_block *nb,
>> return 0;
>> }
>>
>> +static ssize_t syslog_show(struct device *device, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct scmi_imx_misc_sys_sleep_rec *rec;
>> + struct scmi_imx_misc_syslog *syslog;
>> + int ret;
>> + size_t len = 0;
>> +
>> + if (!ph)
>> + return 0;
>> +
>> + syslog = kmalloc(sizeof(*syslog), GFP_KERNEL);
>> + if (!syslog)
>> + return -ENOMEM;
>> +
>> + ret = imx_misc_ctrl_ops->misc_syslog(ph, sizeof(*syslog), syslog);
>
>...ah...so you basically cast to void a structure of u32 words and then
>expect that the firmware perfectly aligned with how the struct is
>defined here....size checks assures no out-of-bounds BUT the meaning of
>he blob itself is entirely up to FW and kernel being aligned, since no
>type checking is done of any kind and fields are NOT reference by
>name...so may I ask why ?
I thought to use "u32 *syslog", but this needs a cast in
misc_syslog(x,y,(u32 *)syslog), or I could directly change
the misc_syslog function prototype to use 'struct scmi_imx_misc_syslog *'.
No specific reason, just think 'void *' could avoid a cast.
..also because the scmi_imx_misc_syslog seems
>pretty tiny to need iterators to parse back the reply...do you have so
>tiny transpotr message size ?
The transport memory size is 0x80 bytes, it could cover the current
syslog, but I am not sure whether in future, the firmware could
extend to add more information.
In our firmware there is one more field that I not include in this patchset,
because it is default not built in in firmware:
typedef struct
{
/*! System sleep record */
dev_sm_sys_sleep_rec_t sysSleepRecord;
/*! Device error log */
uint32_t devErrLog;
#ifdef DEV_SM_MSG_PROF_CNT
/*! Message profiling record */
dev_sm_sys_msg_rec_t sysMsgRecord;
#endif
} dev_sm_syslog_t;
typedef struct
{
uint32_t scmiChannel; /*!< Caller SCMI channel */
uint32_t chanType; /*!< SCMI channel type */
uint32_t protocolId; /*!< SCMI protocol ID */
uint32_t msgId; /*!< SCMI message ID */
uint32_t msgLatUsec; /*!< Message latency */
} dev_sm_sys_msg_prof_t;
/*!
* Message profile record
*/
typedef struct
{
/*! MSG profile log */
dev_sm_sys_msg_prof_t msgProf[DEV_SM_MSG_PROF_CNT];
} dev_sm_sys_msg_rec_t;
With profiling, we need iterator to get all the information.
In our default FW build, DEV_SM_MSG_PROF_CNT is not defined, but
I could keep iterator in case DEV_SM_MSG_PROF_CNT enabled in future.
>
>..anyway I would suggest at least to add some sort of version-field to
>the struct so that at least you can detect if the FW spits out something
>that is not what your driver expect or is ready to handle...especially
>if you plan to extend or modify the layout of the structs in the future.
Would you please share more insights on what version is needed here?
You mean add a field for syslog protocol as below?
struct scmi_imx_misc_syslog {
uint32_t version; //Set as protocol version and display it in sysfs show ops?
struct scmi_imx_misc_sys_sleep_rec syssleeprecord;
uint32_t deverrlog;
};
>
>
>> + if (ret) {
>> + kfree(syslog);
>> + return ret;
>> + }
>> +
>> + rec = &syslog->syssleeprecord;
>> +
>> + len += sysfs_emit_at(buf, len, "Wake Vector = %u\n", rec->wakesource);
>> + len += sysfs_emit_at(buf, len, "Sys sleep mode = %u\n", rec->syssleepmode);
>> + len += sysfs_emit_at(buf, len, "Sys sleep flags = 0x%08x\n", rec->syssleepflags);
>> + len += sysfs_emit_at(buf, len, "MIX power status = 0x%08x\n", rec->mixpwrstat);
>> + len += sysfs_emit_at(buf, len, "MEM power status = 0x%08x\n", rec->mempwrstat);
>> + len += sysfs_emit_at(buf, len, "PLL power status = 0x%08x\n", rec->pllpwrstat);
>> + len += sysfs_emit_at(buf, len, "Sleep latency = %u\n", rec->sleepentryusec);
>> + len += sysfs_emit_at(buf, len, "Wake latency = %u\n", rec->sleepexitusec);
>> + len += sysfs_emit_at(buf, len, "Sleep count = %u\n", rec->sleepcnt);
>> +
>
>... how wonder how much is still frowned upon to serve such big multiline
>structured information payloads from sysfs ... (asking for a friend :P)
Just take what our firmware console displays.
And in case the firmware does not have a dedicated uart on some
boards, using syslog to show similar info would be prefered,
so ...
>
>
>> + kfree(syslog);
>> +
>> + return len;
>> +}
>> +
>> +static DEVICE_ATTR_RO(syslog);
>> +
>> +static ssize_t system_info_show(struct device *device, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct scmi_imx_misc_system_info *info;
>> + int len = 0;
>> + int ret;
>> +
>> + if (!ph)
>> + return 0;
>> +
>> + info = kmalloc(sizeof(*info), GFP_KERNEL);
>> + if (!info)
>> + return -ENOMEM;
>> +
>> + ret = imx_misc_ctrl_ops->misc_discover_build_info(ph, info);
>> + if (ret)
>> + goto err;
>> +
>> + ret = imx_misc_ctrl_ops->misc_cfg_info(ph, info);
>> + if (ret)
>> + goto err;
>> +
>> + ret = imx_misc_ctrl_ops->misc_silicon_info(ph, info);
>> + if (ret)
>> + goto err;
>> +
>> + ret = imx_misc_ctrl_ops->misc_board_info(ph, info);
>> + if (ret)
>> + goto err;
>> +
>> + len += sysfs_emit_at(buf, len, "SM Version = Build %u, Commit 08%x\n",
>> + info->buildnum, info->buildcommit);
>> + len += sysfs_emit_at(buf, len, "SM Config = %s, mSel=%u\n",
>> + info->cfgname, info->msel);
>> + len += sysfs_emit_at(buf, len, "Silicon = %s\n", info->siname);
>> + len += sysfs_emit_at(buf, len, "Board = %s, attr=0x%08x\n",
>> + info->brdname, info->brd_attributes);
>
>...so some of this stuff Build/Silicon/BoaRD info has pretty much
>'forever' lifetime...are you sure you want to query them out of the FW
>each time you issue a sysfs show ?
>
>Cannot you simply dump this stuff once for all at probve time and
>instead query misc_cfg_info() upon each show to refresh only the data
>that will change ?
>
>(this also corroborates my idea that you should somehow partition this
>data into distinct structs by their lifetime...
In next version, I will change to use probe to query the information and
use sysfs to give userspace an interface to get the queried information.
Thanks,
Peng
>
>Thanks,
>Cristian
© 2016 - 2025 Red Hat, Inc.