Re: [PATCH v3 5/5] media: qcom: camss: Add support for Hawi

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] media: qcom: camss: unconditional AUP strobes in csid_configure_stream()
- [High] media: camss: NULL hw_ops deref in msm_csiphy_subdev_init_legacy()
- [Medium] media: qcom: camss: wrong camnoc_rt_axi clock rates in vfe_res_hawi
- [Medium] media: qcom: camss: data race on csid->addr_update and csid->reg_update
- [Low] media: qcom: camss: duplicate CSID 1080 and VFE 1080 code in 1190 files
- [Low] media: qcom: camss: macros in camss-csid-1190.c implicitly capture csid
--

commit 25967aac8ac2c5b636dbbaab24c84dd484d99ab8
Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>

media: qcom: camss: Add support for Hawi

This commit adds CAMSS support for the Hawi SoC, including CSID 1190 and
VFE 1190 hardware operations and Hawi resource definitions.

> diff --git a/drivers/media/platform/qcom/camss/camss-csid-1190.c b/drivers/media/platform/qcom/camss/camss-csid-1190.c
> new file mode 100644
> index 0000000000000..4bd2cce094402
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-1190.c
> @@ -0,0 +1,417 @@

[ ... ]

> +#define MSM_CSID_MAX_SRC_STREAMS_1190		(csid_is_lite(csid) ? 4 : 5)
> +
> +/* RDI Configuration */
> +#define CSID_RDI_CFG0(rdi)	(csid_is_lite(csid) ?\
> +					(0x3080 + 0x200 * (rdi)) :\
> +					(0x5480 + 0x200 * (rdi)))
> +#define		RDI_CFG0_RETIME_BS			BIT(5)
> +#define		RDI_CFG0_TIMESTAMP_EN			BIT(6)
> +#define		RDI_CFG0_TIMESTAMP_STB_SEL		BIT(8)
> +#define		RDI_CFG0_DECODE_FORMAT			12
> +#define		RDI_CFG0_DT				16
> +#define		RDI_CFG0_VC				22
> +#define		RDI_CFG0_EN				BIT(31)
> +
> +/* RDI Control and Configuration */
> +#define CSID_RDI_CTRL(rdi)	(csid_is_lite(csid) ?\
> +					(0x3088 + 0x200 * (rdi)) :\
> +					(0x5488 + 0x200 * (rdi)))
> +#define		RDI_CTRL_START_CMD			BIT(0)
> +
> +#define CSID_RDI_CFG1(rdi)	(csid_is_lite(csid) ?\
> +					(0x3094 + 0x200 * (rdi)) :\
> +					(0x5494 + 0x200 * (rdi)))

[Severity: Low]
As reported by Sashiko on v2 [1], this isn't a bug, but
MSM_CSID_MAX_SRC_STREAMS_1190, CSID_RDI_CFG0(rdi), CSID_RDI_CTRL(rdi), and
CSID_RDI_CFG1(rdi) in drivers/media/platform/qcom/camss/camss-csid-1190.c
expand to expressions calling csid_is_lite(csid) without taking csid as a
macro parameter. Should these macros take csid explicitly instead of relying
on a local variable named csid in the caller's scope?

[ ... ]

> +static void __csid_aup_rup_trigger(struct csid_device *csid)
> +{
> +	/* trigger SET in combined register */
> +	writel(RUP_SET, csid->base + CSID_RUP_AUP_CMD);
> +}
> +
> +static void __csid_addr_update(struct csid_device *csid, int port_id)
> +{
> +	csid->addr_update |= CSID_RUP_AUP_RDI(port_id);
> +	writel(csid->addr_update, csid->base + CSID_AUP_CMD);
> +
> +	__csid_aup_rup_trigger(csid);
> +}
> +
> +static void __csid_addr_update_clear(struct csid_device *csid, int port_id)
> +{
> +	/* Hardware clears the registers upon consuming the settings */
> +	csid->addr_update &= ~CSID_RUP_AUP_RDI(port_id);
> +}
> +
> +static void __csid_reg_update(struct csid_device *csid, int port_id)
> +{
> +	csid->reg_update |= CSID_RUP_AUP_RDI(port_id);
> +	writel(csid->reg_update, csid->base + CSID_RUP_CMD);
> +
> +	__csid_aup_rup_trigger(csid);
> +}
> +
> +static void __csid_reg_update_clear(struct csid_device *csid, int port_id)
> +{
> +	/* Hardware clears the registers upon consuming the settings */
> +	csid->reg_update &= ~CSID_RUP_AUP_RDI(port_id);
> +}

[Severity: Medium]
As reported by Sashiko on v2 [1], can __csid_addr_update() and
__csid_reg_update() race with __csid_addr_update_clear() and
__csid_reg_update_clear() on csid->addr_update and csid->reg_update?

In drivers/media/platform/qcom/camss/camss-csid-1190.c, __csid_addr_update()
and __csid_reg_update() perform non-atomic read-modify-write operations on
csid->addr_update and csid->reg_update before writing CSID_AUP_CMD and
CSID_RUP_CMD.

Meanwhile, in hardirq context, csid_isr() clears those fields when
INFO_RUP_DONE is set without holding any lock:

drivers/media/platform/qcom/camss/camss-csid-1190.c:csid_isr() {
    ...
			if (val & INFO_RUP_DONE) {
				__csid_addr_update_clear(csid, i);
				__csid_reg_update_clear(csid, i);
			}
    ...
}

Because vfe_queue_buffer_v2() and vfe_enable_output_v2() only hold
vfe->output_lock (which csid_isr() does not acquire before camss_buf_done())
and csid_configure_stream() holds no spinlock, concurrent updates from process
context (for example during VIDIOC_QBUF or when starting multiple RDI streams
sharing a CSID) and hardirq context can clobber bits in csid->addr_update and
csid->reg_update.

[ ... ]

> +static void csid_configure_stream(struct csid_device *csid, u8 enable)
> +{
> +	u8 i, k;
> +
> +	__csid_configure_rx(csid, &csid->phy);
> +
> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS_1190; i++) {
> +		if (csid->phy.en_vc & BIT(i)) {
> +			__csid_configure_rdi_stream(csid, enable, i, 0);
> +			__csid_configure_rx_vc(csid, 0);
> +
> +			for (k = 0; k < CAMSS_INIT_BUF_COUNT; k++)
> +				__csid_addr_update(csid, i);
> +
> +			__csid_reg_update(csid, i);
> +
> +			__csid_ctrl_rdi(csid, enable, i);
> +		}
> +	}
> +}

[Severity: High]
What happens in csid_configure_stream() if VIDIOC_STREAMON is called with
fewer than CAMSS_INIT_BUF_COUNT (2) buffers queued?

In msm_video_register() in drivers/media/platform/qcom/camss/camss-video.c,
vb2_queue.min_queued_buffers is 0, so vb2_core_streamon() invokes
vb2_start_streaming() -> video_start_streaming() even when 0 or 1 buffer is
queued before VIDIOC_STREAMON.

video_start_streaming() first calls vfe_set_stream() -> vfe_enable_v2() ->
vfe_enable_output_v2(), which dequeues only the available pending buffers (0
or 1) and writes their addresses via ops->vfe_wm_update():

drivers/media/platform/qcom/camss/camss-vfe.c:vfe_enable_output_v2() {
    ...
	for (i = 0; i < CAMSS_INIT_BUF_COUNT; i++) {
		output->buf[i] = vfe_buf_get_pending(output);
		if (!output->buf[i])
			break;
		output->gen2.active_num++;
		ops->vfe_wm_update(vfe, output->wm_idx[0],
				   output->buf[i]->addr[0], line);
		if (!ops->addr_update)
			ops->reg_update(vfe, line->id);
	}
    ...
}

Next, video_start_streaming() calls csid_set_stream() ->
csid_configure_stream(), which unconditionally loops CAMSS_INIT_BUF_COUNT (2)
times calling __csid_addr_update(csid, i) regardless of how many buffers were
queued in vfe_enable_output_v2(). Moreover, vfe_wm_stop() in
drivers/media/platform/qcom/camss/camss-vfe-1190.c only clears
VFE_BUS_WM_CFG(wm) without clearing VFE_BUS_WM_IMAGE_ADDR(wm).

When userspace later queues buffers via VIDIOC_QBUF, vfe_queue_buffer_v2()
sees output->state == VFE_OUTPUT_ON and output->gen2.active_num < 2, and
issues additional ops->addr_update() and ops->reg_update() strobes:

drivers/media/platform/qcom/camss/camss-vfe.c:vfe_queue_buffer_v2() {
    ...
	if (output->state == VFE_OUTPUT_ON &&
	    output->gen2.active_num < 2) {
		output->buf[output->gen2.active_num++] = buf;
		ops->vfe_wm_update(vfe, output->wm_idx[0],
				   buf->addr[0], line);
		if (ops->addr_update)
			ops->addr_update(vfe, line->id);
		ops->reg_update(vfe, line->id);
	}
    ...
}

Doesn't issuing two unconditional __csid_addr_update() strobes in
csid_configure_stream() latch stale or duplicate DMA addresses in the write
master and desynchronize buffer tracking when fewer than two buffers were
queued before streamon?

[ ... ]

> +const struct csid_hw_ops csid_ops_1190 = {
> +	.configure_stream = csid_configure_stream,
> +	.configure_testgen_pattern = csid_configure_testgen_pattern,
> +	.hw_version = csid_hw_version,
> +	.isr = csid_isr,
> +	.reset = csid_reset,
> +	.src_pad_code = csid_src_pad_code,
> +	.subdev_init = csid_subdev_init,
> +	.reg_update = csid_subdev_reg_update,
> +	.addr_update = csid_subdev_addr_update,
> +};

[Severity: Low]
This isn't a bug, but every register offset, bitmask macro, stream count
definition, helper function, interrupt handler, reset routine, and hardware
operation callback in drivers/media/platform/qcom/camss/camss-csid-1190.c
(including the unused lane_cnt local variable in
__csid_configure_rdi_stream()) is identical to
drivers/media/platform/qcom/camss/camss-csid-1080.c.

Similarly, drivers/media/platform/qcom/camss/camss-vfe-1190.c duplicates all
functions and callbacks of drivers/media/platform/qcom/camss/camss-vfe-1080.c
verbatim, differing only in the BUS_REG_BASE and write master register offset
macros.

Could csid_res_hawi reuse csid_ops_1080 directly (just as hawi_resources
reuses csiphy_res_kaanapali and tpg_res_x1e80100), and could the common VFE
1080/1190 helpers be shared?

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index df4b4cf30711b..0a67d4f746eeb 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -4359,6 +4359,218 @@ static const struct resources_icc icc_res_sa8775p[] = {
>  	},
>  };
>  
> +static const struct camss_subdev_resources vfe_res_hawi[] = {
> +	/* VFE0 - TFE Full */
> +	{
> +		.clock = { "vfe0_fast_ahb", "vfe0", "cpas_vfe0",
> +			   "camnoc_rt_axi", "camnoc_nrt_axi", "qdss_debug_xo",
> +			   "cpas_mnoc_hf", "cpas_mnoc_sf" },
> +		.clock_rate = { { 0 },
> +				{ 238200000, 397000000, 440000000, 480000000,
> +				  630000000, 750000000, 833000000 },
> +				{ 0 },
> +				{ 171428571, 240000000, 300000000, 400000000,
> +				  480000000, 600000000 },

[Severity: Medium]
Are the camnoc_rt_axi clock rates in vfe_res_hawi (for VFE0 through VFE4)
matching the Hawi CAMCC clock driver?

In drivers/clk/qcom/camcc-hawi.c, ftbl_cam_cc_camnoc_rt_axi_clk_src for
qcom,hawi-camcc defines a maximum rate of 480 MHz:

drivers/clk/qcom/camcc-hawi.c:
static const struct freq_tbl ftbl_cam_cc_camnoc_rt_axi_clk_src[] = {
	F(19200000, P_BI_TCXO, 1, 0, 0),
	F(120000000, P_CAM_CC_PLL6_OUT_EVEN, 4, 0, 0),
	F(160000000, P_CAM_CC_PLL6_OUT_EVEN, 3, 0, 0),
	F(240000000, P_CAM_CC_PLL6_OUT_EVEN, 2, 0, 0),
	F(320000000, P_CAM_CC_PLL6_OUT_EVEN, 1.5, 0, 0),
	F(480000000, P_CAM_CC_PLL6_OUT_EVEN, 1, 0, 0),
	{ }
};

(Whereas the 200/240/300/400/480/600 MHz table
ftbl_cam_cc_camnoc_rt_axi_clk_src_maili is only used for qcom,maili-camcc, and
171428571 Hz is in neither table.)

When vfe_get() -> vfe_set_clock_rates() runs with min_rate between 480 MHz and
600 MHz, it selects 600000000 Hz from vfe_res_hawi instead of rejecting the
rate, and clk_round_rate() on Hawi clamps it down to 480000000 Hz. When the
second pipeline entity calls vfe_get() -> vfe_check_clock_rates(), won't
clk_get_rate(clock->clk) < min_rate cause vfe_check_clock_rates() to fail with
-EBUSY?

[ ... ]

> @@ -6078,6 +6290,22 @@ static const struct camss_resources sm8650_resources = {
>  	.vfe_num = ARRAY_SIZE(vfe_res_sm8650),
>  };
>  
> +static const struct camss_resources hawi_resources = {
> +	.version = CAMSS_HAWI,
> +	.pd_name = "top",
> +	.pm_clks = { "cpas_fast_ahb", "cpas_ahb", "gcc_axi_hf" },
> +	.csiphy_res = csiphy_res_kaanapali,

[Severity: High]
As reported by Sashiko on v2 [2], can reusing csiphy_res_kaanapali in
hawi_resources lead to a NULL pointer dereference in
msm_csiphy_subdev_init_legacy() during probe?

The entries in csiphy_res_kaanapali do not set .csiphy.hw_ops, leaving hw_ops
as NULL. During camss_probe() -> camss_init_subdevices(),
camss_detect_legacy_phy() defaults camss->legacy_phy to true:

drivers/media/platform/qcom/camss/camss.c:camss_detect_legacy_phy() {
	camss->legacy_phy = true;

	/* Find first remote-endpoint and determine if its a PHY */
	for_each_endpoint_of_node(camss->dev->of_node, ep) {
        ...
}

If the qcom,hawi-camss node has no endpoints in its ports node (for example,
when enabling CAMSS to use its internal Test Pattern Generators without
external sensors or PHYs), camss->legacy_phy remains true.
camss_init_subdevices() then calls msm_csiphy_subdev_init_legacy(), which
unconditionally dereferences csiphy->res->hw_ops:

drivers/media/platform/qcom/camss/camss-csiphy.c:msm_csiphy_subdev_init_legacy() {
    ...
	ret = csiphy->res->hw_ops->init(csiphy);
    ...
}

[1] https://lore.kernel.org/r/20261013-hawi-maili-camss-v2-7-fc93222da89c@oss.qualcomm.com
[2] https://lore.kernel.org/r/20261013-hawi-maili-camss-v2-6-fc93222da89c@oss.qualcomm.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030-hawi-maili-camss-v3-0-9742bca8cef2@oss.qualcomm.com?part=5