| Message ID | 20230525091615.2324824-7-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Separate links and async sub-devices | expand |
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@linux.intel.com> > --- > 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; >
Hi Laurent, On Tue, May 30, 2023 at 05:42:29AM +0300, Laurent Pinchart wrote: > 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@linux.intel.com> > > --- > > 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 :-) I can add some, the argument will disappear later in the series though. This patch mirrors the old functionality, nothing more. Changing the break to continue is functionally equivalent here as this is the end of the list, I can do that, too.
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. */ 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;
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"). Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-async.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)