On Thu, Jul 23, 2020 at 4:46 PM Philipp Zabel p.zabel@pengutronix.de wrote:
Hi Daniel,
On Thu, 2020-07-23 at 00:22 +0200, daniel@ffwll.ch wrote: [...]
Yeah the drmm_ versions of these need to check that the ->cleanup hook is NULL.
Also there's not actually a double-free, since drm_foo_cleanup removes it from the lists, which means drm_mode_config_cleanup won't even see it. But if the driver has some additional code in ->cleanup that won't ever run, so probably still a bug.
I also think that the drmm_foo_ wrappers should also do the allocation (and upcasting) kinda like drmm_dev_alloc(). Otherwise we're still stuck with tons of boilerplate.
Ok, I'll try this:
drmm_encoder_init() variant can verify that the passed drm_encoder_funcs::destroy hook is NULL.
drmm_simple_encoder_init() can just provide empty drm_encoder_funcs internally.
For now I think it's ok if drivers that switch to drmm_ just copypaste, until we're sure this is the right thing to do. And then maybe also roll these out for all objects that stay for the entire lifetime of drm_device (plane, crtc, encoder, plus variants). Just to make sure we're consistent across all of them.
Thank you for clarifying, I wasn't sure this was the goal. I've started with this function mostly because this is the most used one in imx-drm and the only one where I didn't have to deal with va_args boilerplate.
Hm if we go with also exposing the drmm_foo_init() variants then I think these should check that the passed-in memory is indeed allocated by drmres on the right device. That's kinda the bug I'm afraid of when we exposed drmm_foo_init() to drivers and not just drmm_foo_alloc() which does everything and correctly for you. For the drmm_is_manged or so I think we can just reuse the same loop I've typed up already for drmm_kfree. There shouldn't be too many allocations on that list that the list walk at driver load will matter. Oh also better name than drmm_is_managed would be good, devres doesn't seem to have that. Hm maybe drmm_assert_managed(dev, void, size) and it checks that it's fully contained within a drmm_kmalloc region.
Cheers, Daniel