[v2,16/29] media: mc: Refcount the media device
Commit Message
As the struct media_device embeds struct media_devnode, the lifetime of
that object must be that same than that of the media_device.
References are obtained by media_device_get() and released by
media_device_put(). In order to use refcounting, the driver must set the
release callback before calling media_device_init() on the media device.
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
drivers/media/mc/mc-device.c | 36 +++++++++++++++++++++++++++++------
drivers/media/mc/mc-devnode.c | 6 +++++-
include/media/media-device.h | 28 +++++++++++++++++++++++++++
3 files changed, 63 insertions(+), 7 deletions(-)
Comments
Hi Sakari,
Thank you for the patch.
On Wed, Dec 20, 2023 at 12:37:00PM +0200, Sakari Ailus wrote:
> As the struct media_device embeds struct media_devnode, the lifetime of
> that object must be that same than that of the media_device.
>
> References are obtained by media_device_get() and released by
> media_device_put(). In order to use refcounting, the driver must set the
> release callback before calling media_device_init() on the media device.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> drivers/media/mc/mc-device.c | 36 +++++++++++++++++++++++++++++------
> drivers/media/mc/mc-devnode.c | 6 +++++-
> include/media/media-device.h | 28 +++++++++++++++++++++++++++
> 3 files changed, 63 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> index e6ac9b066524..bbc233e726d2 100644
> --- a/drivers/media/mc/mc-device.c
> +++ b/drivers/media/mc/mc-device.c
> @@ -700,6 +700,31 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
> }
> EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
>
> +static void __media_device_release(struct media_device *mdev)
> +{
> + dev_dbg(mdev->dev, "Media device released\n");
> +
> + ida_destroy(&mdev->entity_internal_idx);
> + mdev->entity_internal_idx_max = 0;
> + media_graph_walk_cleanup(&mdev->pm_count_walk);
> + mutex_destroy(&mdev->graph_mutex);
> + mutex_destroy(&mdev->req_queue_mutex);
> +}
> +
> +static void media_device_release(struct media_devnode *devnode)
> +{
> + struct media_device *mdev = to_media_device(devnode);
> +
> + if (mdev->ops && mdev->ops->release) {
> + /*
> + * If release op isn't set, __media_device_release() is called
> + * via media_device_cleanup().
> + */
> + __media_device_release(mdev);
> + mdev->ops->release(mdev);
> + }
> +}
> +
> void media_device_init(struct media_device *mdev)
> {
> INIT_LIST_HEAD(&mdev->entities);
> @@ -712,6 +737,8 @@ void media_device_init(struct media_device *mdev)
> mutex_init(&mdev->graph_mutex);
> ida_init(&mdev->entity_internal_idx);
> atomic_set(&mdev->request_id, 0);
> +
> + mdev->devnode.release = media_device_release;
> media_devnode_init(&mdev->devnode);
>
> if (!*mdev->bus_info)
> @@ -724,12 +751,9 @@ EXPORT_SYMBOL_GPL(media_device_init);
>
> void media_device_cleanup(struct media_device *mdev)
> {
> - ida_destroy(&mdev->entity_internal_idx);
> - mdev->entity_internal_idx_max = 0;
> - media_graph_walk_cleanup(&mdev->pm_count_walk);
> - mutex_destroy(&mdev->graph_mutex);
> - mutex_destroy(&mdev->req_queue_mutex);
> - put_device(&mdev->devnode.dev);
> + WARN_ON(mdev->ops && mdev->ops->release);
> + __media_device_release(mdev);
> + media_device_put(mdev);
> }
> EXPORT_SYMBOL_GPL(media_device_cleanup);
>
> diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> index 5057c48f8870..4ea05e42dafb 100644
> --- a/drivers/media/mc/mc-devnode.c
> +++ b/drivers/media/mc/mc-devnode.c
> @@ -59,6 +59,10 @@ static void media_devnode_release(struct device *cd)
> {
> struct media_devnode *devnode = to_media_devnode(cd);
>
> + /* If the devnode has a ref, it is simply released by the user. */
> + if (devnode->ref)
The structure has no ref member.
> + return;
> +
> if (devnode->minor != -1)
> media_devnode_free_minor(devnode->minor);
>
> @@ -213,6 +217,7 @@ static const struct file_operations media_devnode_fops = {
> void media_devnode_init(struct media_devnode *devnode)
> {
> device_initialize(&devnode->dev);
> + devnode->dev.release = media_devnode_release;
> devnode->minor = -1;
> }
>
> @@ -246,7 +251,6 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
>
> devnode->dev.bus = &media_bus_type;
> devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
> - devnode->dev.release = media_devnode_release;
> if (devnode->parent)
> devnode->dev.parent = devnode->parent;
> dev_set_name(&devnode->dev, "media%d", devnode->minor);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index fb0855b217ce..c6816be0eee8 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -62,6 +62,7 @@ struct media_entity_notify {
> * request (and thus the buffer) must be available to the driver.
> * And once a buffer is queued, then the driver can complete
> * or delete objects from the request before req_queue exits.
> + * @release: Release the resources of the media device.
> */
> struct media_device_ops {
> int (*link_notify)(struct media_link *link, u32 flags,
> @@ -70,6 +71,7 @@ struct media_device_ops {
> void (*req_free)(struct media_request *req);
> int (*req_validate)(struct media_request *req);
> void (*req_queue)(struct media_request *req);
> + void (*release)(struct media_device *mdev);
> };
>
> /**
> @@ -219,6 +221,30 @@ struct usb_device;
> */
> void media_device_init(struct media_device *mdev);
>
> +/**
> + * media_device_get() - Get a reference to a media device
Maybe mimick the get_device() wording and state "atomically increment
the reference count for the media device" ? Same for put.
> + *
> + * @mdev: media device
This should return a pointer to the media_device, as other get functions
do.
> + */
> +#define media_device_get(mdev) \
> + do { \
> + dev_dbg((mdev)->dev, "%s: get media device %s\n", \
> + __func__, (mdev)->bus_info); \
Do we really need this ? I'd prefer inline functions to ensure type
safety. If we need to track the get/put callers, I think using ftrace
would be a better option.
> + get_device(&(mdev)->devnode.dev); \
> + } while (0)
> +
> +/**
> + * media_device_put() - Put a reference to a media device
> + *
> + * @mdev: media device
> + */
> +#define media_device_put(mdev) \
> + do { \
> + dev_dbg((mdev)->dev, "%s: put media device %s\n", \
> + __func__, (mdev)->bus_info); \
> + put_device(&(mdev)->devnode.dev); \
> + } while (0)
> +
> /**
> * media_device_cleanup() - Cleanups a media device element
> *
> @@ -432,6 +458,8 @@ void __media_device_usb_init(struct media_device *mdev,
> const char *driver_name);
>
> #else
> +#define media_device_get(mdev) do { } while (0)
> +#define media_device_put(mdev) do { } while (0)
> static inline int media_device_register(struct media_device *mdev)
> {
> return 0;
Hi Laurent,
On Wed, Feb 07, 2024 at 01:08:48PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Wed, Dec 20, 2023 at 12:37:00PM +0200, Sakari Ailus wrote:
> > As the struct media_device embeds struct media_devnode, the lifetime of
> > that object must be that same than that of the media_device.
> >
> > References are obtained by media_device_get() and released by
> > media_device_put(). In order to use refcounting, the driver must set the
> > release callback before calling media_device_init() on the media device.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > ---
> > drivers/media/mc/mc-device.c | 36 +++++++++++++++++++++++++++++------
> > drivers/media/mc/mc-devnode.c | 6 +++++-
> > include/media/media-device.h | 28 +++++++++++++++++++++++++++
> > 3 files changed, 63 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c
> > index e6ac9b066524..bbc233e726d2 100644
> > --- a/drivers/media/mc/mc-device.c
> > +++ b/drivers/media/mc/mc-device.c
> > @@ -700,6 +700,31 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
> > }
> > EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
> >
> > +static void __media_device_release(struct media_device *mdev)
> > +{
> > + dev_dbg(mdev->dev, "Media device released\n");
> > +
> > + ida_destroy(&mdev->entity_internal_idx);
> > + mdev->entity_internal_idx_max = 0;
> > + media_graph_walk_cleanup(&mdev->pm_count_walk);
> > + mutex_destroy(&mdev->graph_mutex);
> > + mutex_destroy(&mdev->req_queue_mutex);
> > +}
> > +
> > +static void media_device_release(struct media_devnode *devnode)
> > +{
> > + struct media_device *mdev = to_media_device(devnode);
> > +
> > + if (mdev->ops && mdev->ops->release) {
> > + /*
> > + * If release op isn't set, __media_device_release() is called
> > + * via media_device_cleanup().
> > + */
> > + __media_device_release(mdev);
> > + mdev->ops->release(mdev);
> > + }
> > +}
> > +
> > void media_device_init(struct media_device *mdev)
> > {
> > INIT_LIST_HEAD(&mdev->entities);
> > @@ -712,6 +737,8 @@ void media_device_init(struct media_device *mdev)
> > mutex_init(&mdev->graph_mutex);
> > ida_init(&mdev->entity_internal_idx);
> > atomic_set(&mdev->request_id, 0);
> > +
> > + mdev->devnode.release = media_device_release;
> > media_devnode_init(&mdev->devnode);
> >
> > if (!*mdev->bus_info)
> > @@ -724,12 +751,9 @@ EXPORT_SYMBOL_GPL(media_device_init);
> >
> > void media_device_cleanup(struct media_device *mdev)
> > {
> > - ida_destroy(&mdev->entity_internal_idx);
> > - mdev->entity_internal_idx_max = 0;
> > - media_graph_walk_cleanup(&mdev->pm_count_walk);
> > - mutex_destroy(&mdev->graph_mutex);
> > - mutex_destroy(&mdev->req_queue_mutex);
> > - put_device(&mdev->devnode.dev);
> > + WARN_ON(mdev->ops && mdev->ops->release);
> > + __media_device_release(mdev);
> > + media_device_put(mdev);
> > }
> > EXPORT_SYMBOL_GPL(media_device_cleanup);
> >
> > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c
> > index 5057c48f8870..4ea05e42dafb 100644
> > --- a/drivers/media/mc/mc-devnode.c
> > +++ b/drivers/media/mc/mc-devnode.c
> > @@ -59,6 +59,10 @@ static void media_devnode_release(struct device *cd)
> > {
> > struct media_devnode *devnode = to_media_devnode(cd);
> >
> > + /* If the devnode has a ref, it is simply released by the user. */
> > + if (devnode->ref)
>
> The structure has no ref member.
This could explain some of the problems GCC has had compiling this patch.
;-)
This belongs to patch "media: mc: Implement best effort media device
removal safety sans refcount".
>
> > + return;
> > +
> > if (devnode->minor != -1)
> > media_devnode_free_minor(devnode->minor);
> >
> > @@ -213,6 +217,7 @@ static const struct file_operations media_devnode_fops = {
> > void media_devnode_init(struct media_devnode *devnode)
> > {
> > device_initialize(&devnode->dev);
> > + devnode->dev.release = media_devnode_release;
> > devnode->minor = -1;
> > }
> >
> > @@ -246,7 +251,6 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
> >
> > devnode->dev.bus = &media_bus_type;
> > devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
> > - devnode->dev.release = media_devnode_release;
> > if (devnode->parent)
> > devnode->dev.parent = devnode->parent;
> > dev_set_name(&devnode->dev, "media%d", devnode->minor);
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index fb0855b217ce..c6816be0eee8 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -62,6 +62,7 @@ struct media_entity_notify {
> > * request (and thus the buffer) must be available to the driver.
> > * And once a buffer is queued, then the driver can complete
> > * or delete objects from the request before req_queue exits.
> > + * @release: Release the resources of the media device.
> > */
> > struct media_device_ops {
> > int (*link_notify)(struct media_link *link, u32 flags,
> > @@ -70,6 +71,7 @@ struct media_device_ops {
> > void (*req_free)(struct media_request *req);
> > int (*req_validate)(struct media_request *req);
> > void (*req_queue)(struct media_request *req);
> > + void (*release)(struct media_device *mdev);
> > };
> >
> > /**
> > @@ -219,6 +221,30 @@ struct usb_device;
> > */
> > void media_device_init(struct media_device *mdev);
> >
> > +/**
> > + * media_device_get() - Get a reference to a media device
>
> Maybe mimick the get_device() wording and state "atomically increment
> the reference count for the media device" ? Same for put.
Sounds good.
>
> > + *
> > + * @mdev: media device
>
> This should return a pointer to the media_device, as other get functions
> do.
Yes, I'll do that for v3.
>
> > + */
> > +#define media_device_get(mdev) \
> > + do { \
> > + dev_dbg((mdev)->dev, "%s: get media device %s\n", \
> > + __func__, (mdev)->bus_info); \
>
> Do we really need this ? I'd prefer inline functions to ensure type
> safety. If we need to track the get/put callers, I think using ftrace
> would be a better option.
I think we can drop this. It was useful for development though.
>
> > + get_device(&(mdev)->devnode.dev); \
> > + } while (0)
> > +
> > +/**
> > + * media_device_put() - Put a reference to a media device
> > + *
> > + * @mdev: media device
> > + */
> > +#define media_device_put(mdev) \
> > + do { \
> > + dev_dbg((mdev)->dev, "%s: put media device %s\n", \
> > + __func__, (mdev)->bus_info); \
> > + put_device(&(mdev)->devnode.dev); \
> > + } while (0)
> > +
> > /**
> > * media_device_cleanup() - Cleanups a media device element
> > *
> > @@ -432,6 +458,8 @@ void __media_device_usb_init(struct media_device *mdev,
> > const char *driver_name);
> >
> > #else
> > +#define media_device_get(mdev) do { } while (0)
> > +#define media_device_put(mdev) do { } while (0)
> > static inline int media_device_register(struct media_device *mdev)
> > {
> > return 0;
>
@@ -700,6 +700,31 @@ void media_device_unregister_entity_notify(struct media_device *mdev,
}
EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);
+static void __media_device_release(struct media_device *mdev)
+{
+ dev_dbg(mdev->dev, "Media device released\n");
+
+ ida_destroy(&mdev->entity_internal_idx);
+ mdev->entity_internal_idx_max = 0;
+ media_graph_walk_cleanup(&mdev->pm_count_walk);
+ mutex_destroy(&mdev->graph_mutex);
+ mutex_destroy(&mdev->req_queue_mutex);
+}
+
+static void media_device_release(struct media_devnode *devnode)
+{
+ struct media_device *mdev = to_media_device(devnode);
+
+ if (mdev->ops && mdev->ops->release) {
+ /*
+ * If release op isn't set, __media_device_release() is called
+ * via media_device_cleanup().
+ */
+ __media_device_release(mdev);
+ mdev->ops->release(mdev);
+ }
+}
+
void media_device_init(struct media_device *mdev)
{
INIT_LIST_HEAD(&mdev->entities);
@@ -712,6 +737,8 @@ void media_device_init(struct media_device *mdev)
mutex_init(&mdev->graph_mutex);
ida_init(&mdev->entity_internal_idx);
atomic_set(&mdev->request_id, 0);
+
+ mdev->devnode.release = media_device_release;
media_devnode_init(&mdev->devnode);
if (!*mdev->bus_info)
@@ -724,12 +751,9 @@ EXPORT_SYMBOL_GPL(media_device_init);
void media_device_cleanup(struct media_device *mdev)
{
- ida_destroy(&mdev->entity_internal_idx);
- mdev->entity_internal_idx_max = 0;
- media_graph_walk_cleanup(&mdev->pm_count_walk);
- mutex_destroy(&mdev->graph_mutex);
- mutex_destroy(&mdev->req_queue_mutex);
- put_device(&mdev->devnode.dev);
+ WARN_ON(mdev->ops && mdev->ops->release);
+ __media_device_release(mdev);
+ media_device_put(mdev);
}
EXPORT_SYMBOL_GPL(media_device_cleanup);
@@ -59,6 +59,10 @@ static void media_devnode_release(struct device *cd)
{
struct media_devnode *devnode = to_media_devnode(cd);
+ /* If the devnode has a ref, it is simply released by the user. */
+ if (devnode->ref)
+ return;
+
if (devnode->minor != -1)
media_devnode_free_minor(devnode->minor);
@@ -213,6 +217,7 @@ static const struct file_operations media_devnode_fops = {
void media_devnode_init(struct media_devnode *devnode)
{
device_initialize(&devnode->dev);
+ devnode->dev.release = media_devnode_release;
devnode->minor = -1;
}
@@ -246,7 +251,6 @@ int __must_check media_devnode_register(struct media_devnode *devnode,
devnode->dev.bus = &media_bus_type;
devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor);
- devnode->dev.release = media_devnode_release;
if (devnode->parent)
devnode->dev.parent = devnode->parent;
dev_set_name(&devnode->dev, "media%d", devnode->minor);
@@ -62,6 +62,7 @@ struct media_entity_notify {
* request (and thus the buffer) must be available to the driver.
* And once a buffer is queued, then the driver can complete
* or delete objects from the request before req_queue exits.
+ * @release: Release the resources of the media device.
*/
struct media_device_ops {
int (*link_notify)(struct media_link *link, u32 flags,
@@ -70,6 +71,7 @@ struct media_device_ops {
void (*req_free)(struct media_request *req);
int (*req_validate)(struct media_request *req);
void (*req_queue)(struct media_request *req);
+ void (*release)(struct media_device *mdev);
};
/**
@@ -219,6 +221,30 @@ struct usb_device;
*/
void media_device_init(struct media_device *mdev);
+/**
+ * media_device_get() - Get a reference to a media device
+ *
+ * @mdev: media device
+ */
+#define media_device_get(mdev) \
+ do { \
+ dev_dbg((mdev)->dev, "%s: get media device %s\n", \
+ __func__, (mdev)->bus_info); \
+ get_device(&(mdev)->devnode.dev); \
+ } while (0)
+
+/**
+ * media_device_put() - Put a reference to a media device
+ *
+ * @mdev: media device
+ */
+#define media_device_put(mdev) \
+ do { \
+ dev_dbg((mdev)->dev, "%s: put media device %s\n", \
+ __func__, (mdev)->bus_info); \
+ put_device(&(mdev)->devnode.dev); \
+ } while (0)
+
/**
* media_device_cleanup() - Cleanups a media device element
*
@@ -432,6 +458,8 @@ void __media_device_usb_init(struct media_device *mdev,
const char *driver_name);
#else
+#define media_device_get(mdev) do { } while (0)
+#define media_device_put(mdev) do { } while (0)
static inline int media_device_register(struct media_device *mdev)
{
return 0;