lkml.org 
[lkml]   [2020]   [Apr]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 1/1] platform/x86: Add Slim Bootloader firmware update signaling driver
On Mon, Apr 20, 2020 at 10:50 PM Jithu Joseph <jithu.joseph@intel.com> wrote:
>
> Slim Bootloader(SBL) [1] is a small open-source boot firmware,
> designed for running on certain Intel platforms. SBL can be
> thought-of as fulfilling the role of a minimal BIOS
> implementation, i.e initializing the hardware and booting
> Operating System.
>
> Since SBL is not UEFI compliant, firmware update cannot be triggered
> using standard UEFI runtime services. Further considering performance
> impact, SBL doesn't look for a firmware update image on every reset
> and does so only when firmware update signal is asserted.
>
> SBL exposes an ACPI-WMI device which comes up in sysfs as
> /sys/bus/wmi/44FADEB1xxx and this driver adds a
> "firmware_update_request" device attribute. This attribute normally
> has a value of 0 and userspace can signal SBL to update firmware,
> on next reboot, by writing a value of 1:
>
> echo 1 > /sys/bus/wmi/devices/44FADEB1-B204-40F2-8581-394BBDC1B651/firmware_update_request
>
> This driver only implements a signaling mechanism, the actual firmware
> update process and various details like firmware update image format,
> firmware image location etc are defined by SBL [2] and are not in the
> scope of this driver.

I have noticed that it misses ABI documentation. So, please add. Also
some comments below.

...

> [1] https://slimbootloader.github.io
> [2] https://slimbootloader.github.io/security/firmware-update.html

Can you add a DocLink: tag below for the reference to the official
documentation?

...

> +SLIM BOOTLOADER (SBL) FIRMWARE UPDATE WMI DRIVER
> +M: Jithu Joseph <jithu.joseph@intel.com>
> +R: Maurice Ma <maurice.ma@intel.com>
> +S: Maintained
> +W: https://slimbootloader.github.io/security/firmware-update.html
> +F: drivers/platform/x86/sbl_fwu_wmi.c

I hope you run latest and greatest version of checkpatch.pl and it's
okay with this section.

...

> @@ -114,6 +114,16 @@ config XIAOMI_WMI
> To compile this driver as a module, choose M here: the module will
> be called xiaomi-wmi.
>
> +config SBL_FWU_WMI
> + tristate "WMI driver for Slim Bootloader firmware update signaling"
> + depends on ACPI_WMI
> + help
> + Say Y here if you want to be able to use the WMI interface to signal
> + Slim Bootloader to trigger update on next reboot.
> +
> + To compile this driver as a module, choose M here: the module will
> + be called sbl-fwu-wmi.

> @@ -15,6 +15,7 @@ obj-$(CONFIG_INTEL_WMI_THUNDERBOLT) += intel-wmi-thunderbolt.o
> obj-$(CONFIG_MXM_WMI) += mxm-wmi.o
> obj-$(CONFIG_PEAQ_WMI) += peaq-wmi.o
> obj-$(CONFIG_XIAOMI_WMI) += xiaomi-wmi.o
> +obj-$(CONFIG_SBL_FWU_WMI) += sbl_fwu_wmi.o

I didn't get an ordering schema in above files.
Shouldn't be rather alphasort?

...

> +static ssize_t firmware_update_request_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + bool val;
> + int ret;
> +
> + ret = kstrtobool(buf, &val);
> + if (ret)
> + return ret;
> +

> + ret = set_fwu_request(dev, val ? 1 : 0);

Hmm... If you are going to extend this, why not to pass integer
directly? (And thus take one from user)

> + if (ret)
> + return ret;
> +
> + return count;
> +}

> +

Extra blank line.

> +static DEVICE_ATTR_RW(firmware_update_request);
> +
> +static struct attribute *firmware_update_attrs[] = {
> + &dev_attr_firmware_update_request.attr,
> + NULL
> +};

> +

Extra blank line.

> +ATTRIBUTE_GROUPS(firmware_update);

...

> +MODULE_DEVICE_TABLE(wmi, sbl_fwu_wmi_id_table);

Move it closer to the table structure.

--
With Best Regards,
Andy Shevchenko

\
 
 \ /
  Last update: 2020-04-22 15:42    [W:0.051 / U:0.480 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site