| Message ID | 1305830080-18211-1-git-send-email-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Hi Laurent, On 05/19/2011 08:34 PM, Laurent Pinchart wrote: > The new v4l2_new_subdev_board() function creates and register a subdev > based on generic board information. The board information structure > includes a bus type and bus type-specific information. > > Only I2C and SPI busses are currently supported. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/video/v4l2-common.c | 70 +++++++++++++++++++++++++++++++++++++ > drivers/media/video/v4l2-device.c | 8 ++++ > include/media/v4l2-common.h | 28 +++++++++++++++ > include/media/v4l2-subdev.h | 3 ++ > 4 files changed, 109 insertions(+), 0 deletions(-) > > Hi everybody, > > This approach has been briefly discussed during the Warsaw V4L meeting. Now > that support for platform subdevs has been requested, I'd like to move bus type > handling to the V4L2 core instead of duplicating the logic in every driver. As > usual, comments will be appreciated. Thanks for taking care of this. > > diff --git a/drivers/media/video/v4l2-common.c b/drivers/media/video/v4l2-common.c > index 06b9f9f..46aee94 100644 > --- a/drivers/media/video/v4l2-common.c > +++ b/drivers/media/video/v4l2-common.c > @@ -474,6 +474,76 @@ EXPORT_SYMBOL_GPL(v4l2_spi_new_subdev); > > #endif /* defined(CONFIG_SPI) */ > > +/* > + * v4l2_new_subdev_board - Register a subdevice based on board information > + * @v4l2_dev: Parent V4L2 device > + * @info: I2C subdevs board information array "info" doesn't appear to be a (I2C subdevs) array. > + * > + * Register a subdevice identified by a geenric board information structure. The s/geenric/generic ? > + * structure contains the bus type and bus type-specific information. > + * > + * Return a pointer to the subdevice if registration was successful, or NULL > + * otherwise. > + */ > +struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev, > + struct v4l2_subdev_board_info *info) > +{ > + struct v4l2_subdev *subdev; > + > + switch (info->type) { > +#if defined(CONFIG_I2C) > + case V4L2_SUBDEV_BUS_TYPE_I2C: { > + struct i2c_adapter *adapter; > + > + adapter = i2c_get_adapter(info->info.i2c.i2c_adapter_id); > + if (adapter == NULL) { > + printk(KERN_ERR "%s: Unable to get I2C adapter %d for " > + "device %s/%u\n", __func__, > + info->info.i2c.i2c_adapter_id, > + info->info.i2c.board_info->type, > + info->info.i2c.board_info->addr); > + return NULL; > + } > + > + subdev = v4l2_i2c_new_subdev_board(v4l2_dev, adapter, > + info->info.i2c.board_info, NULL); > + if (subdev == NULL) { > + i2c_put_adapter(adapter); > + return NULL; > + } > + > + subdev->flags |= V4L2_SUBDEV_FL_RELEASE_ADAPTER; > + break; > + } > +#endif /* defined(CONFIG_I2C) */ > +#if defined(CONFIG_SPI) > + case V4L2_SUBDEV_BUS_TYPE_SPI: { > + struct spi_master *master; > + > + master = spi_busnum_to_master(info->info.spi->bus_num); > + if (master == NULL) { > + printk(KERN_ERR "%s: Unable to get SPI master %u for " > + "device %s/%u\n", __func__, > + info->info.spi->bus_num, > + info->info.spi->modalias, > + info->info.spi->chip_select); > + return NULL; > + } > + > + subdev = v4l2_spi_new_subdev(v4l2_dev, master, info->info.spi); > + spi_master_put(master); > + break; > + } > +#endif /* defined(CONFIG_SPI) */ > + default: > + subdev = NULL; > + break; > + } > + > + return subdev; > +} > +EXPORT_SYMBOL_GPL(v4l2_new_subdev_board); I'm just wondering, while we are at it, if it would be worth to try to make v4l2_i2c_new_subdev_board() and v4l2_spi_new_subdev() race-free. There has been an attempt from Guennadi side to solve this issue, http://thread.gmane.org/gmane.linux.kernel/1069603 After request_module there is nothing preventing subdev's driver module to be unloaded and thus it is not safe to dereference dev->driver->owner. > + > /* Clamp x to be between min and max, aligned to a multiple of 2^align. min > * and max don't have to be aligned, but there must be at least one valid > * value. E.g., min=17,max=31,align=4 is not allowed as there are no multiples > diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c > index 4aae501..cfd9caf 100644 > --- a/drivers/media/video/v4l2-device.c > +++ b/drivers/media/video/v4l2-device.c > @@ -246,5 +246,13 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd) > #endif > video_unregister_device(&sd->devnode); > module_put(sd->owner); > + > +#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && defined(MODULE)) > + if ((sd->flags & V4L2_SUBDEV_FL_IS_I2C) && > + (sd->flags & V4L2_SUBDEV_FL_RELEASE_ADAPTER)) { > + struct i2c_client *client = v4l2_get_subdevdata(sd); > + i2c_put_adapter(client->adapter); > + } > +#endif > } > EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev); > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h > index a298ec4..88c38d9 100644 > --- a/include/media/v4l2-common.h > +++ b/include/media/v4l2-common.h > @@ -171,6 +171,34 @@ void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi, > const struct v4l2_subdev_ops *ops); > #endif > > +/* -------------------------------------------------------------------------- */ > + > +/* Generic helper functions */ > + > +struct v4l2_subdev_i2c_board_info { > + struct i2c_board_info *board_info; > + int i2c_adapter_id; > +}; > + > +enum v4l2_subdev_bus_type { > + V4L2_SUBDEV_BUS_TYPE_NONE, > + V4L2_SUBDEV_BUS_TYPE_I2C, > + V4L2_SUBDEV_BUS_TYPE_SPI, > +}; I had an issue when tried to call request_module, to register subdev of platform device type, in probe() of other platform device. Driver's probe() for devices belonging same bus type cannot be nested as the bus lock is taken by the driver core before entering probe(), so this would lead to a deadlock. That exactly happens in __driver_attach(). For the same reason v4l2_new_subdev_board could not be called from probe() of devices belonging to I2C or SPI bus, as request_module is called inside of it. I'm not sure how to solve it, yet:) > + > +struct v4l2_subdev_board_info { > + enum v4l2_subdev_bus_type type; > + union { > + struct v4l2_subdev_i2c_board_info i2c; > + struct spi_board_info *spi; > + } info; > +}; > + > +/* Create a subdevice and load its module. The info argumentidentifies the > + * subdev bus type and the bus type-specific information. */ > +struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev, > + struct v4l2_subdev_board_info *info); > + > /* ------------------------------------------------------------------------- */ > > /* Note: these remaining ioctls/structs should be removed as well, but they are > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 1562c4f..bc1c4d8 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -483,6 +483,9 @@ struct v4l2_subdev_internal_ops { > #define V4L2_SUBDEV_FL_HAS_DEVNODE (1U << 2) > /* Set this flag if this subdev generates events. */ > #define V4L2_SUBDEV_FL_HAS_EVENTS (1U << 3) > +/* Set by the core if the bus adapter needs to be released. Do NOT use in > + * drivers. */ > +#define V4L2_SUBDEV_FL_RELEASE_ADAPTER (1U << 4) > > /* Each instance of a subdev driver should create this struct, either > stand-alone or embedded in a larger struct. Regards,
Hi Sylwester, On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote: > On 05/19/2011 08:34 PM, Laurent Pinchart wrote: > > The new v4l2_new_subdev_board() function creates and register a subdev > > based on generic board information. The board information structure > > includes a bus type and bus type-specific information. > > > > Only I2C and SPI busses are currently supported. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > > > drivers/media/video/v4l2-common.c | 70 > > +++++++++++++++++++++++++++++++++++++ drivers/media/video/v4l2-device.c > > | 8 ++++ > > include/media/v4l2-common.h | 28 +++++++++++++++ > > include/media/v4l2-subdev.h | 3 ++ > > 4 files changed, 109 insertions(+), 0 deletions(-) > > > > Hi everybody, > > > > This approach has been briefly discussed during the Warsaw V4L meeting. > > Now that support for platform subdevs has been requested, I'd like to > > move bus type handling to the V4L2 core instead of duplicating the logic > > in every driver. As usual, comments will be appreciated. > > Thanks for taking care of this. You're welcome. Thanks for the review. > > diff --git a/drivers/media/video/v4l2-common.c > > b/drivers/media/video/v4l2-common.c index 06b9f9f..46aee94 100644 > > --- a/drivers/media/video/v4l2-common.c > > +++ b/drivers/media/video/v4l2-common.c > > @@ -474,6 +474,76 @@ EXPORT_SYMBOL_GPL(v4l2_spi_new_subdev); > > > > #endif /* defined(CONFIG_SPI) */ > > > > +/* > > + * v4l2_new_subdev_board - Register a subdevice based on board > > information + * @v4l2_dev: Parent V4L2 device > > + * @info: I2C subdevs board information array > > "info" doesn't appear to be a (I2C subdevs) array. Oops, I'll fix it. > > + * > > + * Register a subdevice identified by a geenric board information > > structure. The > > s/geenric/generic ? Ditto. > > + * structure contains the bus type and bus type-specific information. > > + * > > + * Return a pointer to the subdevice if registration was successful, or > > NULL + * otherwise. > > + */ > > +struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev, > > + struct v4l2_subdev_board_info *info) > > +{ > > + struct v4l2_subdev *subdev; > > + > > + switch (info->type) { > > +#if defined(CONFIG_I2C) > > + case V4L2_SUBDEV_BUS_TYPE_I2C: { > > + struct i2c_adapter *adapter; > > + > > + adapter = i2c_get_adapter(info->info.i2c.i2c_adapter_id); > > + if (adapter == NULL) { > > + printk(KERN_ERR "%s: Unable to get I2C adapter %d for " > > + "device %s/%u\n", __func__, > > + info->info.i2c.i2c_adapter_id, > > + info->info.i2c.board_info->type, > > + info->info.i2c.board_info->addr); > > + return NULL; > > + } > > + > > + subdev = v4l2_i2c_new_subdev_board(v4l2_dev, adapter, > > + info->info.i2c.board_info, NULL); > > + if (subdev == NULL) { > > + i2c_put_adapter(adapter); > > + return NULL; > > + } > > + > > + subdev->flags |= V4L2_SUBDEV_FL_RELEASE_ADAPTER; > > + break; > > + } > > +#endif /* defined(CONFIG_I2C) */ > > +#if defined(CONFIG_SPI) > > + case V4L2_SUBDEV_BUS_TYPE_SPI: { > > + struct spi_master *master; > > + > > + master = spi_busnum_to_master(info->info.spi->bus_num); > > + if (master == NULL) { > > + printk(KERN_ERR "%s: Unable to get SPI master %u for " > > + "device %s/%u\n", __func__, > > + info->info.spi->bus_num, > > + info->info.spi->modalias, > > + info->info.spi->chip_select); > > + return NULL; > > + } > > + > > + subdev = v4l2_spi_new_subdev(v4l2_dev, master, info->info.spi); > > + spi_master_put(master); > > + break; > > + } > > +#endif /* defined(CONFIG_SPI) */ > > + default: > > + subdev = NULL; > > + break; > > + } > > + > > + return subdev; > > +} > > +EXPORT_SYMBOL_GPL(v4l2_new_subdev_board); > > I'm just wondering, while we are at it, if it would be worth to try to make > v4l2_i2c_new_subdev_board() and v4l2_spi_new_subdev() race-free. There has > been an attempt from Guennadi side to solve this issue, > http://thread.gmane.org/gmane.linux.kernel/1069603 > After request_module there is nothing preventing subdev's driver module to > be unloaded and thus it is not safe to dereference dev->driver->owner. Please see below. > > + > > > > /* Clamp x to be between min and max, aligned to a multiple of 2^align. > > min > > > > * and max don't have to be aligned, but there must be at least one > > valid * value. E.g., min=17,max=31,align=4 is not allowed as there > > are no multiples > > > > diff --git a/drivers/media/video/v4l2-device.c > > b/drivers/media/video/v4l2-device.c index 4aae501..cfd9caf 100644 > > --- a/drivers/media/video/v4l2-device.c > > +++ b/drivers/media/video/v4l2-device.c > > @@ -246,5 +246,13 @@ void v4l2_device_unregister_subdev(struct > > v4l2_subdev *sd) > > > > #endif > > > > video_unregister_device(&sd->devnode); > > module_put(sd->owner); > > > > + > > +#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && > > defined(MODULE)) + if ((sd->flags & V4L2_SUBDEV_FL_IS_I2C) && > > + (sd->flags & V4L2_SUBDEV_FL_RELEASE_ADAPTER)) { > > + struct i2c_client *client = v4l2_get_subdevdata(sd); > > + i2c_put_adapter(client->adapter); > > + } > > +#endif > > > > } > > EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev); > > > > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h > > index a298ec4..88c38d9 100644 > > --- a/include/media/v4l2-common.h > > +++ b/include/media/v4l2-common.h > > @@ -171,6 +171,34 @@ void v4l2_spi_subdev_init(struct v4l2_subdev *sd, > > struct spi_device *spi, > > > > const struct v4l2_subdev_ops *ops); > > > > #endif > > > > +/* > > ------------------------------------------------------------------------ > > -- */ + > > +/* Generic helper functions */ > > + > > +struct v4l2_subdev_i2c_board_info { > > + struct i2c_board_info *board_info; > > + int i2c_adapter_id; > > +}; > > + > > +enum v4l2_subdev_bus_type { > > + V4L2_SUBDEV_BUS_TYPE_NONE, > > + V4L2_SUBDEV_BUS_TYPE_I2C, > > + V4L2_SUBDEV_BUS_TYPE_SPI, > > +}; > > I had an issue when tried to call request_module, to register subdev of > platform device type, in probe() of other platform device. Driver's > probe() for devices belonging same bus type cannot be nested as the bus > lock is taken by the driver core before entering probe(), so this would > lead to a deadlock. > That exactly happens in __driver_attach(). > > For the same reason v4l2_new_subdev_board could not be called from probe() > of devices belonging to I2C or SPI bus, as request_module is called inside > of it. I'm not sure how to solve it, yet:) Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix the subdev registration issue, including the module load race condition. Michael, you said you have a patch to add platform subdev support, how have you avoided the race condition ? I've been thinking for some time now about removing the module load code completely. I2C, SPI and platform subdevs would be registered either by board code (possibly through the device tree on platforms that suppport it) for embedded platforms, and by host drivers for pluggable hardware (PCI and USB). Module loading would be handled automatically by the kernel module auto loader, but asynchronously instead of synchronously. Bus notifiers would then be used by host drivers to wait for all subdevs to be registered. I'm not sure yet if this approach is viable. Hans, I think we've briefly discussed this (possible quite a long time ago), do you have any opinion ? Guennadi, based on your previous experience trying to use bus notifiers to solve the module load race, what do you think about the idea ? Others, please comment as well :-) > > + > > +struct v4l2_subdev_board_info { > > + enum v4l2_subdev_bus_type type; > > + union { > > + struct v4l2_subdev_i2c_board_info i2c; > > + struct spi_board_info *spi; > > + } info; > > +}; > > + > > +/* Create a subdevice and load its module. The info argumentidentifies > > the + * subdev bus type and the bus type-specific information. */ > > +struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev, > > + struct v4l2_subdev_board_info *info); > > + > > > > /* > > ----------------------------------------------------------------------- > > -- */ > > > > /* Note: these remaining ioctls/structs should be removed as well, but > > they are > > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > index 1562c4f..bc1c4d8 100644 > > --- a/include/media/v4l2-subdev.h > > +++ b/include/media/v4l2-subdev.h > > @@ -483,6 +483,9 @@ struct v4l2_subdev_internal_ops { > > > > #define V4L2_SUBDEV_FL_HAS_DEVNODE (1U << 2) > > /* Set this flag if this subdev generates events. */ > > #define V4L2_SUBDEV_FL_HAS_EVENTS (1U << 3) > > > > +/* Set by the core if the bus adapter needs to be released. Do NOT use > > in + * drivers. */ > > +#define V4L2_SUBDEV_FL_RELEASE_ADAPTER (1U << 4) > > > > /* Each instance of a subdev driver should create this struct, either > > > > stand-alone or embedded in a larger struct. -
On 05/20/2011 09:29 AM, Laurent Pinchart wrote: [snip] >> I had an issue when tried to call request_module, to register subdev of >> platform device type, in probe() of other platform device. Driver's >> probe() for devices belonging same bus type cannot be nested as the bus >> lock is taken by the driver core before entering probe(), so this would >> lead to a deadlock. >> That exactly happens in __driver_attach(). >> >> For the same reason v4l2_new_subdev_board could not be called from probe() >> of devices belonging to I2C or SPI bus, as request_module is called inside >> of it. I'm not sure how to solve it, yet:) > > Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix the > subdev registration issue, including the module load race condition. Michael, > you said you have a patch to add platform subdev support, how have you avoided > the race condition ? I spoke too soon. This deadlock is staring me in the face right now, too. Ouch, indeed. [snip] MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler Registergericht: Amtsgericht Stuttgart, HRB 271090 Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, May 20, 2011 09:29:32 Laurent Pinchart wrote: > Hi Sylwester, > > On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote: > > On 05/19/2011 08:34 PM, Laurent Pinchart wrote: > > > The new v4l2_new_subdev_board() function creates and register a subdev > > > based on generic board information. The board information structure > > > includes a bus type and bus type-specific information. > > > > > > Only I2C and SPI busses are currently supported. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > > > > drivers/media/video/v4l2-common.c | 70 > > > +++++++++++++++++++++++++++++++++++++ drivers/media/video/v4l2-device.c > > > | 8 ++++ > > > include/media/v4l2-common.h | 28 +++++++++++++++ > > > include/media/v4l2-subdev.h | 3 ++ > > > 4 files changed, 109 insertions(+), 0 deletions(-) > > > > > > Hi everybody, > > > > > > This approach has been briefly discussed during the Warsaw V4L meeting. > > > Now that support for platform subdevs has been requested, I'd like to > > > move bus type handling to the V4L2 core instead of duplicating the logic > > > in every driver. As usual, comments will be appreciated. > > > > Thanks for taking care of this. > > You're welcome. Thanks for the review. > > > > diff --git a/drivers/media/video/v4l2-common.c > > > b/drivers/media/video/v4l2-common.c index 06b9f9f..46aee94 100644 > > > --- a/drivers/media/video/v4l2-common.c > > > +++ b/drivers/media/video/v4l2-common.c > > > @@ -474,6 +474,76 @@ EXPORT_SYMBOL_GPL(v4l2_spi_new_subdev); > > > > > > #endif /* defined(CONFIG_SPI) */ > > > > > > +/* > > > + * v4l2_new_subdev_board - Register a subdevice based on board > > > information + * @v4l2_dev: Parent V4L2 device > > > + * @info: I2C subdevs board information array > > > > "info" doesn't appear to be a (I2C subdevs) array. > > Oops, I'll fix it. > > > > + * > > > + * Register a subdevice identified by a geenric board information > > > structure. The > > > > s/geenric/generic ? > > Ditto. > > > > + * structure contains the bus type and bus type-specific information. > > > + * > > > + * Return a pointer to the subdevice if registration was successful, or > > > NULL + * otherwise. > > > + */ > > > +struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev, > > > + struct v4l2_subdev_board_info *info) > > > +{ > > > + struct v4l2_subdev *subdev; > > > + > > > + switch (info->type) { > > > +#if defined(CONFIG_I2C) > > > + case V4L2_SUBDEV_BUS_TYPE_I2C: { > > > + struct i2c_adapter *adapter; > > > + > > > + adapter = i2c_get_adapter(info->info.i2c.i2c_adapter_id); > > > + if (adapter == NULL) { > > > + printk(KERN_ERR "%s: Unable to get I2C adapter %d for " > > > + "device %s/%u\n", __func__, > > > + info->info.i2c.i2c_adapter_id, > > > + info->info.i2c.board_info->type, > > > + info->info.i2c.board_info->addr); > > > + return NULL; > > > + } > > > + > > > + subdev = v4l2_i2c_new_subdev_board(v4l2_dev, adapter, > > > + info->info.i2c.board_info, NULL); > > > + if (subdev == NULL) { > > > + i2c_put_adapter(adapter); > > > + return NULL; > > > + } > > > + > > > + subdev->flags |= V4L2_SUBDEV_FL_RELEASE_ADAPTER; > > > + break; > > > + } > > > +#endif /* defined(CONFIG_I2C) */ > > > +#if defined(CONFIG_SPI) > > > + case V4L2_SUBDEV_BUS_TYPE_SPI: { > > > + struct spi_master *master; > > > + > > > + master = spi_busnum_to_master(info->info.spi->bus_num); > > > + if (master == NULL) { > > > + printk(KERN_ERR "%s: Unable to get SPI master %u for " > > > + "device %s/%u\n", __func__, > > > + info->info.spi->bus_num, > > > + info->info.spi->modalias, > > > + info->info.spi->chip_select); > > > + return NULL; > > > + } > > > + > > > + subdev = v4l2_spi_new_subdev(v4l2_dev, master, info->info.spi); > > > + spi_master_put(master); > > > + break; > > > + } > > > +#endif /* defined(CONFIG_SPI) */ > > > + default: > > > + subdev = NULL; > > > + break; > > > + } > > > + > > > + return subdev; > > > +} > > > +EXPORT_SYMBOL_GPL(v4l2_new_subdev_board); > > > > I'm just wondering, while we are at it, if it would be worth to try to make > > v4l2_i2c_new_subdev_board() and v4l2_spi_new_subdev() race-free. There has > > been an attempt from Guennadi side to solve this issue, > > http://thread.gmane.org/gmane.linux.kernel/1069603 > > After request_module there is nothing preventing subdev's driver module to > > be unloaded and thus it is not safe to dereference dev->driver->owner. > > Please see below. > > > > + > > > > > > /* Clamp x to be between min and max, aligned to a multiple of 2^align. > > > min > > > > > > * and max don't have to be aligned, but there must be at least one > > > valid * value. E.g., min=17,max=31,align=4 is not allowed as there > > > are no multiples > > > > > > diff --git a/drivers/media/video/v4l2-device.c > > > b/drivers/media/video/v4l2-device.c index 4aae501..cfd9caf 100644 > > > --- a/drivers/media/video/v4l2-device.c > > > +++ b/drivers/media/video/v4l2-device.c > > > @@ -246,5 +246,13 @@ void v4l2_device_unregister_subdev(struct > > > v4l2_subdev *sd) > > > > > > #endif > > > > > > video_unregister_device(&sd->devnode); > > > module_put(sd->owner); > > > > > > + > > > +#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && > > > defined(MODULE)) + if ((sd->flags & V4L2_SUBDEV_FL_IS_I2C) && > > > + (sd->flags & V4L2_SUBDEV_FL_RELEASE_ADAPTER)) { > > > + struct i2c_client *client = v4l2_get_subdevdata(sd); > > > + i2c_put_adapter(client->adapter); > > > + } > > > +#endif > > > > > > } > > > EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev); > > > > > > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h > > > index a298ec4..88c38d9 100644 > > > --- a/include/media/v4l2-common.h > > > +++ b/include/media/v4l2-common.h > > > @@ -171,6 +171,34 @@ void v4l2_spi_subdev_init(struct v4l2_subdev *sd, > > > struct spi_device *spi, > > > > > > const struct v4l2_subdev_ops *ops); > > > > > > #endif > > > > > > +/* > > > ------------------------------------------------------------------------ > > > -- */ + > > > +/* Generic helper functions */ > > > + > > > +struct v4l2_subdev_i2c_board_info { > > > + struct i2c_board_info *board_info; > > > + int i2c_adapter_id; > > > +}; > > > + > > > +enum v4l2_subdev_bus_type { > > > + V4L2_SUBDEV_BUS_TYPE_NONE, > > > + V4L2_SUBDEV_BUS_TYPE_I2C, > > > + V4L2_SUBDEV_BUS_TYPE_SPI, > > > +}; > > > > I had an issue when tried to call request_module, to register subdev of > > platform device type, in probe() of other platform device. Driver's > > probe() for devices belonging same bus type cannot be nested as the bus > > lock is taken by the driver core before entering probe(), so this would > > lead to a deadlock. > > That exactly happens in __driver_attach(). > > > > For the same reason v4l2_new_subdev_board could not be called from probe() > > of devices belonging to I2C or SPI bus, as request_module is called inside > > of it. I'm not sure how to solve it, yet:) > > Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix the > subdev registration issue, including the module load race condition. Michael, > you said you have a patch to add platform subdev support, how have you avoided > the race condition ? > > I've been thinking for some time now about removing the module load code > completely. I2C, SPI and platform subdevs would be registered either by board > code (possibly through the device tree on platforms that suppport it) for > embedded platforms, and by host drivers for pluggable hardware (PCI and USB). > Module loading would be handled automatically by the kernel module auto > loader, but asynchronously instead of synchronously. Bus notifiers would then > be used by host drivers to wait for all subdevs to be registered. > > I'm not sure yet if this approach is viable. Hans, I think we've briefly > discussed this (possible quite a long time ago), do you have any opinion ? > Guennadi, based on your previous experience trying to use bus notifiers to > solve the module load race, what do you think about the idea ? Others, please > comment as well :-) It's definitely viable (I believe the required bus notification has been added some time ago), but I am not sure how to implement it in an efficient manner. My initial idea would be to just wait in v4l2_new_subdev_board until you get the notification on the bus (with a timeout, of course). However, I suspect that that does not solve the deadlock, although it would solve the race. As an aside: note that if the module is unloaded right after the request_module, then that will be detected by the code and it will just return an error. It won't oops or anything like that. Personally I don't believe it is worth the effort just to solve this race, since it is highly theoretical. The problem of loading another bus module when in a bus probe function is a separate issue. My initial reaction is: why do you want to do this? Even if you use delayed module loads, you probably still have to wait for them to succeed at a higher-level function. For example: in the probe function of module A it will attempt to load module B. That probably can't succeed as long as you are in A's probe function due to the bus lock. So you can't check for a successful load of B until you return from that probe function and a higher- level function (that likely loaded module A in the first place) does that check. That's all pretty tricky code, and my suggestion would be to simply not do nested module loads from the same bus. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Friday 20 May 2011 10:53:32 Hans Verkuil wrote: > On Friday, May 20, 2011 09:29:32 Laurent Pinchart wrote: > > On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote: > > > On 05/19/2011 08:34 PM, Laurent Pinchart wrote: > > > > The new v4l2_new_subdev_board() function creates and register a > > > > subdev based on generic board information. The board information > > > > structure includes a bus type and bus type-specific information. > > > > > > > > Only I2C and SPI busses are currently supported. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> [snip] > > > I had an issue when tried to call request_module, to register subdev of > > > platform device type, in probe() of other platform device. Driver's > > > probe() for devices belonging same bus type cannot be nested as the bus > > > lock is taken by the driver core before entering probe(), so this would > > > lead to a deadlock. > > > That exactly happens in __driver_attach(). > > > > > > For the same reason v4l2_new_subdev_board could not be called from > > > probe() of devices belonging to I2C or SPI bus, as request_module is > > > called inside of it. I'm not sure how to solve it, yet:) > > > > Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix > > the subdev registration issue, including the module load race condition. > > Michael, you said you have a patch to add platform subdev support, how > > have you avoided the race condition ? > > > > I've been thinking for some time now about removing the module load code > > completely. I2C, SPI and platform subdevs would be registered either by > > board code (possibly through the device tree on platforms that suppport > > it) for embedded platforms, and by host drivers for pluggable hardware > > (PCI and USB). Module loading would be handled automatically by the kernel > > module auto loader, but asynchronously instead of synchronously. Bus > > notifiers would then be used by host drivers to wait for all subdevs to be > > registered. > > > > I'm not sure yet if this approach is viable. Hans, I think we've briefly > > discussed this (possible quite a long time ago), do you have any opinion > > ? Guennadi, based on your previous experience trying to use bus notifiers > > to solve the module load race, what do you think about the idea ? Others, > > please comment as well :-) > > It's definitely viable (I believe the required bus notification has been > added some time ago), but I am not sure how to implement it in an > efficient manner. > > My initial idea would be to just wait in v4l2_new_subdev_board until you > get the notification on the bus (with a timeout, of course). However, I > suspect that that does not solve the deadlock, although it would solve the > race. > > As an aside: note that if the module is unloaded right after the > request_module, then that will be detected by the code and it will just > return an error. It won't oops or anything like that. Personally I don't > believe it is worth the effort just to solve this race, since it is highly > theoretical. > > The problem of loading another bus module when in a bus probe function is a > separate issue. My initial reaction is: why do you want to do this? Even if > you use delayed module loads, you probably still have to wait for them to > succeed at a higher-level function. For example: in the probe function of > module A it will attempt to load module B. That probably can't succeed as > long as you are in A's probe function due to the bus lock. So you can't > check for a successful load of B until you return from that probe function > and a higher- level function (that likely loaded module A in the first > place) does that check. > > That's all pretty tricky code, and my suggestion would be to simply not do > nested module loads from the same bus. That's unfortunately not an option. Most bridge/host devices in embedded systems are platform devices, and they will need to load platform subdevs. We need to fix that. My idea was to use bus notifiers to delay the bridge/host device initialization. The bridge probe() function would pre-initialize the bridge and register notifiers. The driver would then wait until all subdevs are properly registered, and then proceed from to register V4L2 devices from the bus notifier callback (or possible a work queue). There would be no nested probe() calls.
On Friday, May 20, 2011 11:05:00 Laurent Pinchart wrote: > Hi Hans, > > On Friday 20 May 2011 10:53:32 Hans Verkuil wrote: > > On Friday, May 20, 2011 09:29:32 Laurent Pinchart wrote: > > > On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote: > > > > On 05/19/2011 08:34 PM, Laurent Pinchart wrote: > > > > > The new v4l2_new_subdev_board() function creates and register a > > > > > subdev based on generic board information. The board information > > > > > structure includes a bus type and bus type-specific information. > > > > > > > > > > Only I2C and SPI busses are currently supported. > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > [snip] > > > > > I had an issue when tried to call request_module, to register subdev of > > > > platform device type, in probe() of other platform device. Driver's > > > > probe() for devices belonging same bus type cannot be nested as the bus > > > > lock is taken by the driver core before entering probe(), so this would > > > > lead to a deadlock. > > > > That exactly happens in __driver_attach(). > > > > > > > > For the same reason v4l2_new_subdev_board could not be called from > > > > probe() of devices belonging to I2C or SPI bus, as request_module is > > > > called inside of it. I'm not sure how to solve it, yet:) > > > > > > Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix > > > the subdev registration issue, including the module load race condition. > > > Michael, you said you have a patch to add platform subdev support, how > > > have you avoided the race condition ? > > > > > > I've been thinking for some time now about removing the module load code > > > completely. I2C, SPI and platform subdevs would be registered either by > > > board code (possibly through the device tree on platforms that suppport > > > it) for embedded platforms, and by host drivers for pluggable hardware > > > (PCI and USB). Module loading would be handled automatically by the kernel > > > module auto loader, but asynchronously instead of synchronously. Bus > > > notifiers would then be used by host drivers to wait for all subdevs to be > > > registered. > > > > > > I'm not sure yet if this approach is viable. Hans, I think we've briefly > > > discussed this (possible quite a long time ago), do you have any opinion > > > ? Guennadi, based on your previous experience trying to use bus notifiers > > > to solve the module load race, what do you think about the idea ? Others, > > > please comment as well :-) > > > > It's definitely viable (I believe the required bus notification has been > > added some time ago), but I am not sure how to implement it in an > > efficient manner. > > > > My initial idea would be to just wait in v4l2_new_subdev_board until you > > get the notification on the bus (with a timeout, of course). However, I > > suspect that that does not solve the deadlock, although it would solve the > > race. > > > > As an aside: note that if the module is unloaded right after the > > request_module, then that will be detected by the code and it will just > > return an error. It won't oops or anything like that. Personally I don't > > believe it is worth the effort just to solve this race, since it is highly > > theoretical. > > > > The problem of loading another bus module when in a bus probe function is a > > separate issue. My initial reaction is: why do you want to do this? Even if > > you use delayed module loads, you probably still have to wait for them to > > succeed at a higher-level function. For example: in the probe function of > > module A it will attempt to load module B. That probably can't succeed as > > long as you are in A's probe function due to the bus lock. So you can't > > check for a successful load of B until you return from that probe function > > and a higher- level function (that likely loaded module A in the first > > place) does that check. > > > > That's all pretty tricky code, and my suggestion would be to simply not do > > nested module loads from the same bus. > > That's unfortunately not an option. Most bridge/host devices in embedded > systems are platform devices, and they will need to load platform subdevs. We > need to fix that. Good point. > My idea was to use bus notifiers to delay the bridge/host device > initialization. The bridge probe() function would pre-initialize the bridge > and register notifiers. The driver would then wait until all subdevs are > properly registered, and then proceed from to register V4L2 devices from the > bus notifier callback (or possible a work queue). There would be no nested > probe() calls. Would it be an option to create a new platform bus for the subdevs? That would have its own lock. It makes sense from a hierarchical point of view, but I'm not certain about the amount of work involved. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Friday 20 May 2011 11:19:48 Hans Verkuil wrote: > On Friday, May 20, 2011 11:05:00 Laurent Pinchart wrote: > > On Friday 20 May 2011 10:53:32 Hans Verkuil wrote: > > > On Friday, May 20, 2011 09:29:32 Laurent Pinchart wrote: > > > > On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote: > > > > > On 05/19/2011 08:34 PM, Laurent Pinchart wrote: > > > > > > The new v4l2_new_subdev_board() function creates and register a > > > > > > subdev based on generic board information. The board information > > > > > > structure includes a bus type and bus type-specific information. > > > > > > > > > > > > Only I2C and SPI busses are currently supported. > > > > > > > > > > > > Signed-off-by: Laurent Pinchart > > > > > > <laurent.pinchart@ideasonboard.com> > > > > [snip] > > > > > > > I had an issue when tried to call request_module, to register subdev > > > > > of platform device type, in probe() of other platform device. > > > > > Driver's probe() for devices belonging same bus type cannot be > > > > > nested as the bus lock is taken by the driver core before entering > > > > > probe(), so this would lead to a deadlock. > > > > > That exactly happens in __driver_attach(). > > > > > > > > > > For the same reason v4l2_new_subdev_board could not be called from > > > > > probe() of devices belonging to I2C or SPI bus, as request_module > > > > > is called inside of it. I'm not sure how to solve it, yet:) > > > > > > > > Ouch. I wasn't aware of that issue. Looks like it's indeed time to > > > > fix the subdev registration issue, including the module load race > > > > condition. Michael, you said you have a patch to add platform subdev > > > > support, how have you avoided the race condition ? > > > > > > > > I've been thinking for some time now about removing the module load > > > > code completely. I2C, SPI and platform subdevs would be registered > > > > either by board code (possibly through the device tree on platforms > > > > that suppport it) for embedded platforms, and by host drivers for > > > > pluggable hardware (PCI and USB). Module loading would be handled > > > > automatically by the kernel module auto loader, but asynchronously > > > > instead of synchronously. Bus notifiers would then be used by host > > > > drivers to wait for all subdevs to be registered. > > > > > > > > I'm not sure yet if this approach is viable. Hans, I think we've > > > > briefly discussed this (possible quite a long time ago), do you have > > > > any opinion ? Guennadi, based on your previous experience trying to > > > > use bus notifiers to solve the module load race, what do you think > > > > about the idea ? Others, please comment as well :-) > > > > > > It's definitely viable (I believe the required bus notification has > > > been added some time ago), but I am not sure how to implement it in an > > > efficient manner. > > > > > > My initial idea would be to just wait in v4l2_new_subdev_board until > > > you get the notification on the bus (with a timeout, of course). > > > However, I suspect that that does not solve the deadlock, although it > > > would solve the race. > > > > > > As an aside: note that if the module is unloaded right after the > > > request_module, then that will be detected by the code and it will just > > > return an error. It won't oops or anything like that. Personally I > > > don't believe it is worth the effort just to solve this race, since it > > > is highly theoretical. > > > > > > The problem of loading another bus module when in a bus probe function > > > is a separate issue. My initial reaction is: why do you want to do this? > > > Even if you use delayed module loads, you probably still have to wait > > > for them to succeed at a higher-level function. For example: in the > > > probe function of module A it will attempt to load module B. That > > > probably can't succeed as long as you are in A's probe function due to > > > the bus lock. So you can't check for a successful load of B until you > > > return from that probe function and a higher- level function (that > > > likely loaded module A in the first place) does that check. > > > > > > That's all pretty tricky code, and my suggestion would be to simply not > > > do nested module loads from the same bus. > > > > That's unfortunately not an option. Most bridge/host devices in embedded > > systems are platform devices, and they will need to load platform > > subdevs. We need to fix that. > > Good point. > > > My idea was to use bus notifiers to delay the bridge/host device > > initialization. The bridge probe() function would pre-initialize the > > bridge and register notifiers. The driver would then wait until all > > subdevs are properly registered, and then proceed from to register V4L2 > > devices from the bus notifier callback (or possible a work queue). There > > would be no nested probe() calls. > > Would it be an option to create a new platform bus for the subdevs? That > would have its own lock. It makes sense from a hierarchical point of view, > but I'm not certain about the amount of work involved. Do you mean a subdev-platform bus for platform subdevs, or a V4L2 subdev bus for all subdevs ? The first option is possible, but it looks more like a hack to me. If the subdev really is a platform device, it should be handled by the platform bus. I don't think the second option is possible, I2C and SPI subdevs need to sit on an I2C or SPI bus (I could be mistaken though, there's at least one example of a logical bus type in the kernel with the HID bus). Let's also not forget about sub-sub-devices. We need to handle them at some point as well. This being said, I think that the use of platform devices to solve the initial problem can also be considered a hack as well. What we really need is a way to handle subdevs that can't be controlled at all (a video source that continuously delivers data for instance), or that can be controlled through GPIO. What bus should we use for a bus-less subdev ? And for GPIO-based subdevs, should we create a GPIO bus ?
On Friday, May 20, 2011 11:37:24 Laurent Pinchart wrote: > Hi Hans, > > On Friday 20 May 2011 11:19:48 Hans Verkuil wrote: > > On Friday, May 20, 2011 11:05:00 Laurent Pinchart wrote: > > > On Friday 20 May 2011 10:53:32 Hans Verkuil wrote: > > > > On Friday, May 20, 2011 09:29:32 Laurent Pinchart wrote: > > > > > On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote: > > > > > > On 05/19/2011 08:34 PM, Laurent Pinchart wrote: > > > > > > > The new v4l2_new_subdev_board() function creates and register a > > > > > > > subdev based on generic board information. The board information > > > > > > > structure includes a bus type and bus type-specific information. > > > > > > > > > > > > > > Only I2C and SPI busses are currently supported. > > > > > > > > > > > > > > Signed-off-by: Laurent Pinchart > > > > > > > <laurent.pinchart@ideasonboard.com> > > > > > > [snip] > > > > > > > > > I had an issue when tried to call request_module, to register subdev > > > > > > of platform device type, in probe() of other platform device. > > > > > > Driver's probe() for devices belonging same bus type cannot be > > > > > > nested as the bus lock is taken by the driver core before entering > > > > > > probe(), so this would lead to a deadlock. > > > > > > That exactly happens in __driver_attach(). > > > > > > > > > > > > For the same reason v4l2_new_subdev_board could not be called from > > > > > > probe() of devices belonging to I2C or SPI bus, as request_module > > > > > > is called inside of it. I'm not sure how to solve it, yet:) > > > > > > > > > > Ouch. I wasn't aware of that issue. Looks like it's indeed time to > > > > > fix the subdev registration issue, including the module load race > > > > > condition. Michael, you said you have a patch to add platform subdev > > > > > support, how have you avoided the race condition ? > > > > > > > > > > I've been thinking for some time now about removing the module load > > > > > code completely. I2C, SPI and platform subdevs would be registered > > > > > either by board code (possibly through the device tree on platforms > > > > > that suppport it) for embedded platforms, and by host drivers for > > > > > pluggable hardware (PCI and USB). Module loading would be handled > > > > > automatically by the kernel module auto loader, but asynchronously > > > > > instead of synchronously. Bus notifiers would then be used by host > > > > > drivers to wait for all subdevs to be registered. > > > > > > > > > > I'm not sure yet if this approach is viable. Hans, I think we've > > > > > briefly discussed this (possible quite a long time ago), do you have > > > > > any opinion ? Guennadi, based on your previous experience trying to > > > > > use bus notifiers to solve the module load race, what do you think > > > > > about the idea ? Others, please comment as well :-) > > > > > > > > It's definitely viable (I believe the required bus notification has > > > > been added some time ago), but I am not sure how to implement it in an > > > > efficient manner. > > > > > > > > My initial idea would be to just wait in v4l2_new_subdev_board until > > > > you get the notification on the bus (with a timeout, of course). > > > > However, I suspect that that does not solve the deadlock, although it > > > > would solve the race. > > > > > > > > As an aside: note that if the module is unloaded right after the > > > > request_module, then that will be detected by the code and it will just > > > > return an error. It won't oops or anything like that. Personally I > > > > don't believe it is worth the effort just to solve this race, since it > > > > is highly theoretical. > > > > > > > > The problem of loading another bus module when in a bus probe function > > > > is a separate issue. My initial reaction is: why do you want to do this? > > > > Even if you use delayed module loads, you probably still have to wait > > > > for them to succeed at a higher-level function. For example: in the > > > > probe function of module A it will attempt to load module B. That > > > > probably can't succeed as long as you are in A's probe function due to > > > > the bus lock. So you can't check for a successful load of B until you > > > > return from that probe function and a higher- level function (that > > > > likely loaded module A in the first place) does that check. > > > > > > > > That's all pretty tricky code, and my suggestion would be to simply not > > > > do nested module loads from the same bus. > > > > > > That's unfortunately not an option. Most bridge/host devices in embedded > > > systems are platform devices, and they will need to load platform > > > subdevs. We need to fix that. > > > > Good point. > > > > > My idea was to use bus notifiers to delay the bridge/host device > > > initialization. The bridge probe() function would pre-initialize the > > > bridge and register notifiers. The driver would then wait until all > > > subdevs are properly registered, and then proceed from to register V4L2 > > > devices from the bus notifier callback (or possible a work queue). There > > > would be no nested probe() calls. > > > > Would it be an option to create a new platform bus for the subdevs? That > > would have its own lock. It makes sense from a hierarchical point of view, > > but I'm not certain about the amount of work involved. > > Do you mean a subdev-platform bus for platform subdevs, or a V4L2 subdev bus > for all subdevs ? The first option is possible, but it looks more like a hack > to me. If the subdev really is a platform device, it should be handled by the > platform bus. The first. So you have a 'top-level' platform device that wants to load platform subdevs (probably representing internal IP blocks). So it would create its own platform bus that is used to probe those platform subdevs. It is similar to e.g. an I2C device that has internal I2C busses: you would also create nested busses there. BTW, why do these platform subdevs have to be on the platform bus? Why not have standalone subdev drivers that are not on any bus? That's for example what ivtv does for the internal GPIO audio subdev. > I don't think the second option is possible, I2C and SPI subdevs need to sit > on an I2C or SPI bus (I could be mistaken though, there's at least one example > of a logical bus type in the kernel with the HID bus). > > Let's also not forget about sub-sub-devices. We need to handle them at some > point as well. Sub-sub-devices are not a problem by themselves. They are only a problem if they on the same bus. > This being said, I think that the use of platform devices to solve the initial > problem can also be considered a hack as well. What we really need is a way to > handle subdevs that can't be controlled at all (a video source that > continuously delivers data for instance), or that can be controlled through > GPIO. What bus should we use for a bus-less subdev ? And for GPIO-based > subdevs, should we create a GPIO bus ? It is perfectly possible to have bus-less subdevs. See ivtv (I think there are one or two other examples as well). Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, On Friday 20 May 2011 11:52:17 Hans Verkuil wrote: > On Friday, May 20, 2011 11:37:24 Laurent Pinchart wrote: > > On Friday 20 May 2011 11:19:48 Hans Verkuil wrote: > > > On Friday, May 20, 2011 11:05:00 Laurent Pinchart wrote: [snip] > > > > My idea was to use bus notifiers to delay the bridge/host device > > > > initialization. The bridge probe() function would pre-initialize the > > > > bridge and register notifiers. The driver would then wait until all > > > > subdevs are properly registered, and then proceed from to register > > > > V4L2 devices from the bus notifier callback (or possible a work > > > > queue). There would be no nested probe() calls. > > > > > > Would it be an option to create a new platform bus for the subdevs? > > > That would have its own lock. It makes sense from a hierarchical point > > > of view, but I'm not certain about the amount of work involved. > > > > Do you mean a subdev-platform bus for platform subdevs, or a V4L2 subdev > > bus for all subdevs ? The first option is possible, but it looks more > > like a hack to me. If the subdev really is a platform device, it should be > > handled by the platform bus. > > The first. So you have a 'top-level' platform device that wants to load > platform subdevs (probably representing internal IP blocks). So it would > create its own platform bus that is used to probe those platform subdevs. > > It is similar to e.g. an I2C device that has internal I2C busses: you would > also create nested busses there. > > BTW, why do these platform subdevs have to be on the platform bus? Why not > have standalone subdev drivers that are not on any bus? That's for example > what ivtv does for the internal GPIO audio subdev. There's some misunderstanging here. Internal IP blocks don't need to sit on any bus. The host/bridge driver can create subdevs for those blocks directly, as it doesn't need to load a separate driver. The issue comes from external subdevs that offer little control or even none at all. The best example is an FPGA that will feed video data to the bridge in a fixed format without any mean of control, or with just an on/off control through a GPIO. Support for such subdevices need to be handled by a separate driver, so we need a way to load it at runtime. I'm not sure on what bus that driver should sit. > > I don't think the second option is possible, I2C and SPI subdevs need to > > sit on an I2C or SPI bus (I could be mistaken though, there's at least > > one example of a logical bus type in the kernel with the HID bus). > > > > Let's also not forget about sub-sub-devices. We need to handle them at > > some point as well. > > Sub-sub-devices are not a problem by themselves. They are only a problem if > they on the same bus. > > > This being said, I think that the use of platform devices to solve the > > initial problem can also be considered a hack as well. What we really need > > is a way to handle subdevs that can't be controlled at all (a video source > > that continuously delivers data for instance), or that can be controlled > > through GPIO. What bus should we use for a bus-less subdev ? And for > > GPIO-based subdevs, should we create a GPIO bus ? > > It is perfectly possible to have bus-less subdevs. See ivtv (I think there > are one or two other examples as well). But how can we handle bus-less subdevs for embedded devices, where the host (e.g. OMAP3 ISP) doesn't know about the external subdevs (e.g. FPGA controlled by a couple of GPIOs) ?
Hi Hans, On 05/20/2011 11:52 AM, Hans Verkuil wrote: > On Friday, May 20, 2011 11:37:24 Laurent Pinchart wrote: >> Hi Hans, >> >> On Friday 20 May 2011 11:19:48 Hans Verkuil wrote: >>> On Friday, May 20, 2011 11:05:00 Laurent Pinchart wrote: >>>> On Friday 20 May 2011 10:53:32 Hans Verkuil wrote: >>>>> On Friday, May 20, 2011 09:29:32 Laurent Pinchart wrote: >>>>>> On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote: >>>>>>> On 05/19/2011 08:34 PM, Laurent Pinchart wrote: >>>>>>>> The new v4l2_new_subdev_board() function creates and register a >>>>>>>> subdev based on generic board information. The board information >>>>>>>> structure includes a bus type and bus type-specific information. >>>>>>>> >>>>>>>> Only I2C and SPI busses are currently supported. >>>>>>>> >>>>>>>> Signed-off-by: Laurent Pinchart >>>>>>>> <laurent.pinchart@ideasonboard.com> >>>> >>>> [snip] >>>> >>>>>>> I had an issue when tried to call request_module, to register > subdev >>>>>>> of platform device type, in probe() of other platform device. >>>>>>> Driver's probe() for devices belonging same bus type cannot be >>>>>>> nested as the bus lock is taken by the driver core before entering >>>>>>> probe(), so this would lead to a deadlock. >>>>>>> That exactly happens in __driver_attach(). >>>>>>> >>>>>>> For the same reason v4l2_new_subdev_board could not be called from >>>>>>> probe() of devices belonging to I2C or SPI bus, as request_module >>>>>>> is called inside of it. I'm not sure how to solve it, yet:) >>>>>> >>>>>> Ouch. I wasn't aware of that issue. Looks like it's indeed time to >>>>>> fix the subdev registration issue, including the module load race >>>>>> condition. Michael, you said you have a patch to add platform subdev >>>>>> support, how have you avoided the race condition ? >>>>>> >>>>>> I've been thinking for some time now about removing the module load >>>>>> code completely. I2C, SPI and platform subdevs would be registered >>>>>> either by board code (possibly through the device tree on platforms >>>>>> that suppport it) for embedded platforms, and by host drivers for >>>>>> pluggable hardware (PCI and USB). Module loading would be handled >>>>>> automatically by the kernel module auto loader, but asynchronously >>>>>> instead of synchronously. Bus notifiers would then be used by host >>>>>> drivers to wait for all subdevs to be registered. >>>>>> >>>>>> I'm not sure yet if this approach is viable. Hans, I think we've >>>>>> briefly discussed this (possible quite a long time ago), do you have >>>>>> any opinion ? Guennadi, based on your previous experience trying to >>>>>> use bus notifiers to solve the module load race, what do you think >>>>>> about the idea ? Others, please comment as well :-) >>>>> >>>>> It's definitely viable (I believe the required bus notification has >>>>> been added some time ago), but I am not sure how to implement it in an >>>>> efficient manner. >>>>> >>>>> My initial idea would be to just wait in v4l2_new_subdev_board until >>>>> you get the notification on the bus (with a timeout, of course). >>>>> However, I suspect that that does not solve the deadlock, although it >>>>> would solve the race. >>>>> >>>>> As an aside: note that if the module is unloaded right after the >>>>> request_module, then that will be detected by the code and it will > just >>>>> return an error. It won't oops or anything like that. Personally I >>>>> don't believe it is worth the effort just to solve this race, since it >>>>> is highly theoretical. >>>>> >>>>> The problem of loading another bus module when in a bus probe function >>>>> is a separate issue. My initial reaction is: why do you want to do > this? >>>>> Even if you use delayed module loads, you probably still have to wait >>>>> for them to succeed at a higher-level function. For example: in the >>>>> probe function of module A it will attempt to load module B. That >>>>> probably can't succeed as long as you are in A's probe function due to >>>>> the bus lock. So you can't check for a successful load of B until you >>>>> return from that probe function and a higher- level function (that >>>>> likely loaded module A in the first place) does that check. >>>>> >>>>> That's all pretty tricky code, and my suggestion would be to simply > not >>>>> do nested module loads from the same bus. >>>> >>>> That's unfortunately not an option. Most bridge/host devices in embedded >>>> systems are platform devices, and they will need to load platform >>>> subdevs. We need to fix that. >>> >>> Good point. >>> >>>> My idea was to use bus notifiers to delay the bridge/host device >>>> initialization. The bridge probe() function would pre-initialize the >>>> bridge and register notifiers. The driver would then wait until all >>>> subdevs are properly registered, and then proceed from to register V4L2 >>>> devices from the bus notifier callback (or possible a work queue). There >>>> would be no nested probe() calls. >>> >>> Would it be an option to create a new platform bus for the subdevs? That >>> would have its own lock. It makes sense from a hierarchical point of view, >>> but I'm not certain about the amount of work involved. >> >> Do you mean a subdev-platform bus for platform subdevs, or a V4L2 subdev bus >> for all subdevs ? The first option is possible, but it looks more like a > hack >> to me. If the subdev really is a platform device, it should be handled by > the >> platform bus. > > The first. So you have a 'top-level' platform device that wants to load > platform subdevs (probably representing internal IP blocks). So it would > create its own platform bus that is used to probe those platform subdevs. > > It is similar to e.g. an I2C device that has internal I2C busses: you would > also create nested busses there. > > BTW, why do these platform subdevs have to be on the platform bus? Why not > have standalone subdev drivers that are not on any bus? That's for example > what ivtv does for the internal GPIO audio subdev. Platform devices can have dependencies on their bus drivers. Power/clock domains can be one of the examples. Mostly host and subdev driver will belong to same power domain though. There still might be some other side effects from ripping platform device off from it's bus I'm not aware of right now. > >> I don't think the second option is possible, I2C and SPI subdevs need to sit >> on an I2C or SPI bus (I could be mistaken though, there's at least one > example >> of a logical bus type in the kernel with the HID bus). >> >> Let's also not forget about sub-sub-devices. We need to handle them at some >> point as well. > > Sub-sub-devices are not a problem by themselves. They are only a problem if > they on the same bus. > >> This being said, I think that the use of platform devices to solve the > initial >> problem can also be considered a hack as well. What we really need is a way > to >> handle subdevs that can't be controlled at all (a video source that >> continuously delivers data for instance), or that can be controlled through >> GPIO. What bus should we use for a bus-less subdev ? And for GPIO-based >> subdevs, should we create a GPIO bus ? > > It is perfectly possible to have bus-less subdevs. See ivtv (I think there are > one or two other examples as well). > Regards,
On Fri, 20 May 2011, Laurent Pinchart wrote: > Hi Sylwester, > > On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote: [snip] > > I had an issue when tried to call request_module, to register subdev of > > platform device type, in probe() of other platform device. Driver's > > probe() for devices belonging same bus type cannot be nested as the bus > > lock is taken by the driver core before entering probe(), so this would > > lead to a deadlock. > > That exactly happens in __driver_attach(). > > > > For the same reason v4l2_new_subdev_board could not be called from probe() > > of devices belonging to I2C or SPI bus, as request_module is called inside > > of it. I'm not sure how to solve it, yet:) > > Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix the > subdev registration issue, including the module load race condition. Michael, > you said you have a patch to add platform subdev support, how have you avoided > the race condition ? > > I've been thinking for some time now about removing the module load code > completely. I2C, SPI and platform subdevs would be registered either by board > code (possibly through the device tree on platforms that suppport it) for > embedded platforms, and by host drivers for pluggable hardware (PCI and USB). > Module loading would be handled automatically by the kernel module auto > loader, but asynchronously instead of synchronously. Bus notifiers would then > be used by host drivers to wait for all subdevs to be registered. Sorry, I'm probably missing something. The reason for this module loading was, that you cannot probe i2c sensors before the host is initialised and has turned the master clock on. If you want to go back to the traditional platform-based I2C device registration, you'll have to wait in your sensor (subdev) probe function for host registration, which wouldn't be a good thing to do, IMHO. > I'm not sure yet if this approach is viable. Hans, I think we've briefly > discussed this (possible quite a long time ago), do you have any opinion ? > Guennadi, based on your previous experience trying to use bus notifiers to > solve the module load race, what do you think about the idea ? Others, please > comment as well :-) Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Friday 20 May 2011 15:08:00 Guennadi Liakhovetski wrote: > On Fri, 20 May 2011, Laurent Pinchart wrote: > > On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote: > [snip] > > > > I had an issue when tried to call request_module, to register subdev of > > > platform device type, in probe() of other platform device. Driver's > > > probe() for devices belonging same bus type cannot be nested as the bus > > > lock is taken by the driver core before entering probe(), so this would > > > lead to a deadlock. > > > That exactly happens in __driver_attach(). > > > > > > For the same reason v4l2_new_subdev_board could not be called from > > > probe() of devices belonging to I2C or SPI bus, as request_module is > > > called inside of it. I'm not sure how to solve it, yet:) > > > > Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix > > the subdev registration issue, including the module load race condition. > > Michael, you said you have a patch to add platform subdev support, how > > have you avoided the race condition ? > > > > I've been thinking for some time now about removing the module load code > > completely. I2C, SPI and platform subdevs would be registered either by > > board code (possibly through the device tree on platforms that suppport > > it) for embedded platforms, and by host drivers for pluggable hardware > > (PCI and USB). Module loading would be handled automatically by the > > kernel module auto loader, but asynchronously instead of synchronously. > > Bus notifiers would then be used by host drivers to wait for all subdevs > > to be registered. > > Sorry, I'm probably missing something. The reason for this module loading > was, that you cannot probe i2c sensors before the host is initialised and > has turned the master clock on. Only when the subdev clock is provided by the host. And worse than that, when clock configuration needs to go through board code, you often need to wait until the host has registered the subdev before the board code can be executed. That's why many subdev drivers only initialize a couple of structures in their probe() function, and don't access the hardware until the registered() callback is called. > If you want to go back to the traditional platform-based I2C device > registration, you'll have to wait in your sensor (subdev) probe function for > host registration, which wouldn't be a good thing to do, IMHO. Waiting for host initialization in the subdev probe function is definitely not a good option. There are various dependencies between the host and the subdevs. The host can't obviously proceed before all subdevs are ready, and subdevs often depend on the host to provide clocks and power. Initializing the host first, making the host register the subdev devices and waiting synchronously for them to be initialized is the option we went for. Unfortunately this brings several issues, such as deadlocks when the host and the subdevs sit on the same bus. Another option I'm proposing is to let Linux load modules and initialize devices without interfering with that. Host drivers would be notified that all subdevs are ready through bus notifiers, and subdev drivers would be notified that they can access host resources through the registered callback. This splits initialization in two parts. I'm of course open to other options. > > I'm not sure yet if this approach is viable. Hans, I think we've briefly > > discussed this (possible quite a long time ago), do you have any opinion > > ? Guennadi, based on your previous experience trying to use bus > > notifiers to solve the module load race, what do you think about the > > idea ? Others, please comment as well :-)
diff --git a/drivers/media/video/v4l2-common.c b/drivers/media/video/v4l2-common.c index 06b9f9f..46aee94 100644 --- a/drivers/media/video/v4l2-common.c +++ b/drivers/media/video/v4l2-common.c @@ -474,6 +474,76 @@ EXPORT_SYMBOL_GPL(v4l2_spi_new_subdev); #endif /* defined(CONFIG_SPI) */ +/* + * v4l2_new_subdev_board - Register a subdevice based on board information + * @v4l2_dev: Parent V4L2 device + * @info: I2C subdevs board information array + * + * Register a subdevice identified by a geenric board information structure. The + * structure contains the bus type and bus type-specific information. + * + * Return a pointer to the subdevice if registration was successful, or NULL + * otherwise. + */ +struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev, + struct v4l2_subdev_board_info *info) +{ + struct v4l2_subdev *subdev; + + switch (info->type) { +#if defined(CONFIG_I2C) + case V4L2_SUBDEV_BUS_TYPE_I2C: { + struct i2c_adapter *adapter; + + adapter = i2c_get_adapter(info->info.i2c.i2c_adapter_id); + if (adapter == NULL) { + printk(KERN_ERR "%s: Unable to get I2C adapter %d for " + "device %s/%u\n", __func__, + info->info.i2c.i2c_adapter_id, + info->info.i2c.board_info->type, + info->info.i2c.board_info->addr); + return NULL; + } + + subdev = v4l2_i2c_new_subdev_board(v4l2_dev, adapter, + info->info.i2c.board_info, NULL); + if (subdev == NULL) { + i2c_put_adapter(adapter); + return NULL; + } + + subdev->flags |= V4L2_SUBDEV_FL_RELEASE_ADAPTER; + break; + } +#endif /* defined(CONFIG_I2C) */ +#if defined(CONFIG_SPI) + case V4L2_SUBDEV_BUS_TYPE_SPI: { + struct spi_master *master; + + master = spi_busnum_to_master(info->info.spi->bus_num); + if (master == NULL) { + printk(KERN_ERR "%s: Unable to get SPI master %u for " + "device %s/%u\n", __func__, + info->info.spi->bus_num, + info->info.spi->modalias, + info->info.spi->chip_select); + return NULL; + } + + subdev = v4l2_spi_new_subdev(v4l2_dev, master, info->info.spi); + spi_master_put(master); + break; + } +#endif /* defined(CONFIG_SPI) */ + default: + subdev = NULL; + break; + } + + return subdev; +} +EXPORT_SYMBOL_GPL(v4l2_new_subdev_board); + /* Clamp x to be between min and max, aligned to a multiple of 2^align. min * and max don't have to be aligned, but there must be at least one valid * value. E.g., min=17,max=31,align=4 is not allowed as there are no multiples diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c index 4aae501..cfd9caf 100644 --- a/drivers/media/video/v4l2-device.c +++ b/drivers/media/video/v4l2-device.c @@ -246,5 +246,13 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd) #endif video_unregister_device(&sd->devnode); module_put(sd->owner); + +#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && defined(MODULE)) + if ((sd->flags & V4L2_SUBDEV_FL_IS_I2C) && + (sd->flags & V4L2_SUBDEV_FL_RELEASE_ADAPTER)) { + struct i2c_client *client = v4l2_get_subdevdata(sd); + i2c_put_adapter(client->adapter); + } +#endif } EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev); diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h index a298ec4..88c38d9 100644 --- a/include/media/v4l2-common.h +++ b/include/media/v4l2-common.h @@ -171,6 +171,34 @@ void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi, const struct v4l2_subdev_ops *ops); #endif +/* -------------------------------------------------------------------------- */ + +/* Generic helper functions */ + +struct v4l2_subdev_i2c_board_info { + struct i2c_board_info *board_info; + int i2c_adapter_id; +}; + +enum v4l2_subdev_bus_type { + V4L2_SUBDEV_BUS_TYPE_NONE, + V4L2_SUBDEV_BUS_TYPE_I2C, + V4L2_SUBDEV_BUS_TYPE_SPI, +}; + +struct v4l2_subdev_board_info { + enum v4l2_subdev_bus_type type; + union { + struct v4l2_subdev_i2c_board_info i2c; + struct spi_board_info *spi; + } info; +}; + +/* Create a subdevice and load its module. The info argumentidentifies the + * subdev bus type and the bus type-specific information. */ +struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev, + struct v4l2_subdev_board_info *info); + /* ------------------------------------------------------------------------- */ /* Note: these remaining ioctls/structs should be removed as well, but they are diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index 1562c4f..bc1c4d8 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -483,6 +483,9 @@ struct v4l2_subdev_internal_ops { #define V4L2_SUBDEV_FL_HAS_DEVNODE (1U << 2) /* Set this flag if this subdev generates events. */ #define V4L2_SUBDEV_FL_HAS_EVENTS (1U << 3) +/* Set by the core if the bus adapter needs to be released. Do NOT use in + * drivers. */ +#define V4L2_SUBDEV_FL_RELEASE_ADAPTER (1U << 4) /* Each instance of a subdev driver should create this struct, either stand-alone or embedded in a larger struct.
The new v4l2_new_subdev_board() function creates and register a subdev based on generic board information. The board information structure includes a bus type and bus type-specific information. Only I2C and SPI busses are currently supported. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/video/v4l2-common.c | 70 +++++++++++++++++++++++++++++++++++++ drivers/media/video/v4l2-device.c | 8 ++++ include/media/v4l2-common.h | 28 +++++++++++++++ include/media/v4l2-subdev.h | 3 ++ 4 files changed, 109 insertions(+), 0 deletions(-) Hi everybody, This approach has been briefly discussed during the Warsaw V4L meeting. Now that support for platform subdevs has been requested, I'd like to move bus type handling to the V4L2 core instead of duplicating the logic in every driver. As usual, comments will be appreciated.