Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Steve Longerbeam <slongerbeam@gmail.com>]

Hi Sakari,


On 04/20/2018 05:24 AM, Sakari Ailus wrote:
> Hi Steve,
>
> Thanks for the patchset.
>
> On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:
>> v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
>> that the asd's match_type is valid and that no other equivalent asd's
>> have already been added to this notifier's asd list, or to other
>> registered notifier's waiting or done lists, and increments num_subdevs.
>>
>> v4l2_async_notifier_add_subdev() does not make use of the notifier subdevs
>> array, otherwise it would have to re-allocate the array every time the
>> function was called. In place of the subdevs array, the function adds
>> the asd to a new master asd_list. The function will return error with a
>> WARN() if it is ever called with the subdevs array allocated.
>>
>> In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
>> and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
>> array or a non-empty notifier->asd_list.
> I do agree with the approach, but this patch leaves the remaining users of
> the subdevs array in the notifier intact. Could you rework them to use the
> v4l2_async_notifier_add_subdev() as well?
>
> There seem to be just a few of them --- exynos4-is and rcar_drif.

I count more than a few :)

% cd drivers/media && grep -l -r --include "*.[ch]" 
'notifier[\.\-]>*subdevs[   ]*='
v4l2-core/v4l2-async.c
platform/pxa_camera.c
platform/ti-vpe/cal.c
platform/exynos4-is/media-dev.c
platform/qcom/camss-8x16/camss.c
platform/soc_camera/soc_camera.c
platform/atmel/atmel-isi.c
platform/atmel/atmel-isc.c
platform/stm32/stm32-dcmi.c
platform/davinci/vpif_capture.c
platform/davinci/vpif_display.c
platform/renesas-ceu.c
platform/am437x/am437x-vpfe.c
platform/xilinx/xilinx-vipp.c
platform/rcar_drif.c


So not including v4l2-core, the list is:

pxa_camera
ti-vpe
exynos4-is
qcom
soc_camera
atmel
stm32
davinci
renesas-ceu
am437x
xilinx
rcar_drif


Given such a large list of the users of the notifier->subdevs array,
I think this should be done is two steps: submit this patchset first,
that keeps the backward compatibility, and then a subsequent patchset
that converts all drivers to use v4l2_async_notifier_add_subdev(), at
which point the subdevs array can be removed from struct 
v4l2_async_notifier.

Or, do you still think this should be done all at once? I could add a
large patch to this patchset that does the conversion and removes
the subdevs array.

Steve


>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>> Changes since v2:
>> - add a NULL asd pointer check to v4l2_async_notifier_asd_valid().
>> Changes since v1:
>> - none
>> ---
>>   drivers/media/v4l2-core/v4l2-async.c | 206 +++++++++++++++++++++++++++--------
>>   include/media/v4l2-async.h           |  22 ++++
>>   2 files changed, 184 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>> index b59bbac..7b7f7e2 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -366,16 +366,26 @@ static bool v4l2_async_notifier_has_async_subdev(
>>   	struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd,
>>   	unsigned int this_index)
>>   {
>> +	struct v4l2_async_subdev *asd_y;
>>   	unsigned int j;
>>   
>>   	lockdep_assert_held(&list_lock);
>>   
>>   	/* Check that an asd is not being added more than once. */
>> -	for (j = 0; j < this_index; j++) {
>> -		struct v4l2_async_subdev *asd_y = notifier->subdevs[j];
>> -
>> -		if (asd_equal(asd, asd_y))
>> -			return true;
>> +	if (notifier->subdevs) {
>> +		for (j = 0; j < this_index; j++) {
>> +			asd_y = notifier->subdevs[j];
>> +			if (asd_equal(asd, asd_y))
>> +				return true;
>> +		}
>> +	} else {
>> +		j = 0;
>> +		list_for_each_entry(asd_y, &notifier->asd_list, asd_list) {
>> +			if (j++ >= this_index)
>> +				break;
>> +			if (asd_equal(asd, asd_y))
>> +				return true;
>> +		}
>>   	}
>>   
>>   	/* Check that an asd does not exist in other notifiers. */
>> @@ -386,10 +396,46 @@ static bool v4l2_async_notifier_has_async_subdev(
>>   	return false;
>>   }
>>   
>> -static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>> +static int v4l2_async_notifier_asd_valid(struct v4l2_async_notifier *notifier,
>> +					 struct v4l2_async_subdev *asd,
>> +					 unsigned int this_index)
>>   {
>>   	struct device *dev =
>>   		notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
>> +
>> +	if (!asd)
>> +		return -EINVAL;
>> +
>> +	switch (asd->match_type) {
>> +	case V4L2_ASYNC_MATCH_CUSTOM:
>> +	case V4L2_ASYNC_MATCH_DEVNAME:
>> +	case V4L2_ASYNC_MATCH_I2C:
>> +	case V4L2_ASYNC_MATCH_FWNODE:
>> +		if (v4l2_async_notifier_has_async_subdev(notifier, asd,
>> +							 this_index))
>> +			return -EEXIST;
>> +		break;
>> +	default:
>> +		dev_err(dev, "Invalid match type %u on %p\n",
>> +			asd->match_type, asd);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void __v4l2_async_notifier_init(struct v4l2_async_notifier *notifier)
>> +{
>> +	lockdep_assert_held(&list_lock);
>> +
>> +	INIT_LIST_HEAD(&notifier->asd_list);
>> +	INIT_LIST_HEAD(&notifier->waiting);
>> +	INIT_LIST_HEAD(&notifier->done);
>> +	notifier->lists_initialized = true;
>> +}
>> +
>> +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>> +{
>>   	struct v4l2_async_subdev *asd;
>>   	int ret;
>>   	int i;
>> @@ -397,34 +443,40 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>>   	if (notifier->num_subdevs > V4L2_MAX_SUBDEVS)
>>   		return -EINVAL;
>>   
>> -	INIT_LIST_HEAD(&notifier->waiting);
>> -	INIT_LIST_HEAD(&notifier->done);
>> -
>>   	mutex_lock(&list_lock);
>>   
>> -	for (i = 0; i < notifier->num_subdevs; i++) {
>> -		asd = notifier->subdevs[i];
>> +	if (!notifier->lists_initialized)
>> +		__v4l2_async_notifier_init(notifier);
>>   
>> -		switch (asd->match_type) {
>> -		case V4L2_ASYNC_MATCH_CUSTOM:
>> -		case V4L2_ASYNC_MATCH_DEVNAME:
>> -		case V4L2_ASYNC_MATCH_I2C:
>> -		case V4L2_ASYNC_MATCH_FWNODE:
>> -			if (v4l2_async_notifier_has_async_subdev(
>> -				    notifier, asd, i)) {
>> -				dev_err(dev,
>> -					"asd has already been registered or in notifier's subdev list\n");
>> -				ret = -EEXIST;
>> -				goto err_unlock;
>> -			}
>> -			break;
>> -		default:
>> -			dev_err(dev, "Invalid match type %u on %p\n",
>> -				asd->match_type, asd);
>> +	if (!list_empty(&notifier->asd_list)) {
>> +		/*
>> +		 * Caller must have either used v4l2_async_notifier_add_subdev
>> +		 * to add asd's to notifier->asd_list, or provided the
>> +		 * notifier->subdevs array, but not both.
>> +		 */
>> +		if (WARN_ON(notifier->subdevs)) {
>>   			ret = -EINVAL;
>>   			goto err_unlock;
>>   		}
>> -		list_add_tail(&asd->list, &notifier->waiting);
>> +
>> +		i = 0;
>> +		list_for_each_entry(asd, &notifier->asd_list, asd_list) {
>> +			ret = v4l2_async_notifier_asd_valid(notifier, asd, i++);
>> +			if (ret)
>> +				goto err_unlock;
>> +
>> +			list_add_tail(&asd->list, &notifier->waiting);
>> +		}
>> +	} else if (notifier->subdevs) {
>> +		for (i = 0; i < notifier->num_subdevs; i++) {
>> +			asd = notifier->subdevs[i];
>> +
>> +			ret = v4l2_async_notifier_asd_valid(notifier, asd, i);
>> +			if (ret)
>> +				goto err_unlock;
>> +
>> +			list_add_tail(&asd->list, &notifier->waiting);
>> +		}
>>   	}
>>   
>>   	ret = v4l2_async_notifier_try_all_subdevs(notifier);
>> @@ -514,36 +566,102 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>>   }
>>   EXPORT_SYMBOL(v4l2_async_notifier_unregister);
>>   
>> -void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
>> +static void __v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
>>   {
>> +	struct v4l2_async_subdev *asd, *tmp;
>>   	unsigned int i;
>>   
>> -	if (!notifier || !notifier->max_subdevs)
>> +	if (!notifier)
>>   		return;
>>   
>> -	for (i = 0; i < notifier->num_subdevs; i++) {
>> -		struct v4l2_async_subdev *asd = notifier->subdevs[i];
>> +	if (notifier->subdevs) {
>> +		if (!notifier->max_subdevs)
>> +			return;
>>   
>> -		switch (asd->match_type) {
>> -		case V4L2_ASYNC_MATCH_FWNODE:
>> -			fwnode_handle_put(asd->match.fwnode);
>> -			break;
>> -		default:
>> -			WARN_ON_ONCE(true);
>> -			break;
>> +		for (i = 0; i < notifier->num_subdevs; i++) {
>> +			asd = notifier->subdevs[i];
>> +
>> +			switch (asd->match_type) {
>> +			case V4L2_ASYNC_MATCH_FWNODE:
>> +				fwnode_handle_put(asd->match.fwnode);
>> +				break;
>> +			default:
>> +				break;
>> +			}
>> +
>> +			kfree(asd);
>>   		}
>>   
>> -		kfree(asd);
>> +		notifier->max_subdevs = 0;
>> +		kvfree(notifier->subdevs);
>> +		notifier->subdevs = NULL;
>> +	} else if (notifier->lists_initialized) {
>> +		list_for_each_entry_safe(asd, tmp,
>> +					 &notifier->asd_list, asd_list) {
>> +			switch (asd->match_type) {
>> +			case V4L2_ASYNC_MATCH_FWNODE:
>> +				fwnode_handle_put(asd->match.fwnode);
>> +				break;
>> +			default:
>> +				break;
>> +			}
>> +
>> +			list_del(&asd->asd_list);
>> +			kfree(asd);
>> +		}
>>   	}
>>   
>> -	notifier->max_subdevs = 0;
>>   	notifier->num_subdevs = 0;
>> +}
>>   
>> -	kvfree(notifier->subdevs);
>> -	notifier->subdevs = NULL;
>> +void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
>> +{
>> +	mutex_lock(&list_lock);
>> +
>> +	__v4l2_async_notifier_cleanup(notifier);
>> +
>> +	mutex_unlock(&list_lock);
>>   }
>>   EXPORT_SYMBOL_GPL(v4l2_async_notifier_cleanup);
>>   
>> +int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
>> +				   struct v4l2_async_subdev *asd)
>> +{
>> +	int ret = 0;
>> +
>> +	mutex_lock(&list_lock);
>> +
>> +	if (notifier->num_subdevs >= V4L2_MAX_SUBDEVS) {
>> +		ret = -EINVAL;
>> +		goto unlock;
>> +	}
>> +
>> +	if (!notifier->lists_initialized)
>> +		__v4l2_async_notifier_init(notifier);
>> +
>> +	/*
>> +	 * If caller uses this function, it cannot also allocate and
>> +	 * place asd's in the notifier->subdevs array.
>> +	 */
>> +	if (WARN_ON(notifier->subdevs)) {
>> +		ret = -EINVAL;
>> +		goto unlock;
>> +	}
>> +
>> +	ret = v4l2_async_notifier_asd_valid(notifier, asd,
>> +					    notifier->num_subdevs);
>> +	if (ret)
>> +		goto unlock;
>> +
>> +	list_add_tail(&asd->asd_list, &notifier->asd_list);
>> +	notifier->num_subdevs++;
>> +
>> +unlock:
>> +	mutex_unlock(&list_lock);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_add_subdev);
>> +
>>   int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>>   {
>>   	struct v4l2_async_notifier *subdev_notifier;
>> @@ -617,7 +735,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
>>   	mutex_lock(&list_lock);
>>   
>>   	__v4l2_async_notifier_unregister(sd->subdev_notifier);
>> -	v4l2_async_notifier_cleanup(sd->subdev_notifier);
>> +	__v4l2_async_notifier_cleanup(sd->subdev_notifier);
>>   	kfree(sd->subdev_notifier);
>>   	sd->subdev_notifier = NULL;
>>   
>> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
>> index 1592d32..fa05905 100644
>> --- a/include/media/v4l2-async.h
>> +++ b/include/media/v4l2-async.h
>> @@ -73,6 +73,8 @@ enum v4l2_async_match_type {
>>    * @match.custom.priv:
>>    *		Driver-specific private struct with match parameters
>>    *		to be used if %V4L2_ASYNC_MATCH_CUSTOM.
>> + * @asd_list:	used to add struct v4l2_async_subdev objects to the
>> + *		master notifier->asd_list
>>    * @list:	used to link struct v4l2_async_subdev objects, waiting to be
>>    *		probed, to a notifier->waiting list
>>    *
>> @@ -98,6 +100,7 @@ struct v4l2_async_subdev {
>>   
>>   	/* v4l2-async core private: not to be used by drivers */
>>   	struct list_head list;
>> +	struct list_head asd_list;
>>   };
>>   
>>   /**
>> @@ -127,9 +130,11 @@ struct v4l2_async_notifier_operations {
>>    * @v4l2_dev:	v4l2_device of the root notifier, NULL otherwise
>>    * @sd:		sub-device that registered the notifier, NULL otherwise
>>    * @parent:	parent notifier
>> + * @asd_list:	master list of struct v4l2_async_subdev, replaces @subdevs
>>    * @waiting:	list of struct v4l2_async_subdev, waiting for their drivers
>>    * @done:	list of struct v4l2_subdev, already probed
>>    * @list:	member in a global list of notifiers
>> + * @lists_initialized: list_head's have been initialized
>>    */
>>   struct v4l2_async_notifier {
>>   	const struct v4l2_async_notifier_operations *ops;
>> @@ -139,12 +144,29 @@ struct v4l2_async_notifier {
>>   	struct v4l2_device *v4l2_dev;
>>   	struct v4l2_subdev *sd;
>>   	struct v4l2_async_notifier *parent;
>> +	struct list_head asd_list;
>>   	struct list_head waiting;
>>   	struct list_head done;
>>   	struct list_head list;
>> +	bool lists_initialized;
>>   };
>>   
>>   /**
>> + * v4l2_async_notifier_add_subdev - Add an async subdev to the
>> + *				notifier's master asd_list.
>> + *
>> + * @notifier: pointer to &struct v4l2_async_notifier
>> + * @asd: pointer to &struct v4l2_async_subdev
>> + *
>> + * This can be used before registering a notifier to add an
>> + * asd to the notifiers master asd_list. If the caller uses
>> + * this method to compose an asd list, it must never allocate
>> + * or place asd's in the @subdevs array.
>> + */
>> +int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
>> +				   struct v4l2_async_subdev *asd);
>> +
>> +/**
>>    * v4l2_async_notifier_register - registers a subdevice asynchronous notifier
>>    *
>>    * @v4l2_dev: pointer to &struct v4l2_device
>> -- 
>> 2.7.4
>>