Re: [PATCH v2 09/21] media: ipu6: Start streaming once all streams have started, stop when not

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Nicola,

Thank you for the review.

On Fri, Sep 18, 2026 at 09:36:19AM +0200, Nicola Fiorillo wrote:
> Hi Sakari,
> 
> On Thu, Sep 17, 2026 at 02:39:11PM +0300, Sakari Ailus wrote:
> > +static bool ipu6_isys_csi2_streaming_change(struct ipu6_isys_subdev *asd,
> > +					    struct v4l2_subdev_state *state,
> > +					    u32 pad, u8 *vc, bool enable)
> 
> Four things in this function, all found by reading it. I cannot build or
> test a kernel at the moment, so please take this as review and nothing
> more; none of it is reproduced on hardware.
> 
> 1) The error return does not fit the return type.
> 
> > +	ret = v4l2_subdev_call(remote_sd, pad, get_frame_desc,
> > +			       remote_pad->index, &desc);
> > +	if (ret)
> > +		return ret;
> 
> The function returns bool, so a negative errno from get_frame_desc()
> becomes true, and both callers read true as "go ahead and change the
> streaming state". In ipu6_isys_csi2_enable_streams() that starts the

Right; this needs to be reworked a little. I'll switch the return type to
int, I probably chose bool before I realised error handling was actually
required.

> firmware stream and then runs
> 
> 	csi2->streaming_vc |= BIT(vc);
> 
> with vc still uninitialised: *vc is assigned only at the end of the
> function, past this early return. ipu6_isys_csi2_disable_streams() does
> the mirror of it with &= ~BIT(vc). The two "return false" exits are
> fine, they make the caller skip the change; only this one carries a
> value.

vc is actually set if all goes well, so this is related to error handling.

> 
> 2) media_pad_remote_pad_unique() never returns NULL, so the guard below
>    it is dead code.

Indeed. This is where others have tripped, too. I'll fix this for v3.

I think I'll just switch to media_pad_remote_pad_first() as there's just a
single one. The check also can be removed as the MUST_CONNECT pad flag
guarantees there's an enabled link there. Let's see what smatch says...

> 
> > +		struct media_pad *video_pad =
> > +			media_pad_remote_pad_unique(&asd->sd.entity.pads[route->source_pad]);
> > +		struct ipu6_isys_video *av = !video_pad ? NULL :
> > +			container_of_const(video_pad,
> > +					   struct ipu6_isys_video, pad);
> > +
> > +		if (!av) {
> 
> It returns ERR_PTR(-ENOLINK) when no enabled link is found and
> ERR_PTR(-ENOTUNIQ) when there is more than one. !video_pad is therefore
> never true, the dev_dbg() below is unreachable, and the error pointer
> goes into container_of_const() instead -- where av->streaming, a few
> lines further down, dereferences it.
> 
> IS_ERR() is what the check wants. This same file already does it that
> way, in ipu6_isys_csi2_get_link_freq():
> 
> 	src_pad = media_pad_remote_pad_unique(...);
> 	if (IS_ERR(src_pad)) {
> 		dev_err(&csi2->isys->adev->auxdev.dev,
> 			"can't get source pad of %s (%pe)\n",
> 			csi2->asd.sd.name, src_pad);
> 		return PTR_ERR(src_pad);
> 	}
> 
> 3) The lookup on the sink pad, higher up in the same function, has no
>    check at all:
> 
> > +	struct media_pad *remote_pad =
> > +		media_pad_remote_pad_unique(&asd->sd.entity.pads[this_route->sink_pad]);
> > +	struct v4l2_subdev *remote_sd =
> > +		media_entity_to_v4l2_subdev(remote_pad->entity);
> 
> remote_pad->entity is read immediately, so the same error pointer is
> dereferenced here, with no guard to correct.

Also the MUST_CONNECT pad flag is set for the video device's pad. I'll
switch to media_pad_remote_pad_first() also here.

> 
> 4) Unless I am misreading it, the inner lookup in the loop does not
>    depend on the loop variable:
> 
> > +	for_each_active_route(&state->routing, route) {
> > +		struct v4l2_mbus_frame_desc_entry *entry = NULL;
> > +
> > +		for (unsigned int i = 0; i < desc.num_entries; i++) {
> > +			if (desc.entry[i].stream == this_entry->stream) {
> > +				entry = &desc.entry[i];
> > +				break;
> > +			}
> > +		}
> > +
> > +		if (entry->bus.csi2.vc != this_entry->bus.csi2.vc)
> > +			continue;
> 
> The search key is this_entry->stream, which does not change across
> iterations, so entry always ends up as this_entry itself and the vc
> comparison can never differ. The "continue" is never taken and every
> active route is counted, whatever its virtual channel. Going by the
> commit message, the key here was meant to be route->sink_stream.

Yes, the frame descriptor entries' streams was compared with wrong stream,
this needs to come from routing instead. There's also a check missing above
that an entry is actually found.

> 
> Finally, a note rather than a request. After the whole series,
> ipu6_isys_csi2_enable_streams() and ipu6_isys_csi2_disable_streams()
> still call media_pad_remote_pad_first() on the sink pad and dereference
> the result unchecked -- and that one does return NULL. It is what my
> [PATCH v2 1/3] touched. I have dropped that patch and I am not
> reopening the question of the scenario behind it; I mention it only
> because if the checks in 2) and 3) go in, that pointer is sitting right
> next to them.

I guess there are two things to consider here: 1) whether that pointer can
be NULL in the circumstances of this driver (shouldn't be) and 2) whether
static analysers are smart enough to determine the pointer cannot be NULL
(or that there's a bug and we've missed it's actually possible).

I'll reply to the framework patch separately.

-- 
Kind regards,

Sakari Ailus