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.
