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
