linux-kernel - Re: [PATCH v2 5/6] firmware: arm_scmi: imx: Support getting syslog of MISC protocol

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250821090515.GA19763@nxa18884-linux.ap.freescale.net>
Date: Thu, 21 Aug 2025 17:05:15 +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 v2 5/6] firmware: arm_scmi: imx: Support getting syslog
 of MISC protocol

Hi Cristian,

On Tue, Aug 19, 2025 at 06:09:52PM +0100, Cristian Marussi wrote:
>On Thu, Jul 10, 2025 at 04:33:30PM +0800, Peng Fan wrote:
>> MISC protocol supports getting system log regarding system sleep latency
>> ,wakeup interrupt and etc. Add the API for user to retrieve the
>> information from SM.
>> 
>> Signed-off-by: Peng Fan <peng.fan@....com>
>> ---
>>  .../firmware/arm_scmi/vendors/imx/imx-sm-misc.c    | 78 ++++++++++++++++++++++
>>  include/linux/scmi_imx_protocol.h                  | 19 ++++++
>>  2 files changed, 97 insertions(+)
>> 
>> diff --git a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> index 6b86c35c192d02e13f0d2a7d713bc447886b84bf..193a862cf9b807232f04a6dbbd6a8efd1b40ff73 100644
>> --- a/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> +++ b/drivers/firmware/arm_scmi/vendors/imx/imx-sm-misc.c
>> @@ -27,6 +27,7 @@ enum scmi_imx_misc_protocol_cmd {
>>  	SCMI_IMX_MISC_CTRL_GET	= 0x4,
>>  	SCMI_IMX_MISC_DISCOVER_BUILDINFO = 0x6,
>>  	SCMI_IMX_MISC_CFG_INFO = 0xC,
>> +	SCMI_IMX_MISC_SYSLOG = 0xD,
>>  	SCMI_IMX_MISC_BOARD_INFO = 0xE,
>>  	SCMI_IMX_MISC_CTRL_NOTIFY = 0x8,
>>  };
>> @@ -89,6 +90,19 @@ struct scmi_imx_misc_cfg_info_out {
>>  	u8 cfgname[MISC_MAX_CFGNAME];
>>  };
>
>Hi,
>
>one consideration down below...
>
>>  
>> +struct scmi_imx_misc_syslog_in {
>> +	__le32 flags;
>> +	__le32 index;
>> +};
>> +
>> +#define REMAINING(x)	le32_get_bits((x), GENMASK(31, 20))
>> +#define RETURNED(x)	le32_get_bits((x), GENMASK(11, 0))
>> +
>> +struct scmi_imx_misc_syslog_out {
>> +	__le32 numlogflags;
>> +	__le32 syslog[];
>> +};
>> +
>>  static int scmi_imx_misc_attributes_get(const struct scmi_protocol_handle *ph,
>>  					struct scmi_imx_misc_info *mi)
>>  {
>> @@ -372,10 +386,74 @@ static int scmi_imx_misc_cfg_info(const struct scmi_protocol_handle *ph)
>>  	return ret;
>>  }
>>  
>> +struct scmi_imx_misc_syslog_ipriv {
>> +	u32 *array;
>> +};
>> +
>
>So, AFAIU, you basically use this generic u32 array to retrieve data words from
>your FW in a generic way....
>
>> +static void iter_misc_syslog_prepare_message(void *message, u32 desc_index,
>> +					     const void *priv)
>> +{
>> +	struct scmi_imx_misc_syslog_in *msg = message;
>> +
>> +	msg->flags = cpu_to_le32(0);
>> +	msg->index = cpu_to_le32(desc_index);
>> +}
>> +
>> +static int iter_misc_syslog_update_state(struct scmi_iterator_state *st,
>> +					 const void *response, void *priv)
>> +{
>> +	const struct scmi_imx_misc_syslog_out *r = response;
>> +
>> +	st->num_returned = RETURNED(r->numlogflags);
>> +	st->num_remaining = REMAINING(r->numlogflags);
>> +
>> +	return 0;
>> +}
>> +
>> +static int
>> +iter_misc_syslog_process_response(const struct scmi_protocol_handle *ph,
>> +				  const void *response,
>> +				  struct scmi_iterator_state *st, void *priv)
>> +{
>> +	const struct scmi_imx_misc_syslog_out *r = response;
>> +	struct scmi_imx_misc_syslog_ipriv *p = priv;
>> +
>> +	p->array[st->desc_index + st->loop_idx] =
>> +		le32_to_cpu(r->syslog[st->loop_idx]);
>> +
>> +	return 0;
>> +}
>> +
>> +static int scmi_imx_misc_syslog(const struct scmi_protocol_handle *ph, u16 size,
>> +				void *array)
>
>...and you provide a output array param and its size to use in the
>iterators to retrieve your data...
>
>...so you can use size to properly let iterators check bounds...
>
>...so far so good BUT...
>
>...is it not a possibility, especially with different FW versions in the
>future, that the platform will return LESS than size data, because maybe
>different platform can return different log data...I deduce this from
>the fact that you are using a generic u32 array...
>
>...so in this scenario wouldn't be useful to have the above size param as
>being both an input and an output parameter using a pointer ? 
>
>So that you can...
>
>	static int scmi_imx_misc_syslog(const struct scmi_protocol_handle *ph,
>					u16 *size, void *array)
>
>...use *size as the max_resources for iterators as of now BUT also pass
>it down to the iterators in ipriv->size so that you can easily once for
>all in prepare_message
>
>	*(ipriv->size) = st->num_returned + st->num_remaining;
>
>...so that you can KNOW if the specific FW has returned less items than
>the maximum *size slots provided in *array ?
>
>I maybe overthinking...and also this scenario will assume that the FW
>can return less items, BUT still contiguos items...i.e. no holes in the
>array....
>
>...thoughts ?

The current platforms that deployed i.MX System Manager firmware use
a fixed log structure and return them all when input log index is 0.
But you bring a valid point that the firmware may report less in future
or new platforms. I will follow you suggestion and update in V3.

I would send out V3 in early next week. Wait to see if Sudeep has
other comments.

Thanks for reviewing the patchset.

Thanks,
Peng

>
>Thanks,
>Cristian