linux-kernel - Re: [PATCH v2 11/23] staging: media: tegra-video: csi: add a check to tegra_channel_get_remote_csi

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: <20250916180418.3fa270a9@booty>
Date: Tue, 16 Sep 2025 18:04:18 +0200
From: Luca Ceresoli <luca.ceresoli@...tlin.com>
To: Svyatoslav Ryhel <clamor95@...il.com>
Cc: Thierry Reding <thierry.reding@...il.com>, Thierry Reding
 <treding@...dia.com>, Mikko Perttunen <mperttunen@...dia.com>, Jonathan
 Hunter <jonathanh@...dia.com>, Sowjanya Komatineni
 <skomatineni@...dia.com>, David Airlie <airlied@...il.com>, Simona Vetter
 <simona@...ll.ch>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann
 <tzimmermann@...e.de>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Prashant Gaikwad
 <pgaikwad@...dia.com>, Michael Turquette <mturquette@...libre.com>, Stephen
 Boyd <sboyd@...nel.org>, Mauro Carvalho Chehab <mchehab@...nel.org>, Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>, Dmitry Osipenko
 <digetx@...il.com>, Jonas Schwöbel
 <jonasschwoebel@...oo.de>, Charan Pedumuru <charan.pedumuru@...il.com>,
 dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
 linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-media@...r.kernel.org, linux-clk@...r.kernel.org,
 linux-staging@...ts.linux.dev
Subject: Re: [PATCH v2 11/23] staging: media: tegra-video: csi: add a check
 to tegra_channel_get_remote_csi_subdev

Hello Svyatoslav,

On Sat,  6 Sep 2025 16:53:32 +0300
Svyatoslav Ryhel <clamor95@...il.com> wrote:

> By default tegra_channel_get_remote_csi_subdev returns next device in pipe
> assuming it is CSI but in case of Tegra20 and Tegra30 it can also be VIP
> or even HOST. Lets check if returned device is actually CSI by comparing
> subdevice operations.

This is just for extra safety, or is there a real case where the lack
of this check creates some issues in your use case?

> --- a/drivers/staging/media/tegra-video/csi.c
> +++ b/drivers/staging/media/tegra-video/csi.c
> @@ -445,6 +445,22 @@ static const struct v4l2_subdev_ops tegra_csi_ops = {
>  	.pad    = &tegra_csi_pad_ops,
>  };
>  
> +struct v4l2_subdev *tegra_channel_get_remote_csi_subdev(struct tegra_vi_channel *chan)
> +{
> +	struct media_pad *pad;
> +	struct v4l2_subdev *subdev;
> +
> +	pad = media_pad_remote_pad_first(&chan->pad);
> +	if (!pad)
> +		return NULL;
> +
> +	subdev = media_entity_to_v4l2_subdev(pad->entity);
> +	if (!subdev)
> +		return NULL;
> +
> +	return subdev->ops == &tegra_csi_ops ? subdev : NULL;
> +}

I tested your series on a Tegra20 with a parallel camera, so using the
VIP for parallel input.

The added check on subdev->ops breaks probing the video device:

  tegra-vi 54080000.vi: failed to setup channel controls: -19
  tegra-vi 54080000.vi: failed to register channel 0 notifier: -19

This is because tegra20_chan_capture_kthread_start() is also calling
tegra_channel_get_remote_csi_subdev(), but when using VIP subdev->ops
points to tegra_vip_ops, not tegra_csi_ops.

Surely the "csi" infix in the function name here is misleading. At
quick glance I don't see a good reason for its presence however, as the
callers are not CSI-specific.

If such quick analysis is correct, instead of this diff we should:
 * not move the function out of vi.c
 * rename the function toremove the "_csi" infix
 * if a check is really needed (see comment above), maybe extend it:
   return (subdev->ops == &tegra_csi_ops ||
           subdev->ops == &tegra_vip_ops) ? subdev : NULL;

Let me know your thoughts.

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com