Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc()
From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Date: 2022-06-29 12:27:06
Also in:
linux-media
On 29/06/2022 15:20, Laurent Pinchart wrote:
On Wed, Jun 29, 2022 at 03:14:54PM +0300, Tomi Valkeinen wrote:quoted
On 29/06/2022 12:41, Hans Verkuil wrote:quoted
Hi Marek, Tomi, Laurent, On 27/06/2022 19:41, Marek Vasut wrote:quoted
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@denx.de> Cc: Alain Volmat <alain.volmat@foss.st.com> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> Cc: Amelie DELAUNAY <amelie.delaunay@foss.st.com> Cc: Hugues FRUCHET <hugues.fruchet@foss.st.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Philippe CORNU <philippe.cornu@foss.st.com> Cc: linux-stm32@st-md-mailman.stormreply.com Cc: linux-arm-kernel@lists.infradead.org --- 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.quoted
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).quoted
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?quoted
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.quoted
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.A quick check identified the following files: atmel-isc-base.c atmel-isi.c cxusb-analog.c fimc-capture.c mcam-core.c pxa_camera.c renesas-ceu.c saa7134-empress.c via-camera.c A few drivers with more complex call patterns may be missing.quoted
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).As long as we prevent any new driver from using that API, that's fine with me.
An alternative would be to keep the v4l2_subdev_state as a local variable (allocated in the stack), and add a new function, v4l2_subdev_state_local_init() or such. The function would initialize the given state, expecting the allocatable fields to be already allocated (state->pads, which in the above cases points to another local variable, i.e. stack). This would prevent the need of a free call, which, while not complex as such, might cause a bigger amount of changes in some cases to handle the error paths correctly. Of course, if the above-mentioned macro works, then that's the easiest solution. But that won't work for all drivers. Tomi _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel