| 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
| ||
|
Message-ID: <ec113029-bf85-44c7-9e56-d242e5eecba0@ideasonboard.com>
Date: Wed, 10 Apr 2024 13:35:41 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
Hans Verkuil <hverkuil@...all.nl>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Umang Jain <umang.jain@...asonboard.com>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 8/9] media: subdev: Refactor
v4l2_subdev_enable/disable_streams()
On 10/04/2024 13:13, Sakari Ailus wrote:
> Moi,
>
> Thank you for working on this.
>
> On Fri, Apr 05, 2024 at 12:14:26PM +0300, 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.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>> ---
>> drivers/media/v4l2-core/v4l2-subdev.c | 111 +++++++++++++++++++---------------
>> 1 file changed, 62 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 015f2fb423c9..6c3c9069f1e2 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -2099,6 +2099,44 @@ 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;
>> +
>> + cfg = &state->stream_configs.configs[i];
>
> You can do the assignment in declaration.
I can, but you want the lines to be split at 80, so that'll end up being
in two lines, which, I think, looks messier than the one above.
Usually I think doing initialization when declaring the variable is best
done if it fits into one line.
Tomi
>
> Same for streams_enabled() below.
>
>> +
>> + 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;
>> +
>> + 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)
>> {
>> @@ -2150,8 +2188,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. */
>> @@ -2172,22 +2210,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",
>> @@ -2196,6 +2221,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);
>> @@ -2213,13 +2245,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);
>>
>> done:
>> if (ret && !already_streaming)
>> @@ -2281,8 +2307,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. */
>> @@ -2303,22 +2329,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;
>> - 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",
>> @@ -2327,6 +2340,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;
>> + goto done;
>> + }
>> +
>> dev_dbg(dev, "disable streams %u:%#llx\n", pad, streams_mask);
>>
>> /* Call the .disable_streams() operation. */
>> @@ -2338,14 +2358,7 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>> goto done;
>> }
>>
>> - /* Mark the streams as 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)))
>> - cfg->enabled = false;
>> - }
>> + v4l2_subdev_set_streams_enabled(sd, state, pad, streams_mask, false);
>>
>> done:
>> if (!v4l2_subdev_is_streaming(sd))
>>
>
Powered by blists - more mailing lists