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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 20 Jul 2026, Tzung-Bi Shih wrote:

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

It should use __free(), not goto.

-- 
 i.

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