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

[Tomi Valkeinen]

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.

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).

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.

  Tomi