Hi Sakari,
Thank you for the patch.
On Tuesday 17 December 2013 15:49:24 Sakari Ailus wrote:
> When the sub-device is registered, increment the use count of the sub-device
> owner only if it's different from the owner of the driver for the media
> device. This avoids increasing the use count by the module itself and thus
> making it possible to unload it when it's not in use.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
This looks good to me, but I wonder whether a more generic solution won't be
needed, to solve the multiple circular reference issues we (will) have with
subdevices and clocks. My gut feeling is that such a generic solution will
also cater for the needs of the problem you're trying to solve here.
This being said, there's no reason to delay this patch until a more generic
solution is available, so
Acked-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> Changes since v1:
>
> - Check that v4l2_dev->dev and v4l2_dev->dev->driver are non-NULL before
> using them.
> - Store the information on the same owner into struct v4l2_subdev. This
> avoids issues related to unregistering subdevs through
> v4l2_device_unregister().
>
> drivers/media/v4l2-core/v4l2-device.c | 18 +++++++++++++++---
> include/media/v4l2-subdev.h | 1 +
> 2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-device.c
> b/drivers/media/v4l2-core/v4l2-device.c index 02d1b63..015f92a 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -158,7 +158,17 @@ int v4l2_device_register_subdev(struct v4l2_device
> *v4l2_dev, /* Warn if we apparently re-register a subdev */
> WARN_ON(sd->v4l2_dev != NULL);
>
> - if (!try_module_get(sd->owner))
> + /*
> + * The reason to acquire the module here is to avoid unloading
> + * a module of sub-device which is registered to a media
> + * device. To make it possible to unload modules for media
> + * devices that also register sub-devices, do not
> + * try_module_get() such sub-device owners.
> + */
> + sd->owner_v4l2_dev = v4l2_dev->dev && v4l2_dev->dev->driver &&
> + sd->owner == v4l2_dev->dev->driver->owner;
> +
> + if (!sd->owner_v4l2_dev && !try_module_get(sd->owner))
> return -ENODEV;
>
> sd->v4l2_dev = v4l2_dev;
> @@ -192,7 +202,8 @@ error_unregister:
> if (sd->internal_ops && sd->internal_ops->unregistered)
> sd->internal_ops->unregistered(sd);
> error_module:
> - module_put(sd->owner);
> + if (!sd->owner_v4l2_dev)
> + module_put(sd->owner);
> sd->v4l2_dev = NULL;
> return err;
> }
> @@ -280,6 +291,7 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev
> *sd) }
> #endif
> video_unregister_device(sd->devnode);
> - module_put(sd->owner);
> + if (!sd->owner_v4l2_dev)
> + module_put(sd->owner);
> }
> EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d67210a..6d03b54 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -579,6 +579,7 @@ struct v4l2_subdev {
> #endif
> struct list_head list;
> struct module *owner;
> + bool owner_v4l2_dev;
> u32 flags;
> struct v4l2_device *v4l2_dev;
> const struct v4l2_subdev_ops *ops;
--
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
