Re: [PATCH v4] firmware: smccc: Support optional Arm SMC SOC_ID name

[Paul Benoit]

Thank you Sudeep and Mark.

On 3/3/2025 9:45 AM, Sudeep Holla wrote:
> On Mon, Mar 03, 2025 at 02:18:05PM +0000, Mark Rutland wrote:
> > Hi Paul,
> >
> > This looks generally good with a couple of minor nits.
> >
> > On Tue, Feb 18, 2025 at 04:59:32PM -0800, Paul Benoit wrote:
> > > Issue Number 1.6 of the Arm SMC Calling Convention introduces an optional
> > > SOC_ID name string.  If implemented, point the 'machine' field of the SoC
> > > Device Attributes at this string so that it will appear under
> > > /sys/bus/soc/devices/soc0/machine.
> > >
> > > On Arm SMC compliant SoCs, this will allow things like 'lscpu' to
> > > eventually get a SoC provider model name from there rather than each
> > > tool/utility needing to get a possibly inconsistent, obsolete, or incorrect
> > > model/machine name from its own hardcoded model/machine name table.
> > >
> > > Signed-off-by: Paul Benoit <paul@xxxxxxxxxxxxxxxxxxxxxx>
> > > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > > Cc: Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx>
> > > Cc: Sudeep Holla <sudeep.holla@xxxxxxx>
> > > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > > ---
>
> [...]
>
> > > +static char __init *smccc_soc_name_init(void)
> > > +{
> > > +	struct arm_smccc_1_2_regs args;
> > > +	struct arm_smccc_1_2_regs res;
> > > +	size_t len;
> > > +
> > > +	/*
> > > +	 * Issue Number 1.6 of the Arm SMC Calling Convention
> > > +	 * specification introduces an optional "name" string
> > > +	 * to the ARM_SMCCC_ARCH_SOC_ID function.  Fetch it if
> > > +	 * available.
> > > +	 */
> > > +	args.a0 = ARM_SMCCC_ARCH_SOC_ID;
> > > +	args.a1 = 2;    /* SOC_ID name */
> > > +	arm_smccc_1_2_invoke(&args, &res);
> > > +	if ((u32)res.a0 == 0) {
> > > +		const unsigned int regsize = sizeof(res.a1);
> > > +
> > > +		/*
> > > +		 * Copy res.a1..res.a17 to the smccc_soc_id_name string
> > > +		 * 8 bytes at a time.  As per Issue 1.6 of the Arm SMC
> > > +		 * Calling Convention, the string will be NUL terminated
> > > +		 * and padded, from the end of the string to the end of the
> > > +		 * 136 byte buffer, with NULs.
> > > +		 */
> > > +		str_fragment_from_reg(smccc_soc_id_name + 0*regsize, res.a1);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 1*regsize, res.a2);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 2*regsize, res.a3);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 3*regsize, res.a4);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 4*regsize, res.a5);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 5*regsize, res.a6);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 6*regsize, res.a7);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 7*regsize, res.a8);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 8*regsize, res.a9);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 9*regsize, res.a10);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 10*regsize, res.a11);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 11*regsize, res.a12);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 12*regsize, res.a13);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 13*regsize, res.a14);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 14*regsize, res.a15);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 15*regsize, res.a16);
> > > +		str_fragment_from_reg(smccc_soc_id_name + 16*regsize, res.a17);
> >
> > Please get rid of 'regsize' and use '8' directly. This only exists for
> > arm64, where the registeres are 8 bytes, and the comment immediately
> > above refers to "8 bytes" specifically anyway, so 'regsize' only serves
> > to make this harder to read.
> >
> > It'd be a bit clearer as:
> >
> > 	str_fragment_from_reg(smccc_soc_id_name + 8 * 0,  res.a1);
> > 	str_fragment_from_reg(smccc_soc_id_name + 8 * 1,  res.a2);
> > 	...
> > 	str_fragment_from_reg(smccc_soc_id_name + 8 * 15, res.a16);
> > 	str_fragment_from_reg(smccc_soc_id_name + 8 * 16, res.a17);
> >
> > Sudeep, are you happy to fix that up when applying?
> >
> > > +
> > > +		len = strnlen(smccc_soc_id_name, sizeof(smccc_soc_id_name));
> > > +		if (len) {
> > > +			if (len == sizeof(smccc_soc_id_name))
> > > +				pr_warn(FW_BUG "Ignoring improperly formatted Name\n");
> >
> > It's odd that 'Name' is capitalized here. Not a big deal, but it doesn't
> > look quite right.
>
> I can fix both of these and apply. No need to repost.