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

[Hans Verkuil]

On 29/06/2022 13:04, Marek Vasut wrote:
> 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 ?

Yes. It would be nice if Tomi can make a patch fixing the comments as suggested above,
and then your patch can go on top. Adding a helper function can be done later, it's
not my main concern.

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

Tomi, do you know?

Regards,

	Hans