| Message ID | 20240605161851.13911-7-kabel@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Turris Omnia MCU driver | expand |
On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> 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. +Cc: Bart for gpiochip_get_desc() usage. ... > +#include <linux/bitfield.h> > +#include <linux/completion.h> + errno.h > +#include <linux/gpio/consumer.h> > +#include <linux/gpio/driver.h> > +#include <linux/hw_random.h> > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/minmax.h> > +#include <linux/module.h> > +#include <linux/string.h> > +#include <linux/turris-omnia-mcu-interface.h> As per other patches. > +#include <linux/types.h> > + > +#include "turris-omnia-mcu.h" ... > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_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"); Okay, it's a bit more complicated than that. The gpiochip_get_desc() shouldn't be used. Bart, what can you suggest to do here? Opencoding it doesn't sound to me a (fully) correct approach in a long term. -- With Best Regards, Andy Shevchenko
On Wed, 5 Jun 2024 22:00:20 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_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"); > > Okay, it's a bit more complicated than that. The gpiochip_get_desc() > shouldn't be used. Bart, what can you suggest to do here? Opencoding > it doesn't sound to me a (fully) correct approach in a long term. Note that I can't use gpiochip_request_own_desc(), nor any other function that calls gpio_request_commit() (like gpiod_get()), because that checks for gpiochip_line_is_valid(), and this returns false for the TRNG line, cause that line is not a GPIO line, but interrupt only line. That is why I used irq = irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx); until v7, with no reference to gpio descriptors, since this line is not a GPIO line. We have discussed this back in April, in the thread https://lore.kernel.org/soc/20240418121116.22184-8-kabel@kernel.org/ where we concluded that irq = gpiod_to_irq(gpiochip_get_desc(gc, irq_idx)); is better... Marek
On Wed, 5 Jun 2024 22:00:20 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > +#include <linux/bitfield.h> > > +#include <linux/completion.h> > > + errno.h I use -EIO, -EINVAL and -ENOMEM in turris-omnia-mcu-base.c, -EINVAL, -ENOTSUPP in turris-omnia-mcu-gpio.c. Should I include errno.h in those also? Or is this only needed for -ERESTARTSYS? Marek
On Thu, Jun 06, 2024 at 11:11:13AM +0200, Marek Behún wrote: > On Wed, 5 Jun 2024 22:00:20 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > +#include <linux/bitfield.h> > > > +#include <linux/completion.h> > > > > + errno.h > > I use -EIO, -EINVAL and -ENOMEM in turris-omnia-mcu-base.c, > -EINVAL, -ENOTSUPP in turris-omnia-mcu-gpio.c. > Should I include errno.h in those also? If you have err.h, then no (it includes asm/errno.h), otherwise, yes. > Or is this only needed for -ERESTARTSYS? Definitely yes for Linux internal error codes (>= 512). Note, that ENOTSUPP is also Linux internal code.
On Thu, Jun 06, 2024 at 10:53:08AM +0200, Marek Behún wrote: > On Wed, 5 Jun 2024 22:00:20 +0300 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_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"); > > > > Okay, it's a bit more complicated than that. The gpiochip_get_desc() > > shouldn't be used. Bart, what can you suggest to do here? Opencoding > > it doesn't sound to me a (fully) correct approach in a long term. > > Note that I can't use gpiochip_request_own_desc(), nor any other > function that calls gpio_request_commit() (like gpiod_get()), because > that checks for gpiochip_line_is_valid(), and this returns false for > the TRNG line, cause that line is not a GPIO line, but interrupt only > line. > > That is why I used > irq = irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx); > until v7, with no reference to gpio descriptors, since this line is not > a GPIO line. > > We have discussed this back in April, in the thread > https://lore.kernel.org/soc/20240418121116.22184-8-kabel@kernel.org/ > where we concluded that > irq = gpiod_to_irq(gpiochip_get_desc(gc, irq_idx)); > is better... That's fine to not use other APIs, the problem here is with reference counting on the GPIO device. The API you could use is gpio_device_get_desc(). But you need to have a GPIO device pointer somewhere in your driver being available.
On Thu, 6 Jun 2024 13:11:09 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Jun 06, 2024 at 10:53:08AM +0200, Marek Behún wrote: > > On Wed, 5 Jun 2024 22:00:20 +0300 > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_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"); > > > > > > Okay, it's a bit more complicated than that. The gpiochip_get_desc() > > > shouldn't be used. Bart, what can you suggest to do here? Opencoding > > > it doesn't sound to me a (fully) correct approach in a long term. > > > > Note that I can't use gpiochip_request_own_desc(), nor any other > > function that calls gpio_request_commit() (like gpiod_get()), because > > that checks for gpiochip_line_is_valid(), and this returns false for > > the TRNG line, cause that line is not a GPIO line, but interrupt only > > line. > > > > That is why I used > > irq = irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx); > > until v7, with no reference to gpio descriptors, since this line is not > > a GPIO line. > > > > We have discussed this back in April, in the thread > > https://lore.kernel.org/soc/20240418121116.22184-8-kabel@kernel.org/ > > where we concluded that > > irq = gpiod_to_irq(gpiochip_get_desc(gc, irq_idx)); > > is better... > > That's fine to not use other APIs, the problem here is with reference counting > on the GPIO device. The API you could use is gpio_device_get_desc(). But you > need to have a GPIO device pointer somewhere in your driver being available. Rewriting to gpio_device_get_desc() is simple, since gpiochip_get_desc(gc, hwnum) is equivalent to gpio_device_get_desc(gc->gpiodev, hwnum) Obviously neither of these take care of reference counting. But what reference counting are you talking about? Is it the try_module_get(desc->gdev->owner) in gpiod_request()? Or are we talking only about the FLAG_REQUESTED flag on the descriptor flags? (This one should not be needed since the GPIO line cannot be requested, becuase it is not a valid GPIO line.) Since the line is not a valid GPIO line, I thought that we don't need to refcount in GPIO code. gpiod_to_irq() will call the gpiochip's .to_irq() method, which will call gpiochip_to_irq(), which will call irq_create_mapping() gpiod_to_irq() gpiochip_to_irq() irq_create_mapping() Then on gpiochip removal, the gpiochip_irqchip_remove() manually disposes all IRQ mappings with irq_dispose_mapping(). Marek
On Wed, Jun 05, 2024 at 06:18:49PM +0200, Marek Behún wrote: > > +static int omnia_trng_read(struct hwrng *rng, void *data, size_t max, bool wait) > +{ > + struct omnia_mcu *mcu = (struct omnia_mcu *)rng->priv; Please don't cast rng->priv in this manner. Please take a look at drivers/char/hw_random/bcm2835-rng.c for how it should be done. Thanks,
On Fri, 7 Jun 2024 18:30:24 +0800 Herbert Xu <herbert@gondor.apana.org.au> wrote: > On Wed, Jun 05, 2024 at 06:18:49PM +0200, Marek Behún wrote: > > > > +static int omnia_trng_read(struct hwrng *rng, void *data, size_t max, bool wait) > > +{ > > + struct omnia_mcu *mcu = (struct omnia_mcu *)rng->priv; > > Please don't cast rng->priv in this manner. Please take a look at > drivers/char/hw_random/bcm2835-rng.c for how it should be done. > > Thanks, THX, prepared for next version.
On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> 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. > > +Cc: Bart for gpiochip_get_desc() usage. > > ... > > > +#include <linux/bitfield.h> > > +#include <linux/completion.h> > > + errno.h > > > +#include <linux/gpio/consumer.h> > > +#include <linux/gpio/driver.h> > > +#include <linux/hw_random.h> > > +#include <linux/i2c.h> > > +#include <linux/interrupt.h> > > +#include <linux/minmax.h> > > +#include <linux/module.h> > > +#include <linux/string.h> > > > +#include <linux/turris-omnia-mcu-interface.h> > > As per other patches. > > > +#include <linux/types.h> > > + > > +#include "turris-omnia-mcu.h" > > ... > > > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_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"); > > Okay, it's a bit more complicated than that. The gpiochip_get_desc() > shouldn't be used. Bart, what can you suggest to do here? Opencoding > it doesn't sound to me a (fully) correct approach in a long term. > Andy's worried about reference counting of the GPIO device. Maybe you should just ref the GPIO device in irq_request_resources() and unref it in irq_release_resources()? Then you could use gpiochip_get_desc() just fine. Bart
On Mon, 17 Jun 2024 10:38:31 +0200 Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> 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. > > > > +Cc: Bart for gpiochip_get_desc() usage. > > > > ... > > > > > +#include <linux/bitfield.h> > > > +#include <linux/completion.h> > > > > + errno.h > > > > > +#include <linux/gpio/consumer.h> > > > +#include <linux/gpio/driver.h> > > > +#include <linux/hw_random.h> > > > +#include <linux/i2c.h> > > > +#include <linux/interrupt.h> > > > +#include <linux/minmax.h> > > > +#include <linux/module.h> > > > +#include <linux/string.h> > > > > > +#include <linux/turris-omnia-mcu-interface.h> > > > > As per other patches. > > > > > +#include <linux/types.h> > > > + > > > +#include "turris-omnia-mcu.h" > > > > ... > > > > > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_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"); > > > > Okay, it's a bit more complicated than that. The gpiochip_get_desc() > > shouldn't be used. Bart, what can you suggest to do here? Opencoding > > it doesn't sound to me a (fully) correct approach in a long term. > > > > Andy's worried about reference counting of the GPIO device. Maybe you > should just ref the GPIO device in irq_request_resources() and unref > it in irq_release_resources()? Then you could use gpiochip_get_desc() > just fine. But this is already being done. The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition: static const struct irq_chip omnia_mcu_irq_chip = { ... GPIOCHIP_IRQ_RESOURCE_HELPERS, }; This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are used as irq_request_resources() and irq_release_resources(). The gpiochip_reqres_irq() code increases the module refcount and even locks the line as IRQ: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732 Marek
On Mon, Jun 17, 2024 at 10:56 AM Marek Behún <kabel@kernel.org> wrote: > > On Mon, 17 Jun 2024 10:38:31 +0200 > Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > > > > On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> 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. > > > > > > +Cc: Bart for gpiochip_get_desc() usage. > > > > > > ... > > > > > > > +#include <linux/bitfield.h> > > > > +#include <linux/completion.h> > > > > > > + errno.h > > > > > > > +#include <linux/gpio/consumer.h> > > > > +#include <linux/gpio/driver.h> > > > > +#include <linux/hw_random.h> > > > > +#include <linux/i2c.h> > > > > +#include <linux/interrupt.h> > > > > +#include <linux/minmax.h> > > > > +#include <linux/module.h> > > > > +#include <linux/string.h> > > > > > > > +#include <linux/turris-omnia-mcu-interface.h> > > > > > > As per other patches. > > > > > > > +#include <linux/types.h> > > > > + > > > > +#include "turris-omnia-mcu.h" > > > > > > ... > > > > > > > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_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"); > > > > > > Okay, it's a bit more complicated than that. The gpiochip_get_desc() > > > shouldn't be used. Bart, what can you suggest to do here? Opencoding > > > it doesn't sound to me a (fully) correct approach in a long term. > > > > > > > Andy's worried about reference counting of the GPIO device. Maybe you > > should just ref the GPIO device in irq_request_resources() and unref > > it in irq_release_resources()? Then you could use gpiochip_get_desc() > > just fine. > > But this is already being done. > > The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition: > > static const struct irq_chip omnia_mcu_irq_chip = { > ... > GPIOCHIP_IRQ_RESOURCE_HELPERS, > }; > > This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are > used as irq_request_resources() and irq_release_resources(). > > The gpiochip_reqres_irq() code increases the module refcount and even > locks the line as IRQ: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732 > > Marek Andy: what is the issue here then exactly? Bart
On Mon, Jun 17, 2024 at 11:07 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Mon, Jun 17, 2024 at 10:56 AM Marek Behún <kabel@kernel.org> wrote: > > On Mon, 17 Jun 2024 10:38:31 +0200 > > Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> 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. > > > > > > > > +Cc: Bart for gpiochip_get_desc() usage. ... > > > > > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_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"); > > > > > > > > Okay, it's a bit more complicated than that. The gpiochip_get_desc() > > > > shouldn't be used. Bart, what can you suggest to do here? Opencoding > > > > it doesn't sound to me a (fully) correct approach in a long term. > > > > > > Andy's worried about reference counting of the GPIO device. Maybe you > > > should just ref the GPIO device in irq_request_resources() and unref > > > it in irq_release_resources()? Then you could use gpiochip_get_desc() > > > just fine. > > > > But this is already being done. > > > > The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition: > > > > static const struct irq_chip omnia_mcu_irq_chip = { > > ... > > GPIOCHIP_IRQ_RESOURCE_HELPERS, > > }; > > > > This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are > > used as irq_request_resources() and irq_release_resources(). > > > > The gpiochip_reqres_irq() code increases the module refcount and even > > locks the line as IRQ: > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732 > > Andy: what is the issue here then exactly? The function in use is marked as DEPRECATED. If you are fine with its usage in this case, I have no issues with it. If you want it to be replaced with the respective gpio_device_get_desc(), it's fine, but then the question is how properly get a pointer to GPIO device object.
On Mon, 17 Jun 2024 12:42:41 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Jun 17, 2024 at 11:07 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Mon, Jun 17, 2024 at 10:56 AM Marek Behún <kabel@kernel.org> wrote: > > > On Mon, 17 Jun 2024 10:38:31 +0200 > > > Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko > > > > <andy.shevchenko@gmail.com> wrote: > > > > > On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> 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. > > > > > > > > > > +Cc: Bart for gpiochip_get_desc() usage. > > ... > > > > > > > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_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"); > > > > > > > > > > Okay, it's a bit more complicated than that. The gpiochip_get_desc() > > > > > shouldn't be used. Bart, what can you suggest to do here? Opencoding > > > > > it doesn't sound to me a (fully) correct approach in a long term. > > > > > > > > Andy's worried about reference counting of the GPIO device. Maybe you > > > > should just ref the GPIO device in irq_request_resources() and unref > > > > it in irq_release_resources()? Then you could use gpiochip_get_desc() > > > > just fine. > > > > > > But this is already being done. > > > > > > The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition: > > > > > > static const struct irq_chip omnia_mcu_irq_chip = { > > > ... > > > GPIOCHIP_IRQ_RESOURCE_HELPERS, > > > }; > > > > > > This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are > > > used as irq_request_resources() and irq_release_resources(). > > > > > > The gpiochip_reqres_irq() code increases the module refcount and even > > > locks the line as IRQ: > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732 > > > > Andy: what is the issue here then exactly? > > The function in use is marked as DEPRECATED. If you are fine with its > usage in this case, I have no issues with it. > If you want it to be replaced with the respective > gpio_device_get_desc(), it's fine, but then the question is how > properly get a pointer to GPIO device object. > Aha, I did not notice that the function is deprecated. What about irq = gpiod_to_irq(gpio_device_get_desc(mcu->gc.gpiodev, irq_idx)); ? Note: I would prefer irq_create_mapping(mcu->gc.irq.domain, irq_idx) since the irq_idx line is not a valid GPIO line and at this part of the driver the fact that the IRQs are provided through a gpiochip are semantically irrelevant (we are interested in "an IRQ", not "an IRQ from a GPIO"). Marek
On Mon, Jun 17, 2024 at 1:34 PM Marek Behún <kabel@kernel.org> wrote: > > On Mon, 17 Jun 2024 12:42:41 +0200 > Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Mon, Jun 17, 2024 at 11:07 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > On Mon, Jun 17, 2024 at 10:56 AM Marek Behún <kabel@kernel.org> wrote: > > > > On Mon, 17 Jun 2024 10:38:31 +0200 > > > > Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > On Wed, Jun 5, 2024 at 9:00 PM Andy Shevchenko > > > > > <andy.shevchenko@gmail.com> wrote: > > > > > > On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> 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. > > > > > > > > > > > > +Cc: Bart for gpiochip_get_desc() usage. > > > > ... > > > > > > > > > + irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_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"); > > > > > > > > > > > > Okay, it's a bit more complicated than that. The gpiochip_get_desc() > > > > > > shouldn't be used. Bart, what can you suggest to do here? Opencoding > > > > > > it doesn't sound to me a (fully) correct approach in a long term. > > > > > > > > > > Andy's worried about reference counting of the GPIO device. Maybe you > > > > > should just ref the GPIO device in irq_request_resources() and unref > > > > > it in irq_release_resources()? Then you could use gpiochip_get_desc() > > > > > just fine. > > > > > > > > But this is already being done. > > > > > > > > The irqchip uses GPIOCHIP_IRQ_RESOURCE_HELPERS macro in its definition: > > > > > > > > static const struct irq_chip omnia_mcu_irq_chip = { > > > > ... > > > > GPIOCHIP_IRQ_RESOURCE_HELPERS, > > > > }; > > > > > > > > This means that gpiochip_irq_reqres() and gpiochip_irq_relres() are > > > > used as irq_request_resources() and irq_release_resources(). > > > > > > > > The gpiochip_reqres_irq() code increases the module refcount and even > > > > locks the line as IRQ: > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.10-rc4#n3732 > > > > > > Andy: what is the issue here then exactly? > > > > The function in use is marked as DEPRECATED. If you are fine with its > > usage in this case, I have no issues with it. > > If you want it to be replaced with the respective > > gpio_device_get_desc(), it's fine, but then the question is how > > properly get a pointer to GPIO device object. > > > > Aha, I did not notice that the function is deprecated. > > What about > > irq = gpiod_to_irq(gpio_device_get_desc(mcu->gc.gpiodev, irq_idx)); > > ? > > Note: I would prefer > irq_create_mapping(mcu->gc.irq.domain, irq_idx) > since the irq_idx line is not a valid GPIO line and at this part of > the driver the fact that the IRQs are provided through a gpiochip are > semantically irrelevant (we are interested in "an IRQ", not "an IRQ from > a GPIO"). > > Marek The reason to deprecate it was the fact that it's dangerous to use from outside of the GPIO provider code. I actually plan to soon make this function private to gpiolib, there's just one pinctrl driver left to convert. So maybe it's better to not use it here. Please keep in mind that gpio_device_get_desc() doesn't increase the reference count of gpio_device so you need to make sure it stays alive. But it seems to be the case here as you're within the driver code still. Bart
diff --git a/drivers/platform/cznic/Kconfig b/drivers/platform/cznic/Kconfig index e262930b3faf..6edac80d5fa3 100644 --- a/drivers/platform/cznic/Kconfig +++ b/drivers/platform/cznic/Kconfig @@ -18,6 +18,7 @@ config TURRIS_OMNIA_MCU depends on I2C select GPIOLIB select GPIOLIB_IRQCHIP + select HW_RANDOM select RTC_CLASS select WATCHDOG_CORE help @@ -27,6 +28,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 f44996588d38..1d4153a96526 100644 --- a/drivers/platform/cznic/turris-omnia-mcu-base.c +++ b/drivers/platform/cznic/turris-omnia-mcu-base.c @@ -380,7 +380,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 bc7965e6c879..972364d3d223 100644 --- a/drivers/platform/cznic/turris-omnia-mcu-gpio.c +++ b/drivers/platform/cznic/turris-omnia-mcu-gpio.c @@ -174,7 +174,7 @@ static const struct omnia_gpio omnia_gpios[64] = { }; /* 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(OMNIA_INT_CARD_DET)] = 4, [__bf_shf(OMNIA_INT_MSATA_IND)] = 5, [__bf_shf(OMNIA_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..fbde00f3fca1 --- /dev/null +++ b/drivers/platform/cznic/turris-omnia-mcu-trng.c @@ -0,0 +1,103 @@ +// 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/hw_random.h> +#include <linux/i2c.h> +#include <linux/interrupt.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 OMNIA_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 + OMNIA_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 -ERESTARTSYS; + + err = omnia_cmd_read(mcu->client, + OMNIA_CMD_TRNG_COLLECT_ENTROPY, + reply, sizeof(reply)); + if (err) + return err; + + bytes = min3(reply[0], max, OMNIA_CMD_TRNG_MAX_ENTROPY_LEN); + } while (wait && !bytes); + + memcpy(data, &reply[1], bytes); + + return bytes; +} + +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 & OMNIA_FEAT_TRNG)) + return 0; + + irq_idx = omnia_int_to_gpio_idx[__bf_shf(OMNIA_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"); + + /* + * 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, OMNIA_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 5f846909934f..ee8d58516156 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> @@ -47,6 +49,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, @@ -176,11 +182,13 @@ static inline int omnia_cmd_read_u8(const struct i2c_client *client, u8 cmd, return omnia_cmd_read(client, cmd, reply, sizeof(*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 | 103 ++++++++++++++++++ drivers/platform/cznic/turris-omnia-mcu.h | 8 ++ 6 files changed, 120 insertions(+), 2 deletions(-) create mode 100644 drivers/platform/cznic/turris-omnia-mcu-trng.c