Hi Sakari, Thank you for the patch. On Thu, May 25, 2023 at 12:15:49PM +0300, Sakari Ailus wrote: > There's a need to verify that a single async sub-device isn't being added > multiple times, this would be an error. This takes place at the time of > adding the async sub-device to the notifier's list as well as when the > notifier is added to the global notifier's list. > > Use the pointer to the sub-device for testing this instead of an index to > an array that is long gone. (There was an array of async sub-devices in > the notifier before it was converted to a linked list by commit > 66beb323e4a0 ("media: v4l2: async: Remove notifier subdevs array"). Unbalanced opening and closing parentheses. > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > --- > drivers/media/v4l2-core/v4l2-async.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index c5781124337af..320fe5cbaaf41 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -520,21 +520,19 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > /* > * Find out whether an async sub-device was set up already or > - * whether it exists in a given notifier before @this_index. > - * If @this_index < 0, search the notifier's entire @asd_list. > + * whether it exists in a given notifier. Please document what the skip_self parameter does. The parameter name doesn't match the 'break' in the test below, I was expecting a 'continue'. If my expectation is wrong documentation should help, if it's correct, then you can fix the code :-) > */ > static bool > v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > - struct v4l2_async_subdev *asd, int this_index) > + struct v4l2_async_subdev *asd, bool skip_self) > { > struct v4l2_async_subdev *asd_y; > - int j = 0; > > lockdep_assert_held(&list_lock); > > /* Check that an asd is not being added more than once. */ > list_for_each_entry(asd_y, ¬ifier->asd_list, asd_list) { > - if (this_index >= 0 && j++ >= this_index) > + if (skip_self && asd == asd_y) > break; > if (asd_equal(asd, asd_y)) > return true; > @@ -550,7 +548,7 @@ v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier, > > static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > struct v4l2_async_subdev *asd, > - int this_index) > + bool skip_self) > { > struct device *dev = notifier_dev(notifier); > > @@ -560,7 +558,7 @@ static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier, > switch (asd->match_type) { > case V4L2_ASYNC_MATCH_I2C: > case V4L2_ASYNC_MATCH_FWNODE: > - if (v4l2_async_nf_has_async_subdev(notifier, asd, this_index)) { > + if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) { > dev_dbg(dev, "v4l2-async: subdev descriptor already listed in a notifier\n"); > return -EEXIST; > } > @@ -583,7 +581,7 @@ EXPORT_SYMBOL(v4l2_async_nf_init); > static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > { > struct v4l2_async_subdev *asd; > - int ret, i = 0; > + int ret; > > INIT_LIST_HEAD(¬ifier->waiting); > INIT_LIST_HEAD(¬ifier->done); > @@ -591,7 +589,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier) > mutex_lock(&list_lock); > > list_for_each_entry(asd, ¬ifier->asd_list, asd_list) { > - ret = v4l2_async_nf_asd_valid(notifier, asd, i++); > + ret = v4l2_async_nf_asd_valid(notifier, asd, true); > if (ret) > goto err_unlock; > > @@ -725,7 +723,7 @@ static int __v4l2_async_nf_add_subdev(struct v4l2_async_notifier *notifier, > > mutex_lock(&list_lock); > > - ret = v4l2_async_nf_asd_valid(notifier, asd, -1); > + ret = v4l2_async_nf_asd_valid(notifier, asd, false); > if (ret) > goto unlock; > -- Regards, Laurent Pinchart