On Wed, Jul 31, 2024 at 12:52:32PM +0100, Jonathan Cameron wrote:

Thanks for the explanation.  I clearly needed more coffee that day :)
Personally I find this to be a confusing use of scoped cleanup
as we aren't associating a constructor / destructor with scope, but
rather sort of 'adopting ownership / destructor'.

It is a pretty typical "move" concept for reference counting, as you
might see in other languages.

Assuming my caffeine level is better today, maybe device managed is
more appropriate?

Oh, perhaps controversial, but I dislike devm, so I'd rather not :) It
makes exciting bugs, IMHO. Someone (Laurent?) gave a nice presentation
on some of its nasty edge cases at LPC a few years ago.

devm_add_action_or_reset to associate the destructor by placing
it immediately after the setup path for both the allocate and unregister.
Should run in very nearly same order for teardown as what you have here.

Yes, in this case it would work technically.

I think devm would be more code lines, more memory usage, and more
failure cases though.

So, I'm interested that cleanup could be a better option. I view it as
positive that the success path remove() flow is actually documented
what order it is doing things. Order is very important there and I've
seen enough places get things wrong here over the years..

One of the nasty traps of devm is you have to know to have your
creation order match your required destruction order, and *everything*
has to use devm or it can't be ordered.

Mind you cleanup has the same order trap, but you don't have to use
cleanup during remove(), and maybe it was overzealous to do so here.

Thanks,
Jason