Re: [PATCH v11 6/8] platform: cznic: turris-omnia-mcu: Add support for MCU provided TRNG

["Marek Behún" <kabel@kernel.org>]

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