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
