From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1C0C6C433EF for ; Wed, 29 Jun 2022 12:21:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232804AbiF2MVW (ORCPT ); Wed, 29 Jun 2022 08:21:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54108 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231924AbiF2MVV (ORCPT ); Wed, 29 Jun 2022 08:21:21 -0400 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A66FC30544 for ; Wed, 29 Jun 2022 05:21:20 -0700 (PDT) Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id EC9283D7; Wed, 29 Jun 2022 14:21:18 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1656505279; bh=53v3k8hByNWfkMxAne8Ll0EBW8HCVXWhiXHa/u7UO4I=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=fM6hSw2y29VzIu1nckGrZTE+DYJ5Ys4nEPcHaj1K/uTp/0zy6zjc/RfBX+iPIFxU/ qrjSqFR56YJrw8lwSC5kMYootcHvMWuvG2YWLEGN3FEsWDYmyPIMNt98bErzDTwLYz Epb3adjh3xn1JfLczip6rxrG0kIgkpFsshKXkgeg= Date: Wed, 29 Jun 2022 15:20:59 +0300 From: Laurent Pinchart To: Tomi Valkeinen Cc: Hans Verkuil , Marek Vasut , linux-media@vger.kernel.org, Alain Volmat , Alexandre Torgue , Amelie DELAUNAY , Hugues FRUCHET , Philippe CORNU , linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc() Message-ID: References: <20220627174156.66919-1-marex@denx.de> <3ef88906-188d-52a6-c3bf-647bc4e36732@xs4all.nl> <32f04271-4a9a-3291-cf36-ead0383db9ca@ideasonboard.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <32f04271-4a9a-3291-cf36-ead0383db9ca@ideasonboard.com> Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org 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 > >> Cc: Alain Volmat > >> Cc: Alexandre Torgue > >> Cc: Amelie DELAUNAY > >> Cc: Hugues FRUCHET > >> Cc: Laurent Pinchart > >> Cc: Philippe CORNU > >> 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. > > > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5A7CEC433EF for ; Wed, 29 Jun 2022 12:22:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=aJ+43LusGb6/BajMyjZraWJjoZa84VI/GEIxq5KK4mI=; b=UQN8FU6CHiNm4U ZSkxfI+/Gc9fVwuWmJe0wIFZlr0bDyUlgnQ/Yz2N1BRRxrCy1dFdzQLx8u1dzl9ng3QVzfNXicM2C xLRMFLqJvRX8NS1V5TOuuZyUt9jSFmVr/LLicdqV8jLzhUB5K4HfqB4ssWIf6DM1DvE3T2D87nr8e uFDOcdMFoQ6TgZ8zCZA80OP2bZwXfrmwlZw/mRRXmRKpnglllLhKKKx0Hqbvbgm8fbZ2RHTrxKeSY cIuLMekL7lB9jbKBTE613blbhCLI7y4HplJ7pKCgdk5MN/69aXuGIhnr9vK6w/oi6ZWTBRQ/JXFU4 KSzXRgh6FGa4Q3MMPsYA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1o6WhD-00Blfu-LS; Wed, 29 Jun 2022 12:21:27 +0000 Received: from perceval.ideasonboard.com ([213.167.242.64]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1o6Wh9-00Ble2-Ln for linux-arm-kernel@lists.infradead.org; Wed, 29 Jun 2022 12:21:25 +0000 Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id EC9283D7; Wed, 29 Jun 2022 14:21:18 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1656505279; bh=53v3k8hByNWfkMxAne8Ll0EBW8HCVXWhiXHa/u7UO4I=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=fM6hSw2y29VzIu1nckGrZTE+DYJ5Ys4nEPcHaj1K/uTp/0zy6zjc/RfBX+iPIFxU/ qrjSqFR56YJrw8lwSC5kMYootcHvMWuvG2YWLEGN3FEsWDYmyPIMNt98bErzDTwLYz Epb3adjh3xn1JfLczip6rxrG0kIgkpFsshKXkgeg= Date: Wed, 29 Jun 2022 15:20:59 +0300 From: Laurent Pinchart To: Tomi Valkeinen Cc: Hans Verkuil , Marek Vasut , linux-media@vger.kernel.org, Alain Volmat , Alexandre Torgue , Amelie DELAUNAY , Hugues FRUCHET , Philippe CORNU , linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc() Message-ID: References: <20220627174156.66919-1-marex@denx.de> <3ef88906-188d-52a6-c3bf-647bc4e36732@xs4all.nl> <32f04271-4a9a-3291-cf36-ead0383db9ca@ideasonboard.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <32f04271-4a9a-3291-cf36-ead0383db9ca@ideasonboard.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220629_052123_918688_75A6A322 X-CRM114-Status: GOOD ( 50.63 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 > >> Cc: Alain Volmat > >> Cc: Alexandre Torgue > >> Cc: Amelie DELAUNAY > >> Cc: Hugues FRUCHET > >> Cc: Laurent Pinchart > >> Cc: Philippe CORNU > >> 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. > > > 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel