| Message ID | 20240424173809.7214-7-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Turris Omnia MCU driver | expand |
On Wed, Apr 24, 2024 at 07:38:05PM +0200, Marek Behún wrote: > Add support for true random number generator provided by the MCU. > New Omnia boards come without the Atmel SHA204-A chip. Instead the > crypto functionality is provided by new microcontroller, which has > a TRNG peripheral. ... > +static void omnia_irq_mapping_drop(void *res) > +{ > + irq_dispose_mapping((unsigned int)(unsigned long)res); > +} Leftover? > +int omnia_mcu_register_trng(struct omnia_mcu *mcu) > +{ > + struct device *dev = &mcu->client->dev; > + u8 irq_idx, dummy; > + int irq, err; > + > + if (!(mcu->features & FEAT_TRNG)) > + return 0; > + > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)]; > + irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx)); > + if (irq < 0) > + return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n"); > + err = devm_add_action_or_reset(dev, omnia_irq_mapping_drop, > + (void *)(unsigned long)irq); > + if (err) > + return err; Are you sure it's correct now? > + /* If someone else cleared the TRNG interrupt but did not read the > + * entropy, a new interrupt won't be generated, and entropy collection > + * will be stuck. Ensure an interrupt will be generated by executing > + * the collect entropy command (and discarding the result). > + */ > + err = omnia_cmd_read(mcu->client, CMD_TRNG_COLLECT_ENTROPY, &dummy, 1); > + if (err) > + return err; > + > + init_completion(&mcu->trng_completion); > + > + err = devm_request_threaded_irq(dev, irq, NULL, omnia_trng_irq_handler, > + IRQF_ONESHOT, "turris-omnia-mcu-trng", > + mcu); > + if (err) > + return dev_err_probe(dev, err, "Cannot request TRNG IRQ\n"); > + > + mcu->trng.name = "turris-omnia-mcu-trng"; > + mcu->trng.read = omnia_trng_read; > + mcu->trng.priv = (unsigned long)mcu; > + > + err = devm_hwrng_register(dev, &mcu->trng); > + if (err) > + return dev_err_probe(dev, err, "Cannot register TRNG\n"); > + > + return 0; > +}
On Wed, Apr 24, 2024 at 07:38:05PM +0200, Marek Behún wrote: > Add support for true random number generator provided by the MCU. > New Omnia boards come without the Atmel SHA204-A chip. Instead the > crypto functionality is provided by new microcontroller, which has > a TRNG peripheral. ... > + /* If someone else cleared the TRNG interrupt but did not read the > + * entropy, a new interrupt won't be generated, and entropy collection > + * will be stuck. Ensure an interrupt will be generated by executing > + * the collect entropy command (and discarding the result). > + */ /* * Note, the above style is solely for net subsystem. The * others should use the usual one, like in this example. */
On Wed, 24 Apr 2024 21:33:44 +0300 Andy Shevchenko <andy@kernel.org> wrote: > On Wed, Apr 24, 2024 at 07:38:05PM +0200, Marek Behún wrote: > > Add support for true random number generator provided by the MCU. > > New Omnia boards come without the Atmel SHA204-A chip. Instead the > > crypto functionality is provided by new microcontroller, which has > > a TRNG peripheral. > > ... > > > +static void omnia_irq_mapping_drop(void *res) > > +{ > > + irq_dispose_mapping((unsigned int)(unsigned long)res); > > +} > > Leftover? What do you mean? I dropped the devm-helpers.h changes, now I do devm_add_action_or_reset() manually, with this function as the action. > > +int omnia_mcu_register_trng(struct omnia_mcu *mcu) > > +{ > > + struct device *dev = &mcu->client->dev; > > + u8 irq_idx, dummy; > > + int irq, err; > > + > > + if (!(mcu->features & FEAT_TRNG)) > > + return 0; > > + > > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)]; > > + irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx)); > > + if (irq < 0) > > + return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n"); > > > + err = devm_add_action_or_reset(dev, omnia_irq_mapping_drop, > > + (void *)(unsigned long)irq); > > + if (err) > > + return err; > > Are you sure it's correct now? Yes, why wouldn't it?
On Wed, Apr 24, 2024 at 08:51:23PM +0200, Marek Behún wrote: > On Wed, 24 Apr 2024 21:33:44 +0300 > Andy Shevchenko <andy@kernel.org> wrote: > > On Wed, Apr 24, 2024 at 07:38:05PM +0200, Marek Behún wrote: ... > > > +static void omnia_irq_mapping_drop(void *res) > > > +{ > > > + irq_dispose_mapping((unsigned int)(unsigned long)res); > > > +} > > > > Leftover? > > What do you mean? I dropped the devm-helpers.h changes, now I do > devm_add_action_or_reset() manually, with this function as the action. But why? ... > > > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)]; > > > + irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx)); > > > + if (irq < 0) > > > + return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n"); > > > + err = devm_add_action_or_reset(dev, omnia_irq_mapping_drop, > > > + (void *)(unsigned long)irq); > > > + if (err) > > > + return err; > > > > Are you sure it's correct now? > > Yes, why wouldn't it? For what purpose? I don't see drivers doing that. Are you expecting that the same IRQ mapping will be reused for something else? Can you elaborate how? (I can imagine one theoretical / weird case how to achieve that, but impractical.) Besides above, this is asymmetrical call to gpiod_to_irq(). If we really care about this, it should be provided by GPIO library.
On Wed, 24 Apr 2024 22:47:10 +0300 Andy Shevchenko <andy@kernel.org> wrote: > On Wed, Apr 24, 2024 at 08:51:23PM +0200, Marek Behún wrote: > > On Wed, 24 Apr 2024 21:33:44 +0300 > > Andy Shevchenko <andy@kernel.org> wrote: > > > On Wed, Apr 24, 2024 at 07:38:05PM +0200, Marek Behún wrote: > > ... > > > > > +static void omnia_irq_mapping_drop(void *res) > > > > +{ > > > > + irq_dispose_mapping((unsigned int)(unsigned long)res); > > > > +} > > > > > > Leftover? > > > > What do you mean? I dropped the devm-helpers.h changes, now I do > > devm_add_action_or_reset() manually, with this function as the action. > > But why? > > ... > > > > > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)]; > > > > + irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx)); > > > > + if (irq < 0) > > > > + return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n"); > > > > > + err = devm_add_action_or_reset(dev, omnia_irq_mapping_drop, > > > > + (void *)(unsigned long)irq); > > > > + if (err) > > > > + return err; > > > > > > Are you sure it's correct now? > > > > Yes, why wouldn't it? > > For what purpose? I don't see drivers doing that. Are you expecting that > the same IRQ mapping will be reused for something else? Can you elaborate > how? (I can imagine one theoretical / weird case how to achieve that, > but impractical.) I do a lot of binding/unbinding of that driver. I was under the impression that all resources should be dropped on driver unbind. > Besides above, this is asymmetrical call to gpiod_to_irq(). If we really care > about this, it should be provided by GPIO library. > Something like the following? From 5aac93d55f6fb750726f7e879672142956981a4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Beh=C3=BAn?= <kabel@kernel.org> Date: Thu, 25 Apr 2024 11:33:33 +0200 Subject: [PATCH] gpiolib: devres: Add resource managed version of gpiod_to_irq() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add devm_gpiod_to_irq(), a resource managed version of gpiod_to_irq(). The release function calls irq_dispose_mapping(). Signed-off-by: Marek Behún <kabel@kernel.org> --- drivers/gpio/gpiolib-devres.c | 27 +++++++++++++++++++++++++++ include/linux/gpio/consumer.h | 10 ++++++++++ 2 files changed, 37 insertions(+) diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c index 4987e62dcb3d..98a40492e596 100644 --- a/drivers/gpio/gpiolib-devres.c +++ b/drivers/gpio/gpiolib-devres.c @@ -12,6 +12,7 @@ #include <linux/gpio/consumer.h> #include <linux/device.h> #include <linux/gfp.h> +#include <linux/irqdomain.h> #include "gpiolib.h" @@ -427,3 +428,29 @@ int devm_gpiochip_add_data_with_key(struct device *dev, struct gpio_chip *gc, vo return devm_add_action_or_reset(dev, devm_gpio_chip_release, gc); } EXPORT_SYMBOL_GPL(devm_gpiochip_add_data_with_key); + +static void devm_gpiod_irq_release(void *data) +{ + irq_dispose_mapping((unsigned int)(unsigned long)data); +} + +/** + * devm_gpiod_to_irq() - Resource managed devm_gpiod_to_irq() + * @dev: pointer to the device that gpio_chip belongs to. + * @desc: gpio whose IRQ will be returned + * + * Return the IRQ corresponding to the passed GPIO, or an error code in case of + * error. + */ +int devm_gpiod_to_irq(struct device *dev, const struct gpio_desc *desc) +{ + int virq; + + virq = gpiod_to_irq(desc); + if (virq < 0) + return virq; + + return devm_add_action_or_reset(dev, devm_gpiod_irq_release, + (void *)(unsigned long)virq); +} +EXPORT_SYMBOL_GPL(devm_gpiod_to_irq); diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index db2dfbae8edb..e8f4829538f6 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -165,6 +165,8 @@ int gpiod_is_active_low(const struct gpio_desc *desc); int gpiod_cansleep(const struct gpio_desc *desc); int gpiod_to_irq(const struct gpio_desc *desc); +int devm_gpiod_to_irq(struct device *dev, const struct gpio_desc *desc); + int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name); /* Convert between the old gpio_ and new gpiod_ interfaces */ @@ -519,6 +521,14 @@ static inline int gpiod_to_irq(const struct gpio_desc *desc) return -EINVAL; } +static inline int devm_gpiod_to_irq(struct device *dev, + const struct gpio_desc *desc) +{ + /* GPIO can never have been requested */ + WARN_ON(desc); + return -EINVAL; +} + static inline int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name) {
On Wed, Apr 24, 2024 at 10:47:11PM +0300, Andy Shevchenko wrote: > On Wed, Apr 24, 2024 at 08:51:23PM +0200, Marek Behún wrote: > > On Wed, 24 Apr 2024 21:33:44 +0300 > > Andy Shevchenko <andy@kernel.org> wrote: > > > On Wed, Apr 24, 2024 at 07:38:05PM +0200, Marek Behún wrote: ... > > > > +static void omnia_irq_mapping_drop(void *res) > > > > +{ > > > > + irq_dispose_mapping((unsigned int)(unsigned long)res); > > > > +} > > > > > > Leftover? > > > > What do you mean? I dropped the devm-helpers.h changes, now I do > > devm_add_action_or_reset() manually, with this function as the action. > > But why? ... > > > > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)]; > > > > + irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx)); > > > > + if (irq < 0) > > > > + return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n"); > > > > > + err = devm_add_action_or_reset(dev, omnia_irq_mapping_drop, > > > > + (void *)(unsigned long)irq); > > > > + if (err) > > > > + return err; > > > > > > Are you sure it's correct now? > > > > Yes, why wouldn't it? > > For what purpose? I don't see drivers doing that. Are you expecting that > the same IRQ mapping will be reused for something else? Can you elaborate > how? (I can imagine one theoretical / weird case how to achieve that, > but impractical.) > > Besides above, this is asymmetrical call to gpiod_to_irq(). If we really care > about this, it should be provided by GPIO library. FWIW, gpiochip_irqchip_remove() disposes mappings for internally allocated IRQ domains. So with your code it even might be a double-disposal.
On Thu, Apr 25, 2024 at 11:34:47AM +0200, Marek Behún wrote: > On Wed, 24 Apr 2024 22:47:10 +0300 > Andy Shevchenko <andy@kernel.org> wrote: > > On Wed, Apr 24, 2024 at 08:51:23PM +0200, Marek Behún wrote: ... > > For what purpose? I don't see drivers doing that. Are you expecting that > > the same IRQ mapping will be reused for something else? Can you elaborate > > how? (I can imagine one theoretical / weird case how to achieve that, > > but impractical.) > > I do a lot of binding/unbinding of that driver. I was under the > impression that all resources should be dropped on driver unbind. > > > Besides above, this is asymmetrical call to gpiod_to_irq(). If we really care > > about this, it should be provided by GPIO library. > > Something like the following? Not needed. IRQ mappings are per domain, and GPIO chip has its own associated with the respective lifetime, AFAIU when you remove the GPIO chip, all mappings will be disposed (as I pointed out in previous mail).
On Thu, 25 Apr 2024 13:04:07 +0300 Andy Shevchenko <andy@kernel.org> wrote: > On Thu, Apr 25, 2024 at 11:34:47AM +0200, Marek Behún wrote: > > On Wed, 24 Apr 2024 22:47:10 +0300 > > Andy Shevchenko <andy@kernel.org> wrote: > > > On Wed, Apr 24, 2024 at 08:51:23PM +0200, Marek Behún wrote: > > ... > > > > For what purpose? I don't see drivers doing that. Are you expecting that > > > the same IRQ mapping will be reused for something else? Can you elaborate > > > how? (I can imagine one theoretical / weird case how to achieve that, > > > but impractical.) > > > > I do a lot of binding/unbinding of that driver. I was under the > > impression that all resources should be dropped on driver unbind. > > > > > Besides above, this is asymmetrical call to gpiod_to_irq(). If we really care > > > about this, it should be provided by GPIO library. > > > > Something like the following? > > Not needed. IRQ mappings are per domain, and GPIO chip has its own associated > with the respective lifetime, AFAIU when you remove the GPIO chip, all mappings > will be disposed (as I pointed out in previous mail). > OMG you are right :) of course. OK, I shall drop this. Marek
diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig index e2649cdecc38..750d5f47dba8 100644 --- a/drivers/platform/cznic/Kconfig +++ b/drivers/platform/cznic/Kconfig @@ -19,6 +19,7 @@ config TURRIS_OMNIA_MCU depends on I2C select GPIOLIB select GPIOLIB_IRQCHIP + select HW_RANDOM select RTC_CLASS select WATCHDOG_CORE help @@ -28,6 +29,7 @@ config TURRIS_OMNIA_MCU - board poweroff into true low power mode (with voltage regulators disabled) and the ability to configure wake up from this mode (via rtcwake) + - true random number generator (if available on the MCU) - MCU watchdog - GPIO pins - to get front button press events (the front button can be diff --git a/drivers/platform/cznic/Makefile b/drivers/platform/cznic/Makefile index 687f7718c0a1..eae4c6b341ff 100644 --- a/drivers/platform/cznic/Makefile +++ b/drivers/platform/cznic/Makefile @@ -4,4 +4,5 @@ obj-$(CONFIG_TURRIS_OMNIA_MCU) += turris-omnia-mcu.o turris-omnia-mcu-y := turris-omnia-mcu-base.o turris-omnia-mcu-y += turris-omnia-mcu-gpio.o turris-omnia-mcu-y += turris-omnia-mcu-sys-off-wakeup.o +turris-omnia-mcu-y += turris-omnia-mcu-trng.o turris-omnia-mcu-y += turris-omnia-mcu-watchdog.o diff --git a/drivers/platform/cznic/turris-omnia-mcu-base.c b/drivers/platform/cznic/turris-omnia-mcu-base.c index 5f88119d825c..7fe4a3df93a6 100644 --- a/drivers/platform/cznic/turris-omnia-mcu-base.c +++ b/drivers/platform/cznic/turris-omnia-mcu-base.c @@ -369,7 +369,11 @@ static int omnia_mcu_probe(struct i2c_client *client) if (err) return err; - return omnia_mcu_register_gpiochip(mcu); + err = omnia_mcu_register_gpiochip(mcu); + if (err) + return err; + + return omnia_mcu_register_trng(mcu); } static const struct of_device_id of_omnia_mcu_match[] = { diff --git a/drivers/platform/cznic/turris-omnia-mcu-gpio.c b/drivers/platform/cznic/turris-omnia-mcu-gpio.c index b3b203f0d2b9..625018ca82cc 100644 --- a/drivers/platform/cznic/turris-omnia-mcu-gpio.c +++ b/drivers/platform/cznic/turris-omnia-mcu-gpio.c @@ -162,7 +162,7 @@ static const struct omnia_gpio { }; /* mapping from interrupts to indexes of GPIOs in the omnia_gpios array */ -static const u8 omnia_int_to_gpio_idx[32] = { +const u8 omnia_int_to_gpio_idx[32] = { [__bf_shf(INT_CARD_DET)] = 4, [__bf_shf(INT_MSATA_IND)] = 5, [__bf_shf(INT_USB30_OVC)] = 6, diff --git a/drivers/platform/cznic/turris-omnia-mcu-trng.c b/drivers/platform/cznic/turris-omnia-mcu-trng.c new file mode 100644 index 000000000000..a9ac36b570e9 --- /dev/null +++ b/drivers/platform/cznic/turris-omnia-mcu-trng.c @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * CZ.NIC's Turris Omnia MCU TRNG driver + * + * 2024 by Marek Behún <kabel@kernel.org> + */ + +#include <linux/bitfield.h> +#include <linux/completion.h> +#include <linux/gpio/consumer.h> +#include <linux/gpio/driver.h> +#include <linux/i2c.h> +#include <linux/irqdomain.h> +#include <linux/minmax.h> +#include <linux/module.h> +#include <linux/string.h> +#include <linux/turris-omnia-mcu-interface.h> +#include <linux/types.h> + +#include "turris-omnia-mcu.h" + +#define CMD_TRNG_MAX_ENTROPY_LEN 64 + +static irqreturn_t omnia_trng_irq_handler(int irq, void *dev_id) +{ + struct omnia_mcu *mcu = dev_id; + + complete(&mcu->trng_completion); + + return IRQ_HANDLED; +} + +static int omnia_trng_read(struct hwrng *rng, void *data, size_t max, bool wait) +{ + struct omnia_mcu *mcu = (struct omnia_mcu *)rng->priv; + u8 reply[1 + CMD_TRNG_MAX_ENTROPY_LEN]; + int err, bytes; + + if (!wait && !completion_done(&mcu->trng_completion)) + return 0; + + do { + if (wait_for_completion_interruptible(&mcu->trng_completion)) + return -EINTR; + + err = omnia_cmd_read(mcu->client, CMD_TRNG_COLLECT_ENTROPY, + reply, sizeof(reply)); + if (err) + return err; + + bytes = min3(reply[0], max, CMD_TRNG_MAX_ENTROPY_LEN); + } while (wait && !bytes); + + memcpy(data, &reply[1], bytes); + + return bytes; +} + +static void omnia_irq_mapping_drop(void *res) +{ + irq_dispose_mapping((unsigned int)(unsigned long)res); +} + +int omnia_mcu_register_trng(struct omnia_mcu *mcu) +{ + struct device *dev = &mcu->client->dev; + u8 irq_idx, dummy; + int irq, err; + + if (!(mcu->features & FEAT_TRNG)) + return 0; + + irq_idx = omnia_int_to_gpio_idx[__bf_shf(INT_TRNG)]; + irq = gpiod_to_irq(gpiochip_get_desc(&mcu->gc, irq_idx)); + if (irq < 0) + return dev_err_probe(dev, irq, "Cannot get TRNG IRQ\n"); + + err = devm_add_action_or_reset(dev, omnia_irq_mapping_drop, + (void *)(unsigned long)irq); + if (err) + return err; + + /* If someone else cleared the TRNG interrupt but did not read the + * entropy, a new interrupt won't be generated, and entropy collection + * will be stuck. Ensure an interrupt will be generated by executing + * the collect entropy command (and discarding the result). + */ + err = omnia_cmd_read(mcu->client, CMD_TRNG_COLLECT_ENTROPY, &dummy, 1); + if (err) + return err; + + init_completion(&mcu->trng_completion); + + err = devm_request_threaded_irq(dev, irq, NULL, omnia_trng_irq_handler, + IRQF_ONESHOT, "turris-omnia-mcu-trng", + mcu); + if (err) + return dev_err_probe(dev, err, "Cannot request TRNG IRQ\n"); + + mcu->trng.name = "turris-omnia-mcu-trng"; + mcu->trng.read = omnia_trng_read; + mcu->trng.priv = (unsigned long)mcu; + + err = devm_hwrng_register(dev, &mcu->trng); + if (err) + return dev_err_probe(dev, err, "Cannot register TRNG\n"); + + return 0; +} diff --git a/drivers/platform/cznic/turris-omnia-mcu.h b/drivers/platform/cznic/turris-omnia-mcu.h index 1838cb3d636e..e0cf10f8c32e 100644 --- a/drivers/platform/cznic/turris-omnia-mcu.h +++ b/drivers/platform/cznic/turris-omnia-mcu.h @@ -9,7 +9,9 @@ #define __TURRIS_OMNIA_MCU_H #include <linux/bitops.h> +#include <linux/completion.h> #include <linux/gpio/driver.h> +#include <linux/hw_random.h> #include <linux/if_ether.h> #include <linux/mutex.h> #include <linux/rtc.h> @@ -45,6 +47,10 @@ struct omnia_mcu { /* MCU watchdog */ struct watchdog_device wdt; + + /* true random number generator */ + struct hwrng trng; + struct completion trng_completion; }; int omnia_cmd_write_read(const struct i2c_client *client, @@ -147,11 +153,13 @@ static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd) return err ?: reply; } +extern const u8 omnia_int_to_gpio_idx[32]; extern const struct attribute_group omnia_mcu_gpio_group; extern const struct attribute_group omnia_mcu_poweroff_group; int omnia_mcu_register_gpiochip(struct omnia_mcu *mcu); int omnia_mcu_register_sys_off_and_wakeup(struct omnia_mcu *mcu); +int omnia_mcu_register_trng(struct omnia_mcu *mcu); int omnia_mcu_register_watchdog(struct omnia_mcu *mcu); #endif /* __TURRIS_OMNIA_MCU_H */
Add support for true random number generator provided by the MCU. New Omnia boards come without the Atmel SHA204-A chip. Instead the crypto functionality is provided by new microcontroller, which has a TRNG peripheral. Signed-off-by: Marek Behún <kabel@kernel.org> --- drivers/platform/cznic/Kconfig | 2 + drivers/platform/cznic/Makefile | 1 + .../platform/cznic/turris-omnia-mcu-base.c | 6 +- .../platform/cznic/turris-omnia-mcu-gpio.c | 2 +- .../platform/cznic/turris-omnia-mcu-trng.c | 109 ++++++++++++++++++ drivers/platform/cznic/turris-omnia-mcu.h | 8 ++ 6 files changed, 126 insertions(+), 2 deletions(-) create mode 100644 drivers/platform/cznic/turris-omnia-mcu-trng.c