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

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

On Mon, Jul 13, 2026 at 09:13:17AM +0100, Sean Rhodes wrote:
> diff --git a/drivers/firmware/coreboot/coreboot-cfr.c b/drivers/firmware/coreboot/coreboot-cfr.c
...
> +static const struct coreboot_table_entry *
> +coreboot_cfr_child_entry(const void *base, size_t len, u32 tag)
> +{
> +	const struct coreboot_table_entry *entry;
> +	size_t off = 0;
> +
> +	while (off < len) {

To be concise,

    for (off = 0; off < len; off += entry->size)

> +static bool coreboot_cfr_value_is_valid(struct coreboot_cfr_setting *setting,
> +					u32 value)
> +{
> +	u32 delta;
> +
> +	if (setting->type != COREBOOT_CFR_SETTING_NUMBER)
> +		return coreboot_cfr_label_from_value(setting, value);

This deserves a comment as coreboot_cfr_label_from_value() returns a `char *`
instead of bool.

> +static ssize_t possible_values_show(struct kobject *kobj,
> +				    struct kobj_attribute *attr, char *buf)
> +{
> +	struct coreboot_cfr_setting *setting = to_coreboot_cfr_setting(kobj);
> +	ssize_t len = 0;
> +	unsigned int i;
> +
> +	for (i = 0; i < setting->n_values; i++) {
> +		len += sysfs_emit_at(buf, len, "%s%s", i ? ";" : "",
> +				     setting->values[i].label);
> +		if (len >= PAGE_SIZE)
> +			return len;

Does it really need the check?

> +static int coreboot_cfr_count_enum_values(const void *base, size_t len)
> +{
> +	const struct coreboot_table_entry *entry;
> +	size_t off = 0;
> +	int count = 0;
> +
> +	while (off < len) {

To be consie,

    for (off = 0; off < len; off += entry->size)

> +static int coreboot_cfr_copy_enum_values(struct coreboot_cfr_setting *setting,
> +					 const void *base, size_t len)
> +{
> +	const struct lb_cfr_enum_value *enum_value;
> +	const struct lb_cfr_varbinary *label;
> +	const struct coreboot_table_entry *entry;
> +	size_t off = 0;
> +	int count;
> +
> +	count = coreboot_cfr_count_enum_values(base, len);
> +	if (count <= 0)
> +		return count ?: -EINVAL;
> +
> +	setting->values = kcalloc(count, sizeof(*setting->values), GFP_KERNEL);
> +	if (!setting->values)
> +		return -ENOMEM;
> +
> +	while (off < len) {

To be consie,

    for (off = 0; off < len; off += entry->size)

> +static int coreboot_cfr_parse_records(struct coreboot_cfr_drvdata *data,
> +				      const void *base, size_t len)
> +{
> +	const struct coreboot_table_entry *entry;
> +	const void *child_base;
> +	size_t child_len;
> +	size_t off = 0;
> +	int ret;
> +
> +	while (off < len) {

To be concise,

    for (off = 0; off < len; off += entry->size)

> +		switch (entry->tag) {
> +		case CFR_TAG_OPTION_FORM:
> +			if (entry->size < sizeof(struct lb_cfr_option_form))
> +				return -EINVAL;
> +
> +			child_base = base + off + sizeof(struct lb_cfr_option_form);
> +			child_len = entry->size - sizeof(struct lb_cfr_option_form);
> +			ret = coreboot_cfr_parse_records(data, child_base,
> +							 child_len);
> +			if (ret)
> +				return ret;
> +			break;
> +		case CFR_TAG_OPTION_ENUM:
> +		case CFR_TAG_OPTION_NUMBER:
> +		case CFR_TAG_OPTION_BOOL:
> +			if (entry->size < sizeof(struct lb_cfr_numeric_option))
> +				return -EINVAL;
> +			ret = coreboot_cfr_add_numeric_option(data, base + off);
> +			if (ret)
> +				return ret;
> +			break;
> +		default:
> +			break;

Doesn't it need to do anything if sees unknown `entry->tag`?

> +static int coreboot_cfr_probe(struct coreboot_device *dev)
> +{
...
> +	data->class_dev = device_create(&firmware_attributes_class, NULL,
> +					MKDEV(0, 0), NULL, DRIVER_NAME);
> +	if (IS_ERR(data->class_dev)) {
> +		ret = PTR_ERR(data->class_dev);
> +		goto err_clear_data;

No more data to clear,

    return PTR_ERR(...);

> +err_clear_data:
> +	return ret;

Nothing to do other than a return.  The label can be removed.