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

[Marek Vasut]

On 6/29/22 11:41, Hans Verkuil wrote:
> Hi Marek, Tomi, Laurent,

Hi,

[...]

>>   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.
> 
> My comments are not specifically for this patch, but for all cases where
> __v4l2_subdev_state_alloc is called.
> 
> It is now used in 4 drivers, so that's no longer a rare case, and the code isn't
> exactly trivial either.
> 
> 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.
> 
> 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).

Would it be OK if I left the core rework/documentation to Tomi as a 
subsequent patch to this one ?

> And another question: are more drivers affected by this? Is it possible to
> find those and fix them all?

Probably, I only ran into it with the DCMI so far.