| Message ID | 20230330115853.1628216-6-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Separate links and async sub-devices | expand |
Hi Sakari On Thu, Mar 30, 2023 at 02:58:40PM +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. > > 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 bb78e3618ab5..fc9ae22e2b47 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -456,21 +456,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) is skip_self used ? > { > 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 (asd == asd_y) > break; > if (asd_equal(asd, asd_y)) > return true; > @@ -486,7 +484,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->v4l2_dev ? notifier->v4l2_dev->dev : NULL; > @@ -497,7 +495,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, "subdev descriptor already listed in this or other notifiers\n"); > return -EEXIST; > } > @@ -520,7 +518,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); > @@ -528,7 +526,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; > > @@ -661,7 +659,7 @@ 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; > > -- > 2.30.2 >
Hi Jacopo, On Thu, Apr 13, 2023 at 06:58:56PM +0200, Jacopo Mondi wrote: > Hi Sakari > > On Thu, Mar 30, 2023 at 02:58:40PM +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. > > > > 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 bb78e3618ab5..fc9ae22e2b47 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -456,21 +456,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) > > is skip_self used ? Yes, it should have been there. I'll add it for v2.
Hi Sakari, Thank you for the patch. On Thu, Mar 30, 2023 at 02:58:40PM +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. Reading the patch, I have no idea what the "long gone array" is. Could you please expand the commit message to make this easier to review ? v4l2-async is very difficult to follow in general, reviewing this series is painful :-S Let's try to improve it with better commit messages. > 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 bb78e3618ab5..fc9ae22e2b47 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -456,21 +456,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 (asd == asd_y) > break; > if (asd_equal(asd, asd_y)) > return true; > @@ -486,7 +484,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->v4l2_dev ? notifier->v4l2_dev->dev : NULL; > @@ -497,7 +495,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, "subdev descriptor already listed in this or other notifiers\n"); > return -EEXIST; > } > @@ -520,7 +518,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); > @@ -528,7 +526,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; > > @@ -661,7 +659,7 @@ 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, Apr 25, 2023 at 04:15:41AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, Mar 30, 2023 at 02:58:40PM +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. > > Reading the patch, I have no idea what the "long gone array" is. Could > you please expand the commit message to make this easier to review ? Yes... the async sub-devices were placed in an array earlier, that's what the index was referring to. Although this could be an entry in a linked list. Not how they are usually referred to though. This will go away permanently later on in the set. I'll add this to the commit message. > v4l2-async is very difficult to follow in general, reviewing this series > is painful :-S Let's try to improve it with better commit messages.
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index bb78e3618ab5..fc9ae22e2b47 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -456,21 +456,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 (asd == asd_y) break; if (asd_equal(asd, asd_y)) return true; @@ -486,7 +484,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->v4l2_dev ? notifier->v4l2_dev->dev : NULL; @@ -497,7 +495,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, "subdev descriptor already listed in this or other notifiers\n"); return -EEXIST; } @@ -520,7 +518,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); @@ -528,7 +526,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; @@ -661,7 +659,7 @@ 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. 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(-)