| 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: <0ef7834f-5efc-4cd3-91ac-482937debc3e@ti.com>
Date: Fri, 21 Nov 2025 12:32:41 +0530
From: Rishikesh Donadkar <r-donadkar@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>, <jai.luthra@...ux.dev>,
<laurent.pinchart@...asonboard.com>, <mripard@...nel.org>
CC: <y-abhilashchandra@...com>, <devarsht@...com>, <s-jain1@...com>,
<vigneshr@...com>, <mchehab@...nel.org>, <robh@...nel.org>,
<krzk+dt@...nel.org>, <p.zabel@...gutronix.de>, <conor+dt@...nel.org>,
<sakari.ailus@...ux.intel.com>, <hverkuil-cisco@...all.nl>,
<jai.luthra@...asonboard.com>, <changhuang.liang@...rfivetech.com>,
<jack.zhu@...rfivetech.com>, <sjoerd@...labora.com>,
<dan.carpenter@...aro.org>, <hverkuil+cisco@...nel.org>,
<linux-kernel@...r.kernel.org>, <linux-media@...r.kernel.org>,
<devicetree@...r.kernel.org>
Subject: Re: [PATCH v8 07/18] media: cadence: csi2rx: Move to
.enable/disable_streams API
On 20/11/25 18:30, Tomi Valkeinen wrote:
> Hi,
Hi Tomi
>
> On 12/11/2025 13:54, Rishikesh Donadkar wrote:
>> The enable_streams() API in v4l2 supports passing a bitmask to enable
>> each pad/stream combination individually on any media subdev. Use this
>> API instead of s_stream() API.
>>
>> Implement the enable_stream and disable_stream hooks in place of the
>> stream-unaware s_stream hook.
>>
>> Remove the lock that was used to serialize stream starts/stops which
>> is not required anymore since the v4l2-core serializes the
>> enable/disable_streams() calls for the subdev.
>>
>> Signed-off-by: Rishikesh Donadkar <r-donadkar@...com>
>> ---
>> drivers/media/platform/cadence/cdns-csi2rx.c | 108 +++++++++----------
>> 1 file changed, 54 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
>> index 8c19f125da3e5..34da81893308b 100644
>> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
>> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
>> @@ -125,12 +125,6 @@ struct csi2rx_priv {
>> unsigned int count;
>> int error_irq;
>>
>> - /*
>> - * Used to prevent race conditions between multiple,
>> - * concurrent calls to start and stop.
>> - */
>> - struct mutex lock;
>> -
>> void __iomem *base;
>> struct clk *sys_clk;
>> struct clk *p_clk;
>> @@ -268,20 +262,21 @@ static int csi2rx_configure_ext_dphy(struct csi2rx_priv *csi2rx)
>> &csi2rx->source_subdev->entity.pads[csi2rx->source_pad];
>> union phy_configure_opts opts = { };
>> struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
>> - struct v4l2_subdev_format sd_fmt = {
>> - .which = V4L2_SUBDEV_FORMAT_ACTIVE,
>> - .pad = CSI2RX_PAD_SINK,
>> - };
>> + struct v4l2_subdev_state *state;
>> + struct v4l2_mbus_framefmt *framefmt;
>> const struct csi2rx_fmt *fmt;
>> s64 link_freq;
>> int ret;
>>
>> - ret = v4l2_subdev_call_state_active(&csi2rx->subdev, pad, get_fmt,
>> - &sd_fmt);
>> - if (ret < 0)
>> - return ret;
>> + state = v4l2_subdev_get_locked_active_state(&csi2rx->subdev);
>>
>> - fmt = csi2rx_get_fmt_by_code(sd_fmt.format.code);
>> + framefmt = v4l2_subdev_state_get_format(state, CSI2RX_PAD_SINK, 0);
>> + if (!framefmt) {
>> + dev_err(csi2rx->dev, "Did not find active sink format\n");
>> + return -EINVAL;
>> + }
>> +
>> + fmt = csi2rx_get_fmt_by_code(framefmt->code);
>>
>> link_freq = v4l2_get_link_freq(src_pad,
>> fmt->bpp, 2 * csi2rx->num_lanes);
>> @@ -401,16 +396,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>>
>> reset_control_deassert(csi2rx->sys_rst);
>>
>> - ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true);
>> - if (ret)
>> - goto err_disable_sysclk;
>> -
>> clk_disable_unprepare(csi2rx->p_clk);
>>
>> return 0;
>>
>> -err_disable_sysclk:
>> - clk_disable_unprepare(csi2rx->sys_clk);
>> err_disable_pixclk:
>> for (; i > 0; i--) {
>> reset_control_assert(csi2rx->pixel_rst[i - 1]);
>> @@ -459,9 +448,6 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>> reset_control_assert(csi2rx->p_rst);
>> clk_disable_unprepare(csi2rx->p_clk);
>>
>> - if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false))
>> - dev_warn(csi2rx->dev, "Couldn't disable our subdev\n");
>> -
>> if (csi2rx->dphy) {
>> writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG);
>>
>> @@ -485,38 +471,56 @@ static int csi2rx_log_status(struct v4l2_subdev *sd)
>> return 0;
>> }
>>
>> -static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
>> +static int csi2rx_enable_streams(struct v4l2_subdev *subdev,
>> + struct v4l2_subdev_state *state, u32 pad,
>> + u64 streams_mask)
>> {
>> struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
>> - int ret = 0;
>> -
>> - mutex_lock(&csi2rx->lock);
>> -
>> - if (enable) {
>> - /*
>> - * If we're not the first users, there's no need to
>> - * enable the whole controller.
>> - */
>> - if (!csi2rx->count) {
>> - ret = csi2rx_start(csi2rx);
>> - if (ret)
>> - goto out;
>> - }
>> + int ret;
>>
>> - csi2rx->count++;
>> - } else {
>> - csi2rx->count--;
>> + /*
>> + * If we're not the first users, there's no need to
>> + * enable the whole controller.
>> + */
>> + if (!csi2rx->count) {
>> + ret = csi2rx_start(csi2rx);
>> + if (ret)
>> + return ret;
>> + }
>>
>> - /*
>> - * Let the last user turn off the lights.
>> - */
>> + /* Start streaming on the source */
>> + ret = v4l2_subdev_enable_streams(csi2rx->source_subdev, csi2rx->source_pad,
>> + BIT(0));
> The stream mask is u64, so better use BIT_U64. It doesn't matter here,
> but before you know it, you have switched BIT(0) to BIT(foo->bar), and
> someone sets bar to 34 and it's broken...
>
> Yes, I see this being changed later, to variable set with BIT_ULL()
> (that could be changed to BIT_U64). Nevertheless, better use the correct
> macro.
Sure, I will change it in the next revision.
>
> But a bigger issue is if this and patches 6 and 8 are correct or could
> be improved.
>
> I think in this one you remove s_stream from cdns-csi2. But the j7
> csi2rx is still calling s_stream, so it would fail with this patch. In
> patch 8 you switch j7 csi2rx from s_stream to enable_streams. But in
> patch 6 you added mutex, locking and s_stream to j7 csi2rx, and in patch
> 8 you remove that.
>
> Maybe the end result is fine, but it all does feel messy. I think the
> patches we should have are:
>
> - Use v4l2_subdev_enable_streams() in j7-csi2rx instead of calling
> s_stream. v4l2_subdev_enable_streams() will work with either .s_stream
> and .enable_stream in cdns-csi2rx
>
> - Patch 7 (with BIT_64()) to convert cdns-csi2rx
>
> - Patches 6 and 8 combined. You're essentially adding a new subdev, just
> do it right from the start with .enable_streams, state, etc., instead of
> mutex and s_stream which are removed two patches later.
Right, I will merge changes from 08/18 into 06/18.
Thanks,
Rishikesh
>
> Tomi
>
>> + if (ret) {
>> + dev_err(csi2rx->dev,
>> + "Failed to start streams %d on subdev\n", 0);
>> if (!csi2rx->count)
>> csi2rx_stop(csi2rx);
>> + return ret;
>> }
>>
>> -out:
>> - mutex_unlock(&csi2rx->lock);
>> - return ret;
>> + csi2rx->count++;
>> + return 0;
>> +}
>> +
>> +static int csi2rx_disable_streams(struct v4l2_subdev *subdev,
>> + struct v4l2_subdev_state *state, u32 pad,
>> + u64 streams_mask)
>> +{
>> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
>> +
>> + if (v4l2_subdev_disable_streams(csi2rx->source_subdev,
>> + csi2rx->source_pad, BIT(0))) {
>> + dev_err(csi2rx->dev, "Couldn't disable our subdev\n");
>> + }
>> +
>> + csi2rx->count--;
>> +
>> + /* Let the last user turn off the lights. */
>> + if (!csi2rx->count)
>> + csi2rx_stop(csi2rx);
>> +
>> + return 0;
>> }
>>
>> static int csi2rx_enum_mbus_code(struct v4l2_subdev *subdev,
>> @@ -611,10 +615,8 @@ static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
>> .enum_mbus_code = csi2rx_enum_mbus_code,
>> .get_fmt = v4l2_subdev_get_fmt,
>> .set_fmt = csi2rx_set_fmt,
>> -};
>> -
>> -static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
>> - .s_stream = csi2rx_s_stream,
>> + .enable_streams = csi2rx_enable_streams,
>> + .disable_streams = csi2rx_disable_streams,
>> };
>>
>> static const struct v4l2_subdev_core_ops csi2rx_core_ops = {
>> @@ -623,7 +625,6 @@ static const struct v4l2_subdev_core_ops csi2rx_core_ops = {
>>
>> static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
>> .core = &csi2rx_core_ops,
>> - .video = &csi2rx_video_ops,
>> .pad = &csi2rx_pad_ops,
>> };
>>
>> @@ -829,7 +830,6 @@ static int csi2rx_probe(struct platform_device *pdev)
>> return -ENOMEM;
>> platform_set_drvdata(pdev, csi2rx);
>> csi2rx->dev = &pdev->dev;
>> - mutex_init(&csi2rx->lock);
>>
>> ret = csi2rx_get_resources(csi2rx, pdev);
>> if (ret)
Powered by blists - more mailing lists