On Tuesday 22 Nov 2016 11:58:43 Hans Verkuil wrote:
> On 22/11/16 10:58, Laurent Pinchart wrote:
> > Hi Hans,
> >
> > On Tuesday 22 Nov 2016 10:46:31 Hans Verkuil wrote:
> >> On 08/11/16 14:55, Sakari Ailus wrote:
> >>> The struct device of the media device driver (i.e. not that of the media
> >>> devnode) is pointed to by the media device. The struct device pointer is
> >>> mostly used for debug prints.
> >>>
> >>> Ensure it will stay around as long as the media device does.
> >>>
> >>> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> >>> ---
> >>>
> >>> drivers/media/media-device.c | 9 ++++++++-
> >>> 1 file changed, 8 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >>> index 2e52e44..648c64c 100644
> >>> --- a/drivers/media/media-device.c
> >>> +++ b/drivers/media/media-device.c
> >>> @@ -724,6 +724,7 @@ static void media_device_release(struct
> >>> media_devnode
> >>> *devnode)
> >>>
> >>> mdev->entity_internal_idx_max = 0;
> >>> media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> >>> mutex_destroy(&mdev->graph_mutex);
> >>>
> >>> + put_device(mdev->dev);
> >>>
> >>> kfree(mdev);
> >>>
> >>> }
> >>>
> >>> @@ -732,9 +733,15 @@ struct media_device *media_device_alloc(struct
> >>> device
> >>> *dev)
> >>>
> >>> {
> >>>
> >>> struct media_device *mdev;
> >>>
> >>> + dev = get_device(dev);
> >>> + if (!dev)
> >>> + return NULL;
> >>
> >> I don't think this is right. When you allocate the media_device struct
> >> it should just be initialized, but not have any side effects until it is
> >> actually registered.
> >>
> >> When the device is registered the device_add call will increase the
> >> parent's refcount as it should, thus ensuring it stays around for as
> >> long as is needed.
> >
> > We're storing a pointer to dev in mdev a few lines below. As dev is
> > refcounted, we need to ensure that we take a reference appropriately. We
> > can either borrow a reference taken elsewhere or take our own reference.
> >
> > Borrowing a reference is only valid if we know it will exist for at least
> > as long as we need to borrow it. That might be the case when creating the
> > media device as the driver performing the operation should hold a
> > reference to the struct device instance (especially given that allocation
> > and registration are usually - but not always - performed at probe time
> > for that driver), but it's harder to guarantee at unregistration time,
> > especially when userspace can keep device nodes open across
> > unregistration. This patch ensures that the pointer always remains valid
> > until we stop needing it.
>
> I disagree. There is no reason to keep the parent device in memory once the
> media devnode is unregistered.
There's a very big one: the media device is accessed through a large number of
APIs, not only through its own devnode. It can for instance be accessed
through V4L2 devnodes, and thus has to live as long as *anything* can access
it.
struct media_devnode was a very very bad idea. The original goal was to share
the implementation with the V4L2 devnodes, but when that got rejected I really
should have merged struct media_device and struct media_devnode into a single
structure. We can keep media_devnode separate if that is believed to improve
readability of the code, but there is absolutely no reason for allocating
media_devnode separately from media_device.
> It seems to be pretty much only used for some debugging. I suspect that in
> almost all cases the debugging happens when the devnode is registered, and
> not when it is unregistered. But in that case you can also use &devnode.dev
> as the device pointer for dev_dbg, or use pr_debug.
>
> Looking at what the CEC framework does I see that I pass a device pointer
> to the allocate function, but I really don't need to do that. It is not
> used anywhere until the register function, so the parent device pointer
> should be passed as an argument to the register function, not to the
> allocate function.
>
> BTW, I would very much prefer it if mdev->dev is renamed to mdev->parent.
> Or better yet, just dropped completely since it is also available as
> mdev->devnode.parent. And even devnode.parent can be dropped and just
> use mdev->devnode.dev.parent.
>
> I plan on posting such a patch for the cec framework as well, since
> it avoids having duplicates of the same device parent pointer in the
> data structures.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
