| Message ID | 20230525091615.2324824-8-sakari.ailus@linux.intel.com |
|---|---|
| State | Superseded |
| 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:50PM +0300, Sakari Ailus wrote: > The list entry is initialised as a head in v4l2_async_register_subdev() > just before being added to the list. This isn't needed, drop the > initialisation. Is this really unneeded ? Before the initialization and the list_add() call there are a few code paths that can access the async_list. For instance, the error path calls v4l2_async_cleanup(), which calls list_del_init(&sd->async_list); That won't work well on an uninitialized (or zero-initialized) list_head. > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 320fe5cbaaf41..aef9a16e892ef 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -823,8 +823,6 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > > mutex_lock(&list_lock); > > - INIT_LIST_HEAD(&sd->async_list); > - > list_for_each_entry(notifier, ¬ifier_list, list) { > struct v4l2_device *v4l2_dev = > v4l2_async_nf_find_v4l2_dev(notifier);
Hi Laurent, On Tue, May 30, 2023 at 05:46:50AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, May 25, 2023 at 12:15:50PM +0300, Sakari Ailus wrote: > > The list entry is initialised as a head in v4l2_async_register_subdev() > > just before being added to the list. This isn't needed, drop the > > initialisation. > > Is this really unneeded ? Before the initialization and the list_add() > call there are a few code paths that can access the async_list. For > instance, the error path calls v4l2_async_cleanup(), which calls > > list_del_init(&sd->async_list); > > That won't work well on an uninitialized (or zero-initialized) > list_head. I think you're right, I'll drop this patch. This initialisation will be removed in a later patch though, as the list will be redundant soon.
On Tue, Jun 13, 2023 at 02:00:43PM +0000, Sakari Ailus wrote: > Hi Laurent, > > On Tue, May 30, 2023 at 05:46:50AM +0300, Laurent Pinchart wrote: > > Hi Sakari, > > > > Thank you for the patch. > > > > On Thu, May 25, 2023 at 12:15:50PM +0300, Sakari Ailus wrote: > > > The list entry is initialised as a head in v4l2_async_register_subdev() > > > just before being added to the list. This isn't needed, drop the > > > initialisation. > > > > Is this really unneeded ? Before the initialization and the list_add() > > call there are a few code paths that can access the async_list. For > > instance, the error path calls v4l2_async_cleanup(), which calls > > > > list_del_init(&sd->async_list); > > > > That won't work well on an uninitialized (or zero-initialized) > > list_head. > > I think you're right, I'll drop this patch. This initialisation will be > removed in a later patch though, as the list will be redundant soon. Actually the list and the field remains, although it becomes unnecessary to initialise it. I'll see if this patch would be meaningful later on in the series or squashed to another patch.
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index 320fe5cbaaf41..aef9a16e892ef 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -823,8 +823,6 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) mutex_lock(&list_lock); - INIT_LIST_HEAD(&sd->async_list); - list_for_each_entry(notifier, ¬ifier_list, list) { struct v4l2_device *v4l2_dev = v4l2_async_nf_find_v4l2_dev(notifier);
The list entry is initialised as a head in v4l2_async_register_subdev() just before being added to the list. This isn't needed, drop the initialisation. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-async.c | 2 -- 1 file changed, 2 deletions(-)