[PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()

[Laurent Pinchart]

On Wed, Jun 29, 2022 at 03:14:54PM +0300, Tomi Valkeinen wrote:
> On 29/06/2022 12:41, Hans Verkuil wrote:
> > Hi Marek, Tomi, Laurent,
> > 
> > On 27/06/2022 19:41, Marek Vasut wrote:
> >> Any local subdev state should be allocated and free'd using
> >> __v4l2_subdev_state_alloc()/__v4l2_subdev_state_free(), which
> >> takes care of calling .init_cfg() subdev op. Without this,
> >> subdev internal state might be uninitialized by the time
> >> any other subdev op is called.
> >>
> >> Signed-off-by: Marek Vasut <marex at denx.de>
> >> Cc: Alain Volmat <alain.volmat at foss.st.com>
> >> Cc: Alexandre Torgue <alexandre.torgue at foss.st.com>
> >> Cc: Amelie DELAUNAY <amelie.delaunay at foss.st.com>
> >> Cc: Hugues FRUCHET <hugues.fruchet at foss.st.com>
> >> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >> Cc: Philippe CORNU <philippe.cornu at foss.st.com>
> >> Cc: linux-stm32 at st-md-mailman.stormreply.com
> >> Cc: linux-arm-kernel at lists.infradead.org
> >> ---
> >> V2: Add FIXME comment above __v4l2_subdev_state_alloc() calls
> >> ---
> >>   drivers/media/platform/st/stm32/stm32-dcmi.c | 59 ++++++++++++--------
> >>   1 file changed, 37 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/st/stm32/stm32-dcmi.c b/drivers/media/platform/st/stm32/stm32-dcmi.c
> >> index c604d672c2156..c68d32931b277 100644
> >> --- a/drivers/media/platform/st/stm32/stm32-dcmi.c
> >> +++ b/drivers/media/platform/st/stm32/stm32-dcmi.c
> >> @@ -996,22 +996,30 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
> >>   			struct dcmi_framesize *sd_framesize)
> >>   {
> >>   	const struct dcmi_format *sd_fmt;
> >> +	static struct lock_class_key key;
> >>   	struct dcmi_framesize sd_fsize;
> >>   	struct v4l2_pix_format *pix = &f->fmt.pix;
> >> -	struct v4l2_subdev_pad_config pad_cfg;
> >> -	struct v4l2_subdev_state pad_state = {
> >> -		.pads = &pad_cfg
> >> -		};
> >> +	struct v4l2_subdev_state *sd_state;
> >>   	struct v4l2_subdev_format format = {
> >>   		.which = V4L2_SUBDEV_FORMAT_TRY,
> >>   	};
> >>   	bool do_crop;
> >>   	int ret;
> >>   
> >> +	/*
> >> +	 * FIXME: Drop this call, drivers are not supposed to use
> >> +	 * __v4l2_subdev_state_alloc().
> >> +	 */
> >> +	sd_state = __v4l2_subdev_state_alloc(dcmi->source, "dcmi:state->lock", &key);
> >> +	if (IS_ERR(sd_state))
> >> +		return PTR_ERR(sd_state);
> >> +
> > 
> > I've been reading the discussion for the v1 patch, and I seriously do not like this.
> 
> I don't like it either.
> 
> > My comments are not specifically for this patch, but for all cases where
> > __v4l2_subdev_state_alloc is called.
> 
> Just to emphasize it: afaics this is not an issue with the subdev state. 
> This driver was already broken. Before the subdev state change the fix 
> would have been to call source subdev's init_cfg. Now 
> __v4l2_subdev_state_alloc handles calling init_cfg (along with a few 
> other inits).
> 
> > It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
> > exactly trivial either.
> 
> Counting this one? I found 3 cases in v5.18-rc4:
> 
> 1) drivers/media/platform/renesas/rcar-vin/rcar-v4l2.c:
> 
> Allocates the state for v4l2_subdev_call, set_fmt, TRY.
> 
> Here, a helper macro which does the alloc would be a solution.
> 
> 2) drivers/media/platform/renesas/vsp1/vsp1_entity.c:
> 
> Allocates the state for storing internal active state.
> 
> Here, I think, the easiest fix is for the driver to use the subdev state 
> properly.
> 
> 3) drivers/staging/media/tegra-video/vi.c:
> 
> Allocates the state for v4l2_subdev_call, enum_frame_size and set_fmt, 
> TRY. Interestingly the code also calls get_selection but without passing 
> the state...
> 
> This is a more interesting case as the source's subdev state is actually 
> modified by the driver. The driver calls enum_frame_size, then modifies 
> the state, then calls set_fmt. I'm not sure if that's really legal... 
> The driver directly modifies the state, instead of calling set_selection?
> 
> > I think a helper function might be beneficial, but the real problem is with the
> > comment: it does not explain why you shouldn't use it and what needs to be done
> > to fix it.
> 
> That is true. There's no single answer to that. I think instead of 
> trying to document that in the v4l2-subdev doc, we can enhance the 
> comments in those three call sites to explain how it needs to be fixed.
> 
> > My suggestion would be to document that in the kerneldoc for this function in
> > media/v4l2-subdev.h, and then refer to that from this comment (and similar comments
> > in the other drivers that use this).
> > 
> > And another question: are more drivers affected by this? Is it possible to
> > find those and fix them all?
> 
> I think any driver that calls a source subdev's subdev ops which a 
> subdev state as a parameter (the ones that used to take 
> v4l2_subdev_pad_config), and does not call init_cfg is broken in the 
> same way. With some grepping, I couldn't find anyone calling init_cfg. 
> Finding those drivers which do those calls is a bit more difficult, but 
> can be done with some efforts.

A quick check identified the following files:

atmel-isc-base.c
atmel-isi.c
cxusb-analog.c
fimc-capture.c
mcam-core.c
pxa_camera.c
renesas-ceu.c
saa7134-empress.c
via-camera.c

A few drivers with more complex call patterns may be missing.

> atmel-isc-base.c is one I found, and looks like it's doing something a 
> bit similar to the 3) case above.
> 
> Perhaps the best way to solve this is just to remove the underscores 
> from __v4l2_subdev_state_alloc, and change all the drivers which create 
> temporary v4l2_subdev_states to use that (and the free) functions. And 
> also create the helper macro which can be used in those cases where the 
> call is simple (the state is not modified or accessed by the caller).

As long as we prevent any new driver from using that API, that's fine
with me.

> It would've been nice to keep __v4l2_subdev_state_alloc internal to the 
> v4l2-subdev, but maybe the v4l2 drivers are not there yet. The non-MC 
> drivers seem to be doing all kinds of interesting things.

-- 
Regards,

Laurent Pinchart