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@xxxxxxx>
Cc: Alain Volmat <alain.volmat@xxxxxxxxxxx>
Cc: Alexandre Torgue <alexandre.torgue@xxxxxxxxxxx>
Cc: Amelie DELAUNAY <amelie.delaunay@xxxxxxxxxxx>
Cc: Hugues FRUCHET <hugues.fruchet@xxxxxxxxxxx>
Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Cc: Philippe CORNU <philippe.cornu@xxxxxxxxxxx>
Cc: linux-stm32@xxxxxxxxxxxxxxxxxxxxxxxxxxxx
Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
---
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
