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

["Liang He" <windhl@xxxxxxx>]

At 2022-07-06 20:47:08, "Linus Walleij" <linus.walleij@xxxxxxxxxx> wrote:
>Hi Liang,
>
>thanks for your patch!

>


Thanks! 

>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 will put it in new version patch.

>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


Hi, Linus,

First of all, thanks for your effort to review and confirm my current patch.

Second, while I would like very much to make a bigger change, it will need 
days for me to learn the whole semantic of the source code as now I only have learned
the semantic of OF APIs and can only make a small step to decide if there is a refcount bug.

But now, should I re-post this patch with the above commit log you suggested or do more
things after I can?

Thanks again, 

Liang