Re: [PATCH v5 1/4] media: nxp: imx8-isi: Use BIT_ULL() for 64-bit stream masks

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Routing validation checks the old routing state instead of the newly requested routing.
- [High] Stream translation silently overwrites sink pad and mixes streams from different inputs, violating hardware assumptions.
--

--- Patch [1]: [PATCH v5 1/4] media: nxp: imx8-isi: Use BIT_ULL() for 64-bit stream masks ---
commit 4ed3fcf5e835032b4860a0f0d335cd63bb0bee70
Author: Guoniu Zhou <[email protected]>

media: nxp: imx8-isi: Use BIT_ULL() for 64-bit stream masks

This commit replaces BIT() with BIT_ULL() for 64-bit stream masks to avoid
incorrect results on 32-bit architectures when stream IDs are 32 or greater.

> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> @@ -145,10 +145,10 @@ mxc_isi_crossbar_xlate_streams(struct mxc_isi_crossbar *xbar,
>  	 */
>  	for_each_active_route(&state->routing, route) {
>  		if (route->source_pad != source_pad ||
> -		    !(source_streams & BIT(route->source_stream)))
> +		    !(source_streams & BIT_ULL(route->source_stream)))
>  			continue;
>  
> -		sink_streams |= BIT(route->sink_stream);
> +		sink_streams |= BIT_ULL(route->sink_stream);
>  		sink_pad = route->sink_pad;

[Severity: High]
This isn't a regression introduced by this patch, but a pre-existing issue.
Could this stream translation silently overwrite the sink pad and mix streams
from different inputs?

In mxc_isi_crossbar_xlate_streams(), the loop iterates over all routes matching
the source pad and source_streams mask. It appears to blindly overwrite
sink_pad and bitwise-OR the sink_streams without checking if they originate
from the same sink pad.

If userspace sets up routing where streams from different sink pads are routed
to the same source pad on different stream ids, could this cause only the last
overwritten sink_pad to be returned?

This might lead to only one remote subdevice being enabled while abandoning
others, passing an invalidly mixed stream mask to the enabled device.

>  	}
>  

As an additional observation in this file:

[Severity: High]
This isn't a regression introduced by this patch, but a pre-existing issue.
Is the old routing state checked instead of the newly requested routing during
validation?

In __mxc_isi_crossbar_set_routing():

    for_each_active_route(&state->routing, route) {

When userspace calls VIDIOC_SUBDEV_S_ROUTING with an invalid route mapping the
memory input to a pipeline other than the first, does this validate the
request against the old routing table (&state->routing) rather than the new
routing array passed as a parameter?

Since the old table does not contain the invalid route yet, it appears this
check might pass incorrectly and allow impossible hardware configurations to
be accepted.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1