On Thu, Apr 29, 2021 at 02:33:17PM +0200, Hans de Goede wrote: > Hi, > > On 4/29/21 2:04 PM, Daniel Vetter wrote: > > On Thu, Apr 29, 2021 at 01:54:46PM +0200, Greg Kroah-Hartman wrote: > >> On Thu, Apr 29, 2021 at 01:40:28PM +0200, Daniel Vetter wrote: > >>> On Wed, Apr 28, 2021 at 11:52:49PM +0200, Hans de Goede wrote: > >>>> Userspace could hold open a reference to the connector->kdev device, > >>>> through e.g. holding a sysfs-atrtribute open after > >>>> drm_sysfs_connector_remove() has been called. In this case the connector > >>>> could be free-ed while the connector->kdev device's drvdata is still > >>>> pointing to it. > >>>> > >>>> Give drm_connector devices there own device type, which allows > >>>> us to specify our own release function and make drm_sysfs_connector_add() > >>>> take a reference on the connector object, and have the new release > >>>> function put the reference when the device is released. > >>>> > >>>> Giving drm_connector devices there own device type, will also allow > >>>> checking if a device is a drm_connector device with a > >>>> "if (device->type == &drm_sysfs_device_connector)" check. > >>>> > >>>> Note that the setting of the name member of the device_type struct will > >>>> cause udev events for drm_connector-s to now contain DEVTYPE=drm_connector > >>>> as extra info. So this extends the uevent part of the userspace API. > >>>> > >>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > >>> > >>> Are you sure? I thought sysfs is supposed to flush out any pending > >>> operations (they complete fast) and handle open fd internally? > >> > >> Yes, it "should" :) > > > > Thanks for confirming my vague memories :-) > > > > Hans, pls drop this one. > > Please see my earlier reply to your review of this patch, it is > still needed but for a different reason: > > """ > We still need this change though to make sure that the > "drm/connector: Add drm_connector_find_by_fwnode() function" > does not end up following a dangling drvdat pointer from one > if the drm_connector kdev-s. > > The class_dev_iter_init() in drm_connector_find_by_fwnode() gets > a reference on all devices and between getting that reference > and it calling drm_connector_get() - drm_connector_unregister() > may run and drop the possibly last reference to the > drm_connector object, freeing it and leaving the kdev's > drvdata as a dangling pointer. > """ > > This is actually why I added it initially, and while adding it > I came up with this wrong theory of why it was necessary independently > of the drm_connector_find_by_fwnode() addition, sorry about that. Generally that's handled by a kref_get_unless_zero under the protection of the lock which protects the weak reference. Which I think is the right model here (at a glance at least) since this is a lookup function. Lookup tables holding full references tends to lead to all kinds of bad side effects. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
