Re: [PATCH v3] iio: trigger: Fix error handling in viio_trigger_alloc

["Nuno Sá" <noname.nuno@gmail.com>]

On Fri, 2025-11-07 at 20:19 +0200, Andy Shevchenko wrote:
> On Fri, Nov 07, 2025 at 04:48:03PM +0000, Nuno Sá wrote:
> > On Fri, 2025-11-07 at 12:42 +0200, Andy Shevchenko wrote:
> > > On Fri, Nov 07, 2025 at 10:26:10AM +0000, Nuno Sá wrote:
> > > > On Fri, 2025-11-07 at 10:02 +0800, Ma Ke wrote:
> > > > > viio_trigger_alloc() initializes the device with device_initialize()
> > > > > but uses kfree() directly in error paths, which bypasses the device's
> > > > > release callback iio_trig_release(). This could lead to memory leaks
> > > > > and inconsistent device state.
> 
> ...
> 
> > > > > -free_descs:
> > > > > -	irq_free_descs(trig->subirq_base,
> > > > > CONFIG_IIO_CONSUMERS_PER_TRIGGER);
> > > > >  free_trig:
> > > > > -	kfree(trig);
> > > > > +	put_device(&trig->dev);
> > > > 
> > > > Yes, device_initialize() docs do say that we should give the reference
> > > > instead of
> > > > freeing the device but I'm not see how that helps in here. Maybe initializing
> > > > the
> > > > device should be done only after all the resources are allocated so the code
> > > > is a
> > > > bit
> > > > more clear... But doing it like you're doing just means that we might get
> > > > into
> > > > the
> > > > release function with things that might or might not be allocated which is a
> > > > pattern
> > > > I would prefer to avoid.
> > > 
> > > The put_device() here is the correct (and must) thing to do independently on
> > > the preferences. The problem is that device_initialise() and followed calls
> > > may do much more than just some initialisation.
> > 
> > Well, I would argue against that (at least in the context the function is now
> > implemented). To me, the right thing to do would be to move the device
> > initialization
> > code to this point:
> > 
> > https://elixir.bootlin.com/linux/v6.17.7/source/drivers/iio/industrialio-trigger.c#L594
> > 
> > trig->dev.parent = parent;
> > trig->dev.type = &iio_trig_type;
> > trig->dev.bus = &iio_bus_type;
> > device_initialize(&trig->dev);
> > 
> > Then we would not even need to think about put_device(). Like it is, using it,
> > it's
> > just prone to errors (I did mentioned a couple of things this patch introduced If
> > I'm
> > not overseeing it) or we do need to have lots of care in the release function to
> > make
> > sure we don't mess up. To me that's a bad sign on how the code is architectured. 
> > 
> > FWIW, the pattern you find for example in SPI is the natural one for me:
> > 
> > You have a spi_alloc_device() [1] that initialises struct device right in the
> > end.
> > Above it, kfree() as usual. Then the callers, will indeed use put_device() in
> > their
> > error paths.
> > 
> > So the pattern to me is to do device_initialize() after all resources of your
> > device
> > are allocated. So that after that point put_device() does not get you into some
> > odd
> > handling in the release callback.
> 
> Sure, this can be another approach. Whatever you, folks, prefer. But at least
> the mutex_destroy() (separate) patch can be issued and accepted independently.
> 

Sure, agreed on that.

> The bottom line is:
> 1) the current code has an issue;
> 2) the proposed fix has its own flaws;
> 3) but the idea in the current approach at least small (if implemented
> correctly) and makes sure that any new allocations won't be forgotten in
> the error patch, nor in the ->release() callback.
> 
> > [1]: https://elixir.bootlin.com/linux/v6.17.7/source/drivers/spi/spi.c#L568

FWIW and unless I'm missing something there's nothing fundamentally wrong in the
current code (i.e any real bug). That said, I would ack a change that moved the
device initialization code to it's natural place (at least in the way I see it).

- Nuno Sá