On Wed, Jun 29, 2022 at 03:14:54PM +0300, Tomi Valkeinen wrote:
> 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.
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.
> 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.
> 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.
--
Regards,
Laurent Pinchart
