Re: [PATCH v10 4/4] firmware: coreboot: Add CFR firmware attributes driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Mixing goto-based error handling with scope-based cleanup helpers (`__free` and `guard`) in the same function.
--

--- Patch [4]: [PATCH v10 4/4] firmware: coreboot: Add CFR firmware attributes driver ---
commit f075e88024f4dbd198791a1dc20f657a026b2206
Author: Sean Rhodes <sean@starlabs.systems>

firmware: coreboot: Add CFR firmware attributes driver

This commit adds a firmware-attributes driver for coreboot CFR settings. It
exposes runtime EFI variable-backed configuration options to userspace via
sysfs, allowing modification and handling of runtime-apply mechanisms.

Link: https://doc.coreboot.org/drivers/cfr.html
Link: https://doc.coreboot.org/drivers/cfr_internal.html

> diff --git a/drivers/firmware/coreboot/coreboot-cfr.c b/drivers/firmware/coreboot/coreboot-cfr.c
> new file mode 100644
> index 0000000000000..21b06aa363111
> --- /dev/null
> +++ b/drivers/firmware/coreboot/coreboot-cfr.c

[ ... ]

> +static int coreboot_cfr_write_value(struct coreboot_cfr_setting *setting,
> +				    u32 value)
> +{
> +	efi_char16_t *efi_name __free(kfree) = NULL;
> +	u32 attrs;
> +	u32 old;
> +	int restore_ret;
> +	int ret;
> +
> +	if (setting->read_only)
> +		return -EACCES;
> +
> +	efi_name = coreboot_cfr_efi_name(setting->name);
> +	if (IS_ERR(efi_name))
> +		return PTR_ERR(no_free_ptr(efi_name));
> +
> +	guard(mutex)(&setting->drvdata->lock);
> +
> +	ret = efivar_lock();
> +	if (ret)
> +		return ret;
> +
> +	ret = coreboot_cfr_read_efi_value(efi_name, &old, &attrs);
> +	if (ret)
> +		goto out_unlock_efi;

[ ... ]

> +
> +out_unlock_efi:
> +	efivar_unlock();
> +	return ret;
> +}

[Severity: Medium]
Does this mix goto-based error handling with scope-based cleanups in the same
function?

According to include/linux/cleanup.h, the expectation is that usage of goto
and cleanup helpers (like __free and guard) should never be mixed in the
same routine. Mixing them creates confusing resource ownership semantics and
increases the risk of future resource leaks.

Could this be unified to exclusively use either traditional goto labels or
scope-based cleanups? For instance, efivar_lock() could potentially use a
cleanup helper to avoid the goto out_unlock_efi labels entirely.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785226809.git.sean@starlabs.systems?part=4