Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Hans Verkuil <hansverk@cisco.com>]

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