| Message ID | 1387288164-15250-1-git-send-email-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> 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@ideasonboard.com> > --- > 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;
Hi Laurent, Laurent Pinchart wrote: > 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@linux.intel.com> > > 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. I can't immediately think of solving this in a generic fashion. There are dependencies to API behaviour for instance. For clocks this could be resolved by changing how clk_get() is used by sensor drivers, or changing the clock framework to allow unregistering clocks even if they have been obtained by the users but not enabled. Considering the current implementation of clk_unregister(), the need for (some) changes is apparent. (I could miss some changes elsewhere as I just checked linux-media.) The above would still resolve this for clocks alone. > 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@ideasonboard.com> Thanks!
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;
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@linux.intel.com> --- 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(-)