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]
Date: Wed, 24 Apr 2024 16:08:58 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Hans Verkuil <hverkuil@...all.nl>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>,
 Umang Jain <umang.jain@...asonboard.com>
Cc: linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 08/10] media: subdev: Refactor
 v4l2_subdev_enable/disable_streams()

On 24/04/2024 11:41, Hans Verkuil wrote:
> On 16/04/2024 15:55, Tomi Valkeinen wrote:
>> Add two internal helper functions, v4l2_subdev_collect_streams() and
>> v4l2_subdev_set_streams_enabled(), which allows us to refactor
>> v4l2_subdev_enable/disable_streams() functions.
>>
>> This (I think) makes the code a bit easier to read, and lets us more
>> easily add new functionality in the helper functions in the following
>> patch.
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-subdev.c | 109 +++++++++++++++++++---------------
>>   1 file changed, 60 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 38388b223564..e45fd42da1e3 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -2100,6 +2100,42 @@ int v4l2_subdev_routing_validate(struct v4l2_subdev *sd,
>>   }
>>   EXPORT_SYMBOL_GPL(v4l2_subdev_routing_validate);
>>   
>> +static void v4l2_subdev_collect_streams(struct v4l2_subdev *sd,
>> +					struct v4l2_subdev_state *state,
>> +					u32 pad, u64 streams_mask,
>> +					u64 *found_streams,
>> +					u64 *enabled_streams)
>> +{
>> +	*found_streams = 0;
>> +	*enabled_streams = 0;
>> +
>> +	for (unsigned int i = 0; i < state->stream_configs.num_configs; ++i) {
>> +		const struct v4l2_subdev_stream_config *cfg =
>> +			&state->stream_configs.configs[i];
>> +
>> +		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
>> +			continue;
>> +
>> +		*found_streams |= BIT_ULL(cfg->stream);
>> +		if (cfg->enabled)
>> +			*enabled_streams |= BIT_ULL(cfg->stream);
>> +	}
>> +}
>> +
>> +static void v4l2_subdev_set_streams_enabled(struct v4l2_subdev *sd,
>> +					    struct v4l2_subdev_state *state,
>> +					    u32 pad, u64 streams_mask,
>> +					    bool enabled)
>> +{
>> +	for (unsigned int i = 0; i < state->stream_configs.num_configs; ++i) {
>> +		struct v4l2_subdev_stream_config *cfg =
>> +			&state->stream_configs.configs[i];
>> +
>> +		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
>> +			cfg->enabled = enabled;
>> +	}
>> +}
>> +
>>   static int v4l2_subdev_enable_streams_fallback(struct v4l2_subdev *sd, u32 pad,
>>   					       u64 streams_mask)
>>   {
>> @@ -2151,8 +2187,8 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>>   	struct device *dev = sd->entity.graph_obj.mdev->dev;
>>   	struct v4l2_subdev_state *state;
>>   	bool already_streaming;
>> -	u64 found_streams = 0;
>> -	unsigned int i;
>> +	u64 enabled_streams;
>> +	u64 found_streams;
>>   	int ret;
>>   
>>   	/* A few basic sanity checks first. */
>> @@ -2173,22 +2209,9 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>>   	 * Verify that the requested streams exist and that they are not
>>   	 * already enabled.
>>   	 */
>> -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
>> -		struct v4l2_subdev_stream_config *cfg =
>> -			&state->stream_configs.configs[i];
>>   
>> -		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
>> -			continue;
>> -
>> -		found_streams |= BIT_ULL(cfg->stream);
>> -
>> -		if (cfg->enabled) {
>> -			dev_dbg(dev, "stream %u already enabled on %s:%u\n",
>> -				cfg->stream, sd->entity.name, pad);
>> -			ret = -EALREADY;
>> -			goto done;
>> -		}
>> -	}
>> +	v4l2_subdev_collect_streams(sd, state, pad, streams_mask,
>> +				    &found_streams, &enabled_streams);
>>   
>>   	if (found_streams != streams_mask) {
>>   		dev_dbg(dev, "streams 0x%llx not found on %s:%u\n",
>> @@ -2197,6 +2220,13 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>>   		goto done;
>>   	}
>>   
>> +	if (enabled_streams) {
>> +		dev_dbg(dev, "streams 0x%llx already enabled on %s:%u\n",
>> +			enabled_streams, sd->entity.name, pad);
>> +		ret = -EINVAL;
>> +		goto done;
>> +	}
>> +
>>   	dev_dbg(dev, "enable streams %u:%#llx\n", pad, streams_mask);
>>   
>>   	already_streaming = v4l2_subdev_is_streaming(sd);
>> @@ -2211,13 +2241,7 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>>   	}
>>   
>>   	/* Mark the streams as enabled. */
>> -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
>> -		struct v4l2_subdev_stream_config *cfg =
>> -			&state->stream_configs.configs[i];
>> -
>> -		if (cfg->pad == pad && (streams_mask & BIT_ULL(cfg->stream)))
>> -			cfg->enabled = true;
>> -	}
>> +	v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, true);
>>   
>>   	if (!already_streaming)
>>   		v4l2_subdev_enable_privacy_led(sd);
>> @@ -2279,8 +2303,8 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>>   {
>>   	struct device *dev = sd->entity.graph_obj.mdev->dev;
>>   	struct v4l2_subdev_state *state;
>> -	u64 found_streams = 0;
>> -	unsigned int i;
>> +	u64 enabled_streams;
>> +	u64 found_streams;
>>   	int ret;
>>   
>>   	/* A few basic sanity checks first. */
>> @@ -2301,22 +2325,9 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>>   	 * Verify that the requested streams exist and that they are not
>>   	 * already disabled.
>>   	 */
>> -	for (i = 0; i < state->stream_configs.num_configs; ++i) {
>> -		struct v4l2_subdev_stream_config *cfg =
>> -			&state->stream_configs.configs[i];
>> -
>> -		if (cfg->pad != pad || !(streams_mask & BIT_ULL(cfg->stream)))
>> -			continue;
>>   
>> -		found_streams |= BIT_ULL(cfg->stream);
>> -
>> -		if (!cfg->enabled) {
>> -			dev_dbg(dev, "stream %u already disabled on %s:%u\n",
>> -				cfg->stream, sd->entity.name, pad);
>> -			ret = -EALREADY;
> 
> This used to return -EALREADY...
> 
>> -			goto done;
>> -		}
>> -	}
>> +	v4l2_subdev_collect_streams(sd, state, pad, streams_mask,
>> +				    &found_streams, &enabled_streams);
>>   
>>   	if (found_streams != streams_mask) {
>>   		dev_dbg(dev, "streams 0x%llx not found on %s:%u\n",
>> @@ -2325,6 +2336,13 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>>   		goto done;
>>   	}
>>   
>> +	if (enabled_streams != streams_mask) {
>> +		dev_dbg(dev, "streams 0x%llx already disabled on %s:%u\n",
>> +			streams_mask & ~enabled_streams, sd->entity.name, pad);
>> +		ret = -EINVAL;
> 
> ...but now it returns -EINVAL.
> 
> Is that intentional?

No. I think it was a copy-paste thing.

> I prefer EINVAL to be honest, but I was just wondering about this change.
> 
> It looks like the next patch removes the last of the EALREADY error returns
> as well.

Good catch. We document that the enable_streams() returns EALREADY if a 
stream is already enabled, so lets stick to that. I'll update the patches.

  Tomi


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ