| 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
| ||
|
Message-ID: <20210528140654.ffhetiikhde6seuo@earth.universe>
Date: Fri, 28 May 2021 16:06:54 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
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: [PATCHv3 5/5] misc: gehc-achc: new driver
Hi Greg,
On Fri, May 28, 2021 at 02:19:08PM +0200, Greg Kroah-Hartman wrote:
> On Fri, May 28, 2021 at 01:33:47PM +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 exposed to userspace (as before).
> >
> > 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.
> >
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@...labora.com>
> > ---
> > drivers/misc/Kconfig | 15 +
> > drivers/misc/Makefile | 2 +
> > drivers/misc/gehc-achc.c | 160 ++++++++
> > drivers/misc/nxp-ezport.c | 476 +++++++++++++++++++++++
> > drivers/spi/spidev.c | 7 +-
> > include/linux/platform_data/nxp-ezport.h | 9 +
> > include/linux/spi/spi.h | 5 +
> > 7 files changed, 671 insertions(+), 3 deletions(-)
> > create mode 100644 drivers/misc/gehc-achc.c
> > create mode 100644 drivers/misc/nxp-ezport.c
> > create mode 100644 include/linux/platform_data/nxp-ezport.h
> >
> > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > index f4fb5c52b863..f058b551a7b1 100644
> > --- a/drivers/misc/Kconfig
> > +++ b/drivers/misc/Kconfig
> > @@ -208,6 +208,21 @@ 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 NXP_EZPORT
> > + tristate
> > +
> > +config GEHC_ACHC
> > + tristate "GEHC ACHC support"
> > + depends on SPI && SYSFS && SPI_SPIDEV
> > + select FW_LOADER
> > + select NXP_EZPORT
> > + help
> > + Support for GE ACHC microcontroller, that is part of the GE
> > + CS One 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..b7bad5a16c8f 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_NXP_EZPORT) += nxp-ezport.o
> > +obj-$(CONFIG_GEHC_ACHC) += gehc-achc.o
> > 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..7856dd70a80c
> > --- /dev/null
> > +++ b/drivers/misc/gehc-achc.c
> > @@ -0,0 +1,160 @@
> > +// SPDX-License-Identifier: GPL-2.0+
>
> Are you _sure_ you mean "+"? I have to ask :)
Yes, but I'm also fine with 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 <linux/platform_data/nxp-ezport.h>
> > +
> > +#define ACHC_MAX_FREQ 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)
> > + return -EINVAL;
> > +
> > + mutex_lock(&achc->device_lock);
> > + ret = ezport_flash(achc->ezport, achc->reset, "achc.bin");
> > + mutex_unlock(&achc->device_lock);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + return count;
> > +}
> > +static DEVICE_ATTR_WO(update_firmware);
>
> You add new sysfs files, yet forget to document them in
> Documentation/ABI/ Please fix up for your next submission.
Ack.
> > +
> > +static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct achc_data *achc = dev_get_drvdata(dev);
> > +
> > + if (!count)
> > + return -EINVAL;
> > +
> > + mutex_lock(&achc->device_lock);
> > + ezport_reset(achc->reset);
> > + mutex_unlock(&achc->device_lock);
> > +
> > + return count;
> > +}
> > +static DEVICE_ATTR_WO(reset);
> > +
> > +static struct attribute *gehc_achc_attrs[] = {
> > + &dev_attr_update_firmware.attr,
> > + &dev_attr_reset.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group gehc_achc_attr_group = {
> > + .attrs = gehc_achc_attrs,
> > +};
> > +
> > +static void unregister_ezport(void *data)
> > +{
> > + struct spi_device *ezport = data;
> > +
> > + spi_unregister_device(ezport);
> > +}
> > +
> > +static int gehc_achc_probe(struct spi_device *spi)
> > +{
> > + struct achc_data *achc;
> > + int ezport_reg, ret;
> > +
> > + spi->max_speed_hz = ACHC_MAX_FREQ;
> > + spi->bits_per_word = 8;
> > + spi->mode = SPI_MODE_0;
> > +
> > + achc = devm_kzalloc(&spi->dev, sizeof(*achc), GFP_KERNEL);
> > + if (!achc)
> > + return -ENOMEM;
> > + achc->main = spi;
> > +
> > + mutex_init(&achc->device_lock);
> > +
> > + ret = of_property_read_u32_index(spi->dev.of_node, "reg", 1, &ezport_reg);
> > + if (ret)
> > + return dev_err_probe(&spi->dev, ret, "missing second reg entry!\n");
> > +
> > + achc->ezport = spi_new_ancillary_device(spi, ezport_reg);
> > + if (IS_ERR(achc->ezport))
> > + return PTR_ERR(achc->ezport);
> > +
> > + ret = devm_add_action_or_reset(&spi->dev, unregister_ezport, achc->ezport);
> > + if (ret)
> > + return ret;
> > +
> > + achc->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
> > + if (IS_ERR(achc->reset))
> > + return dev_err_probe(&spi->dev, PTR_ERR(achc->reset), "Could not get reset gpio\n");
> > +
> > + /*
> > + * The sysfs properties are bound to the dummy device, since the main device already
> > + * uses drvdata assigned by the spidev driver.
> > + */
> > + spi_set_drvdata(achc->ezport, achc);
> > + ret = devm_device_add_group(&achc->ezport->dev, &gehc_achc_attr_group);
>
> You just raced and lost. Please use the default groups attribute for
> your driver instead of this. Or properly attach it to the device some
> other way, but what you have done here does not work, sorry.
I've been told the race got fixed from kernel POV?
https://lore.kernel.org/linux-input/20200521055400.GX89269@dtor-ws/
If this is still an issue, I think most (all?) existing instances of
devm_device_add_group() are a problem and it makes sense to have a
checkpatch warning for it.
> And you should probably break this up into at least 2 patches, one for
> each "driver" you are adding.
I expect, that Mark wants this to be restructured anyways. If
it stays in its current form I will split this into three patches
1) export spidev probe/remove
2) add ezport support module
3) add gehc-achc driver gluing both together
Thanks for the review.
-- Sebastian
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists