Re: [PATCH v2 2/8] firmware: smccc: Add support for Live Firmware Activation (LFA)

[Andre Przywara]

Hi Krzysztof,

thanks for having a look!

On 3/18/26 09:12, Krzysztof Kozlowski wrote:
> On 18/03/2026 09:09, Krzysztof Kozlowski wrote:
> > On Tue, Mar 17, 2026 at 11:33:28AM +0100, Andre Przywara wrote:
> > > +
> > > +/* A list of known GUIDs, to be shown in the "name" sysfs file. */
> > > +static const struct fw_image_uuid {
> > > +	const char *name;
> > > +	const char *uuid;
> > > +} fw_images_uuids[] = {
> > > +	{
> > > +		.name = "TF-A BL31 runtime",
> > > +		.uuid = "47d4086d-4cfe-9846-9b95-2950cbbd5a00",
> > > +	},
> > > +	{
> > > +		.name = "BL33 non-secure payload",
> > > +		.uuid = "d6d0eea7-fcea-d54b-9782-9934f234b6e4",
> > > +	},
> > > +	{
> > > +		.name = "TF-RMM",
> > > +		.uuid = "6c0762a6-12f2-4b56-92cb-ba8f633606d9",
> > > +	},
> > > +};
> > > +
> > > +static struct kset *lfa_kset;
> > > +static struct workqueue_struct *fw_images_update_wq;
> > > +static struct work_struct fw_images_update_work;
> > > +static struct attribute *image_default_attrs[LFA_ATTR_NR_IMAGES + 1];
> >
> > Bunch of singletons here because (see later)...
> >
> > > +
> > > +static const struct attribute_group image_attr_group = {
> > > +	.attrs = image_default_attrs,
> > > +};
> > > +
> > > +static const struct attribute_group *image_default_groups[] = {
> > > +	&image_attr_group,
> > > +	NULL
> > > +};
> > > +
> > > +static int __init lfa_init(void)
> > > +{
> > > +	struct arm_smccc_1_2_regs reg = { 0 };
> > > +	int err;
> > > +
> > > +	reg.a0 = LFA_1_0_FN_GET_VERSION;
> > > +	arm_smccc_1_2_invoke(&reg, &reg);
> > > +	if (reg.a0 == -LFA_NOT_SUPPORTED) {
> > > +		pr_info("Live Firmware activation: no firmware agent found\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	pr_info("Live Firmware Activation: detected v%ld.%ld\n",
> > > +		reg.a0 >> 16, reg.a0 & 0xffff);
> > > +
> > > +	fw_images_update_wq = alloc_workqueue("fw_images_update_wq",
> > > +					      WQ_UNBOUND | WQ_MEM_RECLAIM, 1);
> > > +	if (!fw_images_update_wq) {
> > > +		pr_err("Live Firmware Activation: Failed to allocate workqueue.\n");
> > > +
> > > +		return -ENOMEM;
> > > +	}
> > > +	INIT_WORK(&fw_images_update_work, remove_invalid_fw_images);
> > > +
> > > +	init_image_default_attrs();
> > > +	lfa_kset = kset_create_and_add("lfa", NULL, firmware_kobj);
> > > +	if (!lfa_kset)
> > > +		return -ENOMEM;
> > > +
> > > +	err = update_fw_images_tree();
> > > +	if (err != 0) {
> > > +		kset_unregister(lfa_kset);
> > > +		destroy_workqueue(fw_images_update_wq);
> > > +	}
> > > +
> > > +	return err;
> > > +}
> > > +module_init(lfa_init);
> >
> > You do not use driver model, but 199x style of modprobing and performing
> > actions.
> >
> > I do not understand why module load is already doign anything. This
> > looks like misinterpretation/misuse of Linux driver model - in a way,
> > you don't use it all and this is like back to 199x where modprobe was
> > already meaning you bind drivers...
>
> Although now going through further patches I found you implementing some
> parts of driver model, so probably this split is just needing fix.

The discovery of the LFA service works via SMC calls, which have a safe 
discovery route by just calling arm_smccc_1_2_invoke() - that function 
will query the conduit and do all the necessary checks.

But yes, this looks a bit out of place, and indeed there have been 
proposals to create some kind of "SMCCC bus", even though this requires 
some squinting to call this a "bus". But it makes some sense in the 
Linux driver model, so we probably want this.
At the moment there is smccc_trng[1], where the SMCCC code registers a 
simple platform device, which is matched later in the driver. But this 
seems somewhat ad-hoc and overkill as well.
I just got pointed to Aneesh's recently proposed [2] auxiliary bus 
approach for SMCCC, it looks like this could be generalised for all 
SMCCC users?
Sudeep, can you comment what's the latest on this front? Have there been 
patches or more sketched out proposals for a proper bus already?

Cheers,
Andre

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/smccc/smccc.c#n88
[2] https://lore.kernel.org/linux-arm-kernel/yq5av7f51d8y.fsf@xxxxxxxxxx/

P.S.: So yes, I was aware that putting this in module_init() was not the 
neatest solution, but I didn't want to hold all the rest of the code 
back until we get a better one (TM).

> Modprobe must not do "arm_smccc_1_2_invoke" or any other device related
> things. You only initialize your bus, just like every other bus driver
> would do, but honestly this should not be a bus-like code.
>
> Best regards,
> Krzysztof