Re: [PATCH] gpio: gpiolib-of: Fix refcount bugs in of_mm_gpiochip_add_data()

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

Hi Liang,

thanks for your patch!

On Mon, Jul 4, 2022 at 11:13 AM Liang He <windhl@xxxxxxx> wrote:

> We should use of_node_get() when a new reference of device_node
> is created. It is noted that the old reference stored in
> 'mm_gc->gc.of_node' should also be decreased.
>
> Fixes: f141ed65f256 ("gpio: Move DT support code into drivers/gpio")

I doubt this is fixing that commit since it is only moving code?

> -       mm_gc->gc.of_node = np;
> +       of_node_put(mm_gc->gc.of_node);
> +       mm_gc->gc.of_node = of_node_get(np);

Aha


> This patch is based on the fact that there is a call site in function 'qe_add_gpiochips()'
> of src file 'drivers\soc\fsl\qe\gpio.c'.
>
> In that function, of_mm_gpiochip_add_data() is contained in a iteration of for_each_compatible_node(),
> which will automatically increase and decrease the refcount.

Put this into the commit log message!

I guess it solves the immediate problem, but the real solution is to
get rid of of_mm_gpiochip_add_data() and struct of_mm_gpio_chip
and replace all calls with instances of proper gpiochips using
[devm_]gpiochip_add_data().

Which is in our TODO file.

It's not very much using this old helper:

$ git grep of_mm_gpiochip_add_data
arch/powerpc/platforms/4xx/gpio.c:              ret =
of_mm_gpiochip_add_data(np, mm_gc, ppc4xx_gc);
arch/powerpc/platforms/8xx/cpm1.c:      return
of_mm_gpiochip_add_data(np, mm_gc, cpm1_gc);
arch/powerpc/platforms/8xx/cpm1.c:      return
of_mm_gpiochip_add_data(np, mm_gc, cpm1_gc);
arch/powerpc/sysdev/cpm_common.c:       return
of_mm_gpiochip_add_data(np, mm_gc, cpm2_gc);
drivers/gpio/gpio-altera.c:     ret = of_mm_gpiochip_add_data(node,
&altera_gc->mmchip, altera_gc);
drivers/gpio/gpio-mm-lantiq.c:  return
of_mm_gpiochip_add_data(pdev->dev.of_node, &chip->mmchip, chip);
drivers/gpio/gpio-mpc5200.c:    ret =
of_mm_gpiochip_add_data(ofdev->dev.of_node, &chip->mmchip, chip);
drivers/gpio/gpio-mpc5200.c:    ret =
of_mm_gpiochip_add_data(ofdev->dev.of_node, &chip->mmchip, chip);
drivers/soc/fsl/qe/gpio.c:              ret =
of_mm_gpiochip_add_data(np, mm_gc, qe_gc);

Those are all.

They all seem like they can be simplified a lot by using select GPIO_GENERIC
and bgpio_init() in the manner of eg drivers/gpio/gpio-ftgpio010.c replacing
a lot of boilerplate code.

If you have access to the FSL board and can test this, please consider doing
this bigger change instead, at least for that board. It is certainly making
the world a much better place than reparing the mistakes in code
using of_mm_gpio_chip.

Yours,
Linus Walleij