Re: [PATCH 2/2] media: V4L2: support asynchronous subdevice registration

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

Hi,

On 10/24/2012 03:54 PM, Guennadi Liakhovetski wrote:
> On Sat, 20 Oct 2012, Guennadi Liakhovetski wrote:
> 
>> Currently bridge device drivers register devices for all subdevices
>> synchronously, tupically, during their probing. E.g. if an I2C CMOS sensor
>> is attached to a video bridge device, the bridge driver will create an I2C
>> device and wait for the respective I2C driver to probe. This makes linking
>> of devices straight forward, but this approach cannot be used with
>> intrinsically asynchronous and unordered device registration systems like
>> the Flattened Device Tree. To support such systems this patch adds an
>> asynchronous subdevice registration framework to V4L2. To use it respective
>> (e.g. I2C) subdevice drivers must request deferred probing as long as their
>> bridge driver hasn't probed. The bridge driver during its probing submits a
>> an arbitrary number of subdevice descriptor groups to the framework to
>> manage. After that it can add callbacks to each of those groups to be
>> called at various stages during subdevice probing, e.g. after completion.
>> Then the bridge driver can request single groups to be probed, finish its
>> own probing and continue its video subsystem configuration from its
>> callbacks.
>>
>> Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@gmx.de>
> 
> Sorry, I did indeed forget to include you on CC for this patch as promised
> in https://lkml.org/lkml/2012/10/17/306. In the patch below just look for
> the only occurrance of the device_release_driver() / device_attach(). In
> your mail you said, that only bus drivers should do this. In fact this is
> indeed what is happening here. A device is attached to two busses:
> (typically) I2C and "media." And the code below is called when the device
> is detached from the media bus.
> 
> Thanks
> Guennadi
> 
>> ---
>>
>> One more thing to note about this patch. Subdevice drivers, supporting
>> asynchronous probing, and using this framework, need a unified way to
>> detect, whether their probing should succeed or they should request
>> deferred probing. I implement this using device platform data. This means,
>> that all subdevice drivers, wishing to use this API will have to use the
>> same platform data struct. I don't think this is a major inconvenience,
>> but if we decide against this, we'll have to add a V4L2 function to verify
>> "are you ready for me or not." The latter would be inconvenient, because
>> then we would have to look through all registered subdevice descriptor
>> groups for this specific subdevice.
>>
>>   drivers/media/v4l2-core/Makefile      |    3 +-
>>   drivers/media/v4l2-core/v4l2-async.c  |  249 +++++++++++++++++++++++++++++++++
>>   drivers/media/v4l2-core/v4l2-device.c |    2 +
>>   include/media/v4l2-async.h            |   88 ++++++++++++
>>   include/media/v4l2-device.h           |    6 +
>>   include/media/v4l2-subdev.h           |   16 ++
>>   6 files changed, 363 insertions(+), 1 deletions(-)
>>   create mode 100644 drivers/media/v4l2-core/v4l2-async.c
>>   create mode 100644 include/media/v4l2-async.h
>>
>> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
>> index cb5fede..074e01c 100644
>> --- a/drivers/media/v4l2-core/Makefile
>> +++ b/drivers/media/v4l2-core/Makefile
>> @@ -5,7 +5,8 @@
>>   tuner-objs	:=	tuner-core.o
>>
>>   videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
>> -			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o
>> +			v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o \
>> +			v4l2-async.o
>>   ifeq ($(CONFIG_COMPAT),y)
>>     videodev-objs += v4l2-compat-ioctl32.o
>>   endif
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>> new file mode 100644
>> index 0000000..871f116
>> --- /dev/null
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -0,0 +1,249 @@
>> +/*
>> + * V4L2 asynchronous subdevice registration API
>> + *
>> + * Copyright (C) 2012, Guennadi Liakhovetski<g.liakhovetski@gmx.de>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include<linux/device.h>
>> +#include<linux/err.h>
>> +#include<linux/i2c.h>
>> +#include<linux/list.h>
>> +#include<linux/module.h>
>> +#include<linux/mutex.h>
>> +#include<linux/notifier.h>
>> +#include<linux/platform_device.h>
>> +#include<linux/slab.h>
>> +#include<linux/types.h>
>> +
>> +#include<media/v4l2-async.h>
>> +#include<media/v4l2-device.h>
>> +#include<media/v4l2-subdev.h>
>> +
>> +static bool match_i2c(struct device *dev, struct v4l2_async_hw_device *hw_dev)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	return hw_dev->bus_type = V4L2_ASYNC_BUS_I2C&&
>> +		hw_dev->match.i2c.adapter_id = client->adapter->nr&&
>> +		hw_dev->match.i2c.address = client->addr;
>> +}
>> +
>> +static bool match_platform(struct device *dev, struct v4l2_async_hw_device *hw_dev)
>> +{
>> +	return hw_dev->bus_type = V4L2_ASYNC_BUS_PLATFORM&&
>> +		!strcmp(hw_dev->match.platform.name, dev_name(dev));
>> +}
>> +
>> +/*
>> + * I think, notifiers on different busses can run concurrently, so, we have to
>> + * protect common data, e.g. sub-device lists.
>> + */
>> +static int async_notifier_cb(struct v4l2_async_group *group,
>> +		unsigned long action, struct device *dev,
>> +		bool (*match)(struct device *, struct v4l2_async_hw_device *))
>> +{
>> +	struct v4l2_device *v4l2_dev = group->v4l2_dev;
>> +	struct v4l2_async_subdev *asd;
>> +	bool done;
>> +	int ret;
>> +
>> +	if (action != BUS_NOTIFY_BOUND_DRIVER&&
>> +	    action != BUS_NOTIFY_BIND_DRIVER)
>> +		return NOTIFY_DONE;
>> +
>> +	/* Asynchronous: have to lock */
>> +	mutex_lock(&v4l2_dev->group_lock);
>> +
>> +	list_for_each_entry(asd,&group->group, list) {
>> +		if (match(dev,&asd->hw))
>> +			break;
>> +	}
>> +
>> +	if (&asd->list =&group->group) {
>> +		/* Not our device */
>> +		mutex_unlock(&v4l2_dev->group_lock);
>> +		return NOTIFY_DONE;
>> +	}
>> +
>> +	asd->dev = dev;
>> +
>> +	if (action = BUS_NOTIFY_BIND_DRIVER) {
>> +		/*
>> +		 * Provide platform data to the driver: it can complete probing
>> +		 * now.
>> +		 */
>> +		dev->platform_data =&asd->sdpd;
>> +		mutex_unlock(&v4l2_dev->group_lock);
>> +		if (group->bind_cb)
>> +			group->bind_cb(group, asd);
>> +		return NOTIFY_OK;
>> +	}
>> +
>> +	/* BUS_NOTIFY_BOUND_DRIVER */
>> +	if (asd->hw.bus_type = V4L2_ASYNC_BUS_I2C)
>> +		asd->sdpd.subdev = i2c_get_clientdata(to_i2c_client(dev));
>> +	/*
>> +	 * Non-I2C subdevice drivers should take care to assign their subdevice
>> +	 * pointers
>> +	 */
>> +	ret = v4l2_device_register_subdev(v4l2_dev,
>> +					  asd->sdpd.subdev);
>> +	if (ret<  0) {
>> +		mutex_unlock(&v4l2_dev->group_lock);
>> +		/* FIXME: error, clean up world? */
>> +		dev_err(dev, "Failed registering a subdev: %d\n", ret);
>> +		return NOTIFY_OK;
>> +	}
>> +	list_move(&asd->list,&group->done);
>> +
>> +	/* Client probed&  all subdev drivers collected */
>> +	done = list_empty(&group->group);
>> +
>> +	mutex_unlock(&v4l2_dev->group_lock);
>> +
>> +	if (group->bound_cb)
>> +		group->bound_cb(group, asd);
>> +
>> +	if (done&&  group->complete_cb)
>> +		group->complete_cb(group);
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static int platform_cb(struct notifier_block *nb,
>> +		       unsigned long action, void *data)
>> +{
>> +	struct device *dev = data;
>> +	struct v4l2_async_group *group = container_of(nb, struct v4l2_async_group,
>> +						     platform_notifier);
>> +
>> +	return async_notifier_cb(group, action, dev, match_platform);
>> +}
>> +
>> +static int i2c_cb(struct notifier_block *nb,
>> +		  unsigned long action, void *data)
>> +{
>> +	struct device *dev = data;
>> +	struct v4l2_async_group *group = container_of(nb, struct v4l2_async_group,
>> +						     i2c_notifier);
>> +
>> +	return async_notifier_cb(group, action, dev, match_i2c);
>> +}
>> +
>> +/*
>> + * Typically this function will be called during bridge driver probing. It
>> + * installs bus notifiers to handle asynchronously probing subdevice drivers.
>> + * Once the bridge driver probing completes, subdevice drivers, waiting in
>> + * EPROBE_DEFER state are re-probed, at which point they get their platform
>> + * data, which allows them to complete probing.
>> + */
>> +int v4l2_async_group_probe(struct v4l2_async_group *group)
>> +{
>> +	struct v4l2_async_subdev *asd, *tmp;
>> +	bool i2c_used = false, platform_used = false;
>> +	int ret;
>> +
>> +	/* This group is inactive so far - no notifiers yet */
>> +	list_for_each_entry_safe(asd, tmp,&group->group, list) {
>> +		if (asd->sdpd.subdev) {
>> +			/* Simulate a BIND event */
>> +			if (group->bind_cb)
>> +				group->bind_cb(group, asd);
>> +

Still we can't be sure at this moment asd->sdpd.subdev's driver is
valid and not unloaded, can we ? 

In the case when a sub-device driver is probed after the host driver
(a caller of this function) I assume doing

	asd->sdpd.subdev = i2c_get_clientdata(to_i2c_client(dev));
	...
	ret = v4l2_device_register_subdev(v4l2_dev, asd->sdpd.subdev);

is safe, because it is done in the i2c bus notifier callback itself,
i.e. under device_lock(dev). 

But for these already probed sub-devices, how do we prevent races from 
subdev module unloading ? By not setting CONFIG_MODULE_UNLOAD?... ;)

>> +			/* Already probed, don't wait for it */
>> +			ret = v4l2_device_register_subdev(group->v4l2_dev,
>> +							  asd->sdpd.subdev);
>> +
>> +			if (ret<  0)
>> +				return ret;
>> +
>> +			list_move(&asd->list,&group->done);
>> +			continue;
>> +		}
>> +
>> +		switch (asd->hw.bus_type) {
>> +		case V4L2_ASYNC_BUS_PLATFORM:
>> +			platform_used = true;
>> +			break;
>> +		case V4L2_ASYNC_BUS_I2C:
>> +			i2c_used = true;
>> +		}
>> +	}
>> +
>> +	if (list_empty(&group->group)) {
>> +		if (group->complete_cb)
>> +			group->complete_cb(group);
>> +		return 0;
>> +	}
>> +
>> +	/* TODO: so far bus_register_notifier() never fails */
>> +	if (platform_used) {
>> +		group->platform_notifier.notifier_call = platform_cb;
>> +		bus_register_notifier(&platform_bus_type,
>> +				&group->platform_notifier);
>> +	}
>> +
>> +	if (i2c_used) {
>> +		group->i2c_notifier.notifier_call = i2c_cb;
>> +		bus_register_notifier(&i2c_bus_type,
>> +				&group->i2c_notifier);
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(v4l2_async_group_probe);
>> +
>> +int v4l2_async_group_init(struct v4l2_device *v4l2_dev,
>> +			  struct v4l2_async_group *group,
>> +			  struct v4l2_async_subdev *asd, int cnt)
>> +{
>> +	int i;
>> +
>> +	if (!group)
>> +		return -EINVAL;
>> +
>> +	INIT_LIST_HEAD(&group->group);
>> +	INIT_LIST_HEAD(&group->done);
>> +	group->v4l2_dev = v4l2_dev;
>> +
>> +	for (i = 0; i<  cnt; i++)
>> +		list_add_tail(&asd[i].list,&group->group);
>> +
>> +	/* Protect the global V4L2 device group list */
>> +	mutex_lock(&v4l2_dev->group_lock);
>> +	list_add_tail(&group->list,&v4l2_dev->group_head);
>> +	mutex_unlock(&v4l2_dev->group_lock);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(v4l2_async_group_init);
>> +
>> +void v4l2_async_group_release(struct v4l2_async_group *group)
>> +{
>> +	struct v4l2_async_subdev *asd, *tmp;
>> +
>> +	/* Also no problem, if notifiers haven't been registered */
>> +	bus_unregister_notifier(&platform_bus_type,
>> +				&group->platform_notifier);
>> +	bus_unregister_notifier(&i2c_bus_type,
>> +				&group->i2c_notifier);
>> +
>> +	mutex_lock(&group->v4l2_dev->group_lock);
>> +	list_del(&group->list);
>> +	mutex_unlock(&group->v4l2_dev->group_lock);
>> +
>> +	list_for_each_entry_safe(asd, tmp,&group->done, list) {
>> +		v4l2_device_unregister_subdev(asd->sdpd.subdev);
>> +		/* If we handled USB devices, we'd have to lock the parent too */
>> +		device_release_driver(asd->dev);
>> +		asd->dev->platform_data = NULL;
>> +		if (device_attach(asd->dev)<= 0)
>> +			dev_dbg(asd->dev, "Failed to re-probe to %s\n", asd->dev->driver ?
>> +				asd->dev->driver->name : "(none)");
>> +		list_del(&asd->list);
>> +	}
>> +}
>> +EXPORT_SYMBOL(v4l2_async_group_release);
>> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
>> index 513969f..52faf2f 100644
>> --- a/drivers/media/v4l2-core/v4l2-device.c
>> +++ b/drivers/media/v4l2-core/v4l2-device.c
>> @@ -40,6 +40,8 @@ int v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
>>   	mutex_init(&v4l2_dev->ioctl_lock);
>>   	v4l2_prio_init(&v4l2_dev->prio);
>>   	kref_init(&v4l2_dev->ref);
>> +	INIT_LIST_HEAD(&v4l2_dev->group_head);
>> +	mutex_init(&v4l2_dev->group_lock);
>>   	get_device(dev);
>>   	v4l2_dev->dev = dev;
>>   	if (dev = NULL) {
>> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
>> new file mode 100644
>> index 0000000..8f7bc06
>> --- /dev/null
>> +++ b/include/media/v4l2-async.h
>> @@ -0,0 +1,88 @@
>> +/*
>> + * V4L2 asynchronous subdevice registration API
>> + *
>> + * Copyright (C) 2012, Guennadi Liakhovetski<g.liakhovetski@gmx.de>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef V4L2_ASYNC_H
>> +#define V4L2_ASYNC_H
>> +
>> +#include<linux/list.h>
>> +#include<linux/mutex.h>
>> +#include<linux/notifier.h>
>> +
>> +#include<media/v4l2-subdev.h>
>> +
>> +struct device;
>> +struct v4l2_device;
>> +
>> +enum v4l2_async_bus_type {
>> +	V4L2_ASYNC_BUS_PLATFORM,
>> +	V4L2_ASYNC_BUS_I2C,
>> +};
>> +
>> +struct v4l2_async_hw_device {
>> +	enum v4l2_async_bus_type bus_type;
>> +	union {
>> +		struct {
>> +			const char *name;
>> +		} platform;
>> +		struct {
>> +			int adapter_id;
>> +			unsigned short address;
>> +		} i2c;
>> +	} match;
>> +};
>> +
>> +/**
>> + * struct v4l2_async_subdev - device descriptor
>> + * @hw:		this device descriptor
>> + * @list:	member in the group
>> + * @dev:	corresponding hardware device (I2C, platform,...)
>> + * @sdpd:	embedded subdevice platform data
>> + * @role:	this subdevice role in the video pipeline
>> + */
>> +struct v4l2_async_subdev {
>> +	struct v4l2_async_hw_device hw;
>> +	struct list_head list;
>> +	struct device *dev;
>> +	struct v4l2_subdev_platform_data sdpd;
>> +	enum v4l2_subdev_role role;
>> +};
>> +
>> +/**
>> + * struct v4l2_async_group - list of device descriptors
>> + * @list:		member in the v4l2 group list
>> + * @group:		head of device list
>> + * @done:		head of probed device list
>> + * @platform_notifier:	platform bus notifier block
>> + * @i2c_notifier:	I2C bus notifier block
>> + * @v4l2_dev:		link to the respective struct v4l2_device
>> + * @bind:		callback, called upon BUS_NOTIFY_BIND_DRIVER for each
>> + *			subdevice
>> + * @complete:		callback, called once after all subdevices in the group
>> + *			have been bound
>> + */
>> +struct v4l2_async_group {
>> +	struct list_head list;
>> +	struct list_head group;
>> +	struct list_head done;
>> +	struct notifier_block platform_notifier;
>> +	struct notifier_block i2c_notifier;
>> +	struct v4l2_device *v4l2_dev;
>> +	int (*bind)(struct v4l2_async_group *group,
>> +		    struct v4l2_async_subdev *asd);
>> +	int (*complete)(struct v4l2_async_group *group);
>> +};
>> +
>> +int v4l2_async_group_init(struct v4l2_device *v4l2_dev,
>> +			  struct v4l2_async_group *group,
>> +			  struct v4l2_async_subdev *asd, int cnt);
>> +int v4l2_async_group_probe(struct v4l2_async_group *group);
>> +void v4l2_async_group_release(struct v4l2_async_group *group);
>> +
>> +#endif
>> diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
>> index d61febf..84b18c9 100644
>> --- a/include/media/v4l2-device.h
>> +++ b/include/media/v4l2-device.h
>> @@ -21,6 +21,9 @@
>>   #ifndef _V4L2_DEVICE_H
>>   #define _V4L2_DEVICE_H
>>
>> +#include<linux/list.h>
>> +#include<linux/mutex.h>
>> +
>>   #include<media/media-device.h>
>>   #include<media/v4l2-subdev.h>
>>   #include<media/v4l2-dev.h>
>> @@ -60,6 +63,9 @@ struct v4l2_device {
>>   	struct v4l2_prio_state prio;
>>   	/* BKL replacement mutex. Temporary solution only. */
>>   	struct mutex ioctl_lock;
>> +	/* Subdevice group handling */
>> +	struct mutex group_lock;
>> +	struct list_head group_head;
>>   	/* Keep track of the references to this struct. */
>>   	struct kref ref;
>>   	/* Release function that is called when the ref count goes to 0. */
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 2ecd737..1756c6c 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -574,6 +574,22 @@ struct v4l2_subdev_fh {
>>   #endif
>>   };
>>
>> +enum v4l2_subdev_role {
>> +	V4L2_SUBDEV_DATA_SOURCE = 1,
>> +	V4L2_SUBDEV_DATA_SINK,
>> +	V4L2_SUBDEV_DATA_PROCESSOR,
>> +};
>> +
>> +/**
>> + * struct v4l2_subdev_platform_data - subdev platform data
>> + * @subdev:		subdevice
>> + * @platform_data:	subdevice driver platform data
>> + */
>> +struct v4l2_subdev_platform_data {
>> +	struct v4l2_subdev *subdev;
>> +	void *platform_data;
>> +};
>> +
>>   #define to_v4l2_subdev_fh(fh)	\
>>   	container_of(fh, struct v4l2_subdev_fh, vfh)
>>
>> -- 
>> 1.7.2.5

Thanks,
Sylwester