Re: [PATCH v1 1/1] spi: Simplify devm_spi_*_controller()

[Felix Gu]

On Wed, Mar 25, 2026 at 5:54 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> > It seems has a potential corner case in this commit.
> >
> > The issue is that devm_add_action_or_reset() triggers its callback
> > immediately if the internal devres allocation fails.
>
> Yes, it does.
>
> > This creates the following sequence:
> > 1. spi_register_controller(ctlr) succeeds.
> > 2. devm_add_action_or_reset() is called but fails.
> > 3. The "reset" triggers devm_spi_unregister_controller(ctlr) immediately.
> > 4. This calls spi_unregister_controller(ctlr).
> > 5. For controllers allocated via spi_alloc_host() (where
> >    ctlr->devm_allocated is false), spi_unregister_controller() calls
> >    put_device(&ctlr->dev).
> > 6. This drops the reference count to zero and frees the ctlr structure.
>
> I am not sure I see how the sequence is different from the say this
>
>         ret = spi_register_controller(...)
>         if (ret)
>                 goto err;
>         // ...if success do something else...
>         ret = foo();
>         if (ret) {
>                 spi_unregister_controller(...);
>                 goto err;
>         }
>
> So, each driver should be prepared for an error handling in which it should
> release resources in the reversed order.
>
> > However, the driver still holds the ctlr pointer and will typically
> > attempt its own cleanup, leading to a use-after-free or double-free.
>
> You mean buggy driver? Any real example of the use case?
>
> In the 5. you implied something like
>
>         ret = spi_alloc_host();
>         ...
>         ret = devm_spi_register_controller()
>
> ?
>
> But this is against the rule how devm must be used.
>
> > What are your thoughts on this? Does this analysis seem correct, or am
> > I missing a detail in how the refcounting is handled here?
>
> Is this analysis AI assisted?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Hi Andy,
I got the thoughts from an AI review from sashiko and  verified it on my own.
https://sashiko.dev/#/patchset/20260322-rockchip-v1-1-fac3f0c6dad8%40gmail.com

Best Regards,
Felix