Message-ID: <20250630033456.GE13878@nxa18884-linux>
Date: Mon, 30 Jun 2025 11:34:56 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Cristian Marussi <cristian.marussi@....com>
Cc: Peng Fan <peng.fan@....com>, Sudeep Holla <sudeep.holla@....com>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, arm-scmi@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 7/7] firmware: imx: sm-misc: Dump syslog and system info

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@....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

Powered by blists - more mailing lists