Re: [PATCH] gpio: gpiolib: fix refcount imbalance in gpiochip_setup_dev()

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>]

On Fri, Dec 06, 2024 at 05:53:25PM +0900, Joe Hattori wrote:
> Thank you for the review. And yes, I should have paid more attention to the
> code, sorry. I have reflected the reviewed points in the V2 patch, so please
> take a look at it.

I can't look at something I haven't received.

> On 12/5/24 22:20, Andy Shevchenko wrote:
> > On Wed, Dec 04, 2024 at 09:21:52PM +0900, Joe Hattori wrote:
> >> In gpiochip_setup_dev(), the refcount incremented in device_initialize()
> >> is not decremented in the error path. Fix it by calling put_device().
> >
> > First of all, we have gpio_put_device().
> 
> Fixed in the V2 patch.
> 
> > Second, what the problem do you have in practice? Can you show any
> backtrace?
> > Third, how had this change been tested?
> 
> I am building a static analysis tool, and it reported this reference leak. I
> overlooked that it still reported the same error after this patch, and it is
> fixed in the V2 patch. If a backtrace is needed, I will try.

You missed the guidelines / rules about static analysis tools:
Documentation/process/researcher-guidelines.rst.

> > Looking at the current code I noticed the following:
> > 1) gpiochip_add_data_with_key() has already that call;
> 
> Thank you for pointing out, the call has been removed in the V2 patch.
> > 2) gpiochip_setup_devs() misses that call.
> 
> I was not sure if the gpiochip_setup_devs() should have an error path to
> remove all the registered GPIO devices when a single registration fails,

I don't think so. But that failed device should be put. It's the only issue
I see with the current code. If I'm wrong, prove it and elaborate in the commit
message (backtraces will be very helpful).

> thus it is not addressed in the V2 patch. If such a cleanup path is needed,
> please let me know.
> 
> > This effectively means that you inroduce a regression while fixing a bug.
> >
> > The GPIO device initialisation is non-trivial, please pay more attention
> to the
> > code.
> >
> > Bart, can this be removed or reverted before it poisons stable?

-- 
With Best Regards,
Andy Shevchenko