On Thu, Jul 23, 2020 at 4:46 PM Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> wrote: > > Hi Daniel, > > On Thu, 2020-07-23 at 00:22 +0200, daniel@xxxxxxxx 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
