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

[Tzung-Bi Shih]

On Fri, Jul 10, 2026 at 11:40:24AM -0500, Sean Rhodes wrote:
> diff --git a/drivers/firmware/coreboot/coreboot-cfr.c
...
> +#include <linux/array_size.h>
> +#include <linux/bitops.h>
> +#include <linux/cleanup.h>
> +#include <linux/ctype.h>
> +#include <linux/efi.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kdev_t.h>
> +#include <linux/kobject.h>
> +#include <linux/limits.h>
> +#include <linux/list.h>
> +#include <linux/device-id/coreboot.h>

Keep the list sorted.

> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>
> +
> +#include <linux/firmware_attributes.h>

Why this needs to be separated from the above list?

> +struct coreboot_cfr_drvdata {
> +	struct device *class_dev;
> +	struct kset *attrs_kset;
> +	struct list_head settings;
> +	/* Serializes EFI variable writes and the matching runtime apply hook. */
> +	struct mutex lock;
> +	bool pending_reboot;
> +};
> +
> +static struct coreboot_cfr_drvdata *coreboot_cfr_data;

Can't this be some drvdata so that it doesn't need to be global?

> +static char *coreboot_cfr_string_dup(const struct lb_cfr_varbinary *str)
> +{
> +	const char *data = (const char *)str + sizeof(*str);

(const char *)(str + 1)?  As the patch uses similar pattern otherwhere.