Re: [PATCH v7 3/3] firmware: coreboot: Add CFR firmware attributes driver

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Fri, Jul 17, 2026 at 09:50:03AM +0100, Sean Rhodes wrote:
> diff --git a/drivers/firmware/coreboot/coreboot-cfr.c b/drivers/firmware/coreboot/coreboot-cfr.c
...
> +static int coreboot_cfr_read_efi_value_locked(efi_char16_t *efi_name,
> +					      u32 *value, u32 *attrs)
> +{

There is no coreboot_cfr_read_efi_value() and the symbol isn't exported
anyway.  The "_locked" suffix here is redundant.  Instead, it can leave a
note in the comment to indicate the lock requirement.

> +static int coreboot_cfr_read_value(const struct coreboot_cfr_setting *setting,
> +				   u32 *value, u32 *attrs)
> +{
> +	efi_char16_t *efi_name;
> +	int ret;
> +
> +	efi_name = coreboot_cfr_efi_name(setting->name);
> +	if (IS_ERR(efi_name))
> +		return PTR_ERR(efi_name);
> +
> +	ret = efivar_lock();
> +	if (ret) {
> +		kfree(efi_name);
> +		return ret;

To be neat, use a goto statement to clean up.

> +static int coreboot_cfr_write_efi_value_locked(efi_char16_t *efi_name,
> +					       u32 value, u32 attrs)

Same here.  s/_locked//.

> +static int coreboot_cfr_write_value(struct coreboot_cfr_setting *setting,
> +				    u32 value)
> +{
> +	efi_char16_t *efi_name;
> +	u32 attrs;
> +	u32 old;
> +	int restore_ret;
> +	int ret;
> +	bool changed = false;

The initialization can be eliminated.  See comments below.

> +
> +	if (setting->read_only)
> +		return -EACCES;
> +
> +	efi_name = coreboot_cfr_efi_name(setting->name);
> +	if (IS_ERR(efi_name))
> +		return PTR_ERR(efi_name);
> +
> +	mutex_lock(&setting->drvdata->lock);

Since it already includes cleanup.h, how about using a guard()?

> +	ret = coreboot_cfr_write_efi_value_locked(efi_name, value, attrs);
> +	if (ret)
> +		goto out_unlock_efi;
> +	changed = true;
> +
> +	ret = coreboot_cfr_apply_runtime(setting);
> +	if (ret == -EOPNOTSUPP) {
> +		/* EFI changed; firmware will consume it after reboot. */
> +		setting->drvdata->pending_reboot = true;
> +		ret = 0;
> +	} else if (ret) {
> +		restore_ret = coreboot_cfr_write_efi_value_locked(efi_name, old, attrs);
> +		if (restore_ret) {
> +			setting->drvdata->pending_reboot = true;
> +			ret = restore_ret;
> +		} else if (coreboot_cfr_apply_runtime(setting)) {

Does it need to apply runtime again for old values?  The previous
coreboot_cfr_apply_runtime() for new values was just failed (i.e., the new
values shouldn't take effect).

> +			setting->drvdata->pending_reboot = true;
> +		} else {
> +			changed = false;
> +		}
> +	}
> +
> +out_unlock_efi:
> +	efivar_unlock();
> +out_unlock_mutex:
> +	mutex_unlock(&setting->drvdata->lock);
> +	kfree(efi_name);
> +
> +	if (changed)
> +		kobject_uevent(&setting->drvdata->class_dev->kobj, KOBJ_CHANGE);

This shouldn't be in the cleanup path.  Move it before the label
"out_unlock_efi".  `changed` only makes sense after calling
coreboot_cfr_write_efi_value_locked().

> +static bool
> +coreboot_cfr_possible_values_fit(const struct coreboot_cfr_setting *setting)

The function needs some comments to explain why and what.

> +{
> +	size_t len = 1; /* Trailing newline. */
> +	unsigned int i;
> +
> +	for (i = 0; i < setting->n_values; i++) {
> +		if (i)
> +			len++;

What is this for?  For ';'?

> +
> +		if (strlen(setting->values[i].label) >= PAGE_SIZE - len)
> +			return false;
> +
> +		len += strlen(setting->values[i].label);

A straightforward way:

    len += strlen(...);
    if (len >= PAGE_SIZE)
        ...

> +static int coreboot_cfr_add_numeric_option(struct coreboot_cfr_drvdata *data,
> +					   const struct lb_cfr_numeric_option *option,
> +					   bool parent_read_only)
> +{
...
> +	if (setting->type != COREBOOT_CFR_SETTING_NUMBER &&
> +	    !coreboot_cfr_possible_values_fit(setting)) {
> +		ret = 0;
> +		goto err_put_setting;
> +	}

Why it skips if the possible values can be truncated?  Is it seen as a
critical setting?

This needs some comments to explain why and what.

> +
> +	ret = coreboot_cfr_setting_is_usable(setting);
> +	if (ret) {
> +		if (ret == -ENOENT || ret == -EINVAL || ret == -EOPNOTSUPP ||
> +		    ret == -ENAMETOOLONG)
> +			ret = 0;

Does it check for values from coreboot table have corresponding EFI vars?
Why these errors aren't considered as failures?

This needs some comments to explain why and what.

> +		goto err_put_setting;
> +	}
> +
> +	ret = coreboot_cfr_register_setting(data, setting);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

A simple way:

    return coreboot_cfr_register_setting(...);

Also it may be worth some comments to mention that after calling
coreboot_cfr_register_setting(), `coreboot_cfr_setting_ktype` takes care of
the resource release.