linux-kernel - Re: [PATCH 1/4] media: v4l2-subdev: Add cleanup macros for active state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240924171736.GE30551@pendragon.ideasonboard.com>
Date: Tue, 24 Sep 2024 20:17:36 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Niklas Söderlund <niklas.soderlund@...natech.se>,
	Hans Verkuil <hverkuil-cisco@...all.nl>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-renesas-soc@...r.kernel.org,
	Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>
Subject: Re: [PATCH 1/4] media: v4l2-subdev: Add cleanup macros for active
 state

Hi Tomi,

Thank you for the patch.

On Tue, Sep 17, 2024 at 05:09:29PM +0300, Tomi Valkeinen wrote:
> From: Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>
> 
> Add cleanup macros for active state. These can be used to call
> v4l2_subdev_lock_and_get_active_state() and manage the unlocking either
> in unscoped or scoped fashion:
> 
> This locks the state, gets it to the 'state' variable, and unlocks when
> exiting the surrounding scope:
> 
> CLASS(v4l2_subdev_lock_and_get_active_state, state)(subdev);
> 
> This does the same, but inside the given scope:
> 
> scoped_v4l2_subdev_lock_and_get_active_state(subdev) {
> }
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>
> ---
>  include/media/v4l2-subdev.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index bd235d325ff9..699007cfffd7 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -8,6 +8,7 @@
>  #ifndef _V4L2_SUBDEV_H
>  #define _V4L2_SUBDEV_H
>  
> +#include <linux/cleanup.h>
>  #include <linux/types.h>
>  #include <linux/v4l2-subdev.h>
>  #include <media/media-entity.h>
> @@ -1854,6 +1855,15 @@ v4l2_subdev_lock_and_get_active_state(struct v4l2_subdev *sd)
>  	return sd->active_state;
>  }
>  
> +DEFINE_CLASS(v4l2_subdev_lock_and_get_active_state, struct v4l2_subdev_state *,
> +	     v4l2_subdev_unlock_state(_T),
> +	     v4l2_subdev_lock_and_get_active_state(sd), struct v4l2_subdev *sd);
> +
> +#define scoped_v4l2_subdev_lock_and_get_active_state(sd)              \
> +	for (CLASS(v4l2_subdev_lock_and_get_active_state, state)(sd), \
> +	     *done = NULL;                                            \
> +	     !done; done = (void *)1)

That a very long name :-S Could this be done using the scoped_guard()
macro instead ? For instance, with spinlocks you can do

	scoped_guard(spinlock_irqsave, &dev->lock) {
		...
	}

It would be nice to be able to write

	scoped_guard(v4l2_subdev_state, sd) {
		...
	}

This being said, we would then end up with the state variable being
named scope, which wouldn't be great.

This is actually one of my issues with the above macros, and especially
scoped_v4l2_subdev_lock_and_get_active_state(). It creates a local state
variable in the scope, which makes the code less readable in my opinion.

We could keep the class and drop
scoped_v4l2_subdev_lock_and_get_active_state(). I think I would like to
shorten the class name then.

Another option is to use DEFINE_FREE() and __free() instead.

> +
>  /**
>   * v4l2_subdev_init - initializes the sub-device struct
>   *
> 

-- 
Regards,

Laurent Pinchart