| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 12 Jul 2021 19:46:05 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Mark Brown <broonie@...nel.org>, Rob Herring <robh+dt@...nel.org>,
Arnd Bergmann <arnd@...db.de>, Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>, Ian Ray <ian.ray@...com>,
linux-kernel@...r.kernel.org, linux-spi@...r.kernel.org,
devicetree@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCHv6 3/3] misc: gehc-achc: new driver
On Mon, Jul 12, 2021 at 05:02:42PM +0200, Sebastian Reichel wrote:
> General Electric Healthcare's PPD has a secondary processor from
> NXP's Kinetis K20 series. That device has two SPI chip selects:
>
> The main interface's behaviour depends on the loaded firmware
> and is currently unused.
>
> The secondary interface can be used to update the firmware using
> EzPort protocol. This is implemented by this driver using the
> kernel's firmware API. It's not done during probe time, since
> the device has non-volatile memory and flashing lasts almost 3
> minutes.
In thinking about this some more, why does it matter? Spin up a
workqueue when probing and do the firmware loading then. That way you
do not end up creating yet another custom user/kernel api just to do
something as trivial as loading the firmware for a device.
And I think the firmware loader even handles async loading, or at least
it used to, maybe not anymore, it's a complex api, I recommend reading
the docs...
>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@...labora.com>
> ---
> .../ABI/testing/sysfs-driver-ge-achc | 14 +
> drivers/misc/Kconfig | 11 +
> drivers/misc/Makefile | 2 +
> drivers/misc/gehc-achc.c | 136 +++++
> drivers/misc/nxp-ezport.c | 468 ++++++++++++++++++
> drivers/misc/nxp-ezport.h | 9 +
Why is there two different modules here for the same piece of hardware?
> drivers/spi/spidev.c | 1 -
> 7 files changed, 640 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-ge-achc
> create mode 100644 drivers/misc/gehc-achc.c
> create mode 100644 drivers/misc/nxp-ezport.c
> create mode 100644 drivers/misc/nxp-ezport.h
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-ge-achc b/Documentation/ABI/testing/sysfs-driver-ge-achc
> new file mode 100644
> index 000000000000..f02812731082
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-ge-achc
> @@ -0,0 +1,14 @@
> +What: /sys/bus/spi/<dev>/update_firmware
> +Date: Jul 2021
> +Contact: sebastian.reichel@...labora.com
> +Description: Write 1 to this file to update the ACHC microcontroller
> + firmware via the EzPort interface. For this the kernel
> + will load "achc.bin" via the firmware API (so usually
> + from /lib/firmware). The write will block until the FW
> + has either been flashed successfully or an error occured.
> +
> +What: /sys/bus/spi/<dev>/reset
> +Date: Jul 2021
> +Contact: sebastian.reichel@...labora.com
> +Description: Write 1 to this file to reset the microcontroller via the
> + reset GPIO. The write will block until the reset completes.
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index f4fb5c52b863..1ebf7000671d 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -208,6 +208,17 @@ config CS5535_CLOCK_EVENT_SRC
> MFGPTs have a better resolution and max interval than the
> generic PIT, and are suitable for use as high-res timers.
>
> +config GEHC_ACHC
> + tristate "GEHC ACHC support"
> + depends on SPI && SYSFS
> + select FW_LOADER
> + help
> + Support for GE ACHC microcontroller, that is part of the GE
> + PPD device.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called gehc-achc.
> +
> config HP_ILO
> tristate "Channel interface driver for the HP iLO processor"
> depends on PCI
> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> index e92a56d4442f..b50180796931 100644
> --- a/drivers/misc/Makefile
> +++ b/drivers/misc/Makefile
> @@ -24,6 +24,8 @@ obj-$(CONFIG_KGDB_TESTS) += kgdbts.o
> obj-$(CONFIG_SGI_XP) += sgi-xp/
> obj-$(CONFIG_SGI_GRU) += sgi-gru/
> obj-$(CONFIG_CS5535_MFGPT) += cs5535-mfgpt.o
> +obj-$(CONFIG_GEHC_ACHC) += gehc-achc-mod.o
> +gehc-achc-mod-objs := gehc-achc.o nxp-ezport.o
Yeah, you are creating 2 modules for one Kconfig option, why not put
these together into 1? Makes the code smaller.
> obj-$(CONFIG_HP_ILO) += hpilo.o
> obj-$(CONFIG_APDS9802ALS) += apds9802als.o
> obj-$(CONFIG_ISL29003) += isl29003.o
> diff --git a/drivers/misc/gehc-achc.c b/drivers/misc/gehc-achc.c
> new file mode 100644
> index 000000000000..893940e7ca4e
> --- /dev/null
> +++ b/drivers/misc/gehc-achc.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * datasheet: https://www.nxp.com/docs/en/data-sheet/K20P144M120SF3.pdf
> + *
> + * Copyright (C) 2018-2021 Collabora
> + * Copyright (C) 2018-2021 GE Healthcare
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/of.h>
> +#include "nxp-ezport.h"
> +
> +#define ACHC_MAX_FREQ_HZ 300000
> +
> +struct achc_data {
> + struct spi_device *main;
> + struct spi_device *ezport;
> + struct gpio_desc *reset;
> +
> + struct mutex device_lock; /* avoid concurrent device access */
> +};
> +
> +static ssize_t update_firmware_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct achc_data *achc = dev_get_drvdata(dev);
> + int ret;
> +
> + if (count != 1 || buf[0] != '1')
> + return -EINVAL;
There a core kernel call to see if the data written to a sysfs file is
"true/false" I would recommend, if you stick with this, to use that
instead.
thanks,
greg k-h
Powered by blists - more mailing lists