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 485F5C43334 for ; Wed, 29 Jun 2022 11:58:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232978AbiF2L6U (ORCPT ); Wed, 29 Jun 2022 07:58:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60858 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229911AbiF2L6S (ORCPT ); Wed, 29 Jun 2022 07:58:18 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1D74012096 for ; Wed, 29 Jun 2022 04:58:17 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id B702A61B6F for ; Wed, 29 Jun 2022 11:58:16 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4620FC341D8; Wed, 29 Jun 2022 11:58:14 +0000 (UTC) Message-ID: <5ff3220b-d8c9-430b-7e7a-621746ccf23d@xs4all.nl> Date: Wed, 29 Jun 2022 13:58:12 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc() Content-Language: en-US To: Marek Vasut , Tomi Valkeinen Cc: Alain Volmat , Alexandre Torgue , Amelie DELAUNAY , Hugues FRUCHET , Laurent Pinchart , Philippe CORNU , linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org References: <20220627174156.66919-1-marex@denx.de> <3ef88906-188d-52a6-c3bf-647bc4e36732@xs4all.nl> From: Hans Verkuil In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On 29/06/2022 13:04, Marek Vasut wrote: > 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 ? Yes. It would be nice if Tomi can make a patch fixing the comments as suggested above, and then your patch can go on top. Adding a helper function can be done later, it's not my main concern. > >> 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. Tomi, do you know? Regards, Hans 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 0A9ABC433EF for ; Wed, 29 Jun 2022 11:59:27 +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:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=+DdcMW2oFpyrKrp3Exg/U/qkrKk40x4zMI+YBrPnq/g=; b=TxPCY3K4tMC6ox b4nj3hMFJ5nlbQ6oU/46+dbPU+/OAG7Sd73VWQfUrdY/RUbDBOE3jRy0YA7jKwO3y8jyO5RLs+4H6 dpdHMhzKN5oHZLR8H2lrKroIHXBu5akfcYfcY1vCZaCF+HwOj03PulBK3gaxU80dFtiFYR79TMF03 RzT7zt5jek7toBddo/xcNQH1RStk+IYQMl3xNCNHDL0fd2D28sOVoEb1X91EY0eS2DgLIyhJ7GPC7 2qvwj4nG/jKO/W3qYD3qv/8tUNaqeNYtCwPw3FkwW0zxnddjSOnGDZa+hyvGRiBWIx/FWI0N6ubYh b3bgjDqyZTDEvf+t6udw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1o6WKs-00Bdak-1S; Wed, 29 Jun 2022 11:58:22 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1o6WKm-00BdYL-Rp for linux-arm-kernel@lists.infradead.org; Wed, 29 Jun 2022 11:58:19 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 4F59F61B6B; Wed, 29 Jun 2022 11:58:16 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4620FC341D8; Wed, 29 Jun 2022 11:58:14 +0000 (UTC) Message-ID: <5ff3220b-d8c9-430b-7e7a-621746ccf23d@xs4all.nl> Date: Wed, 29 Jun 2022 13:58:12 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.10.0 Subject: Re: [PATCH v2] media: stm32: dcmi: Switch to __v4l2_subdev_state_alloc() Content-Language: en-US To: Marek Vasut , Tomi Valkeinen Cc: Alain Volmat , Alexandre Torgue , Amelie DELAUNAY , Hugues FRUCHET , Laurent Pinchart , Philippe CORNU , linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org References: <20220627174156.66919-1-marex@denx.de> <3ef88906-188d-52a6-c3bf-647bc4e36732@xs4all.nl> From: Hans Verkuil In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220629_045817_013378_DA6249D2 X-CRM114-Status: GOOD ( 29.26 ) 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="utf-8" Content-Transfer-Encoding: base64 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org T24gMjkvMDYvMjAyMiAxMzowNCwgTWFyZWsgVmFzdXQgd3JvdGU6Cj4gT24gNi8yOS8yMiAxMTo0 MSwgSGFucyBWZXJrdWlsIHdyb3RlOgo+PiBIaSBNYXJlaywgVG9taSwgTGF1cmVudCwKPiAKPiBI aSwKPiAKPiBbLi4uXQo+IAo+Pj4gwqAgZHJpdmVycy9tZWRpYS9wbGF0Zm9ybS9zdC9zdG0zMi9z dG0zMi1kY21pLmMgfCA1OSArKysrKysrKysrKystLS0tLS0tLQo+Pj4gwqAgMSBmaWxlIGNoYW5n ZWQsIDM3IGluc2VydGlvbnMoKyksIDIyIGRlbGV0aW9ucygtKQo+Pj4KPj4+IGRpZmYgLS1naXQg YS9kcml2ZXJzL21lZGlhL3BsYXRmb3JtL3N0L3N0bTMyL3N0bTMyLWRjbWkuYyBiL2RyaXZlcnMv bWVkaWEvcGxhdGZvcm0vc3Qvc3RtMzIvc3RtMzItZGNtaS5jCj4+PiBpbmRleCBjNjA0ZDY3MmMy MTU2Li5jNjhkMzI5MzFiMjc3IDEwMDY0NAo+Pj4gLS0tIGEvZHJpdmVycy9tZWRpYS9wbGF0Zm9y bS9zdC9zdG0zMi9zdG0zMi1kY21pLmMKPj4+ICsrKyBiL2RyaXZlcnMvbWVkaWEvcGxhdGZvcm0v c3Qvc3RtMzIvc3RtMzItZGNtaS5jCj4+PiBAQCAtOTk2LDIyICs5OTYsMzAgQEAgc3RhdGljIGlu dCBkY21pX3RyeV9mbXQoc3RydWN0IHN0bTMyX2RjbWkgKmRjbWksIHN0cnVjdCB2NGwyX2Zvcm1h dCAqZiwKPj4+IMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgIHN0cnVjdCBkY21pX2ZyYW1lc2l6 ZSAqc2RfZnJhbWVzaXplKQo+Pj4gwqAgewo+Pj4gwqDCoMKgwqDCoCBjb25zdCBzdHJ1Y3QgZGNt aV9mb3JtYXQgKnNkX2ZtdDsKPj4+ICvCoMKgwqAgc3RhdGljIHN0cnVjdCBsb2NrX2NsYXNzX2tl eSBrZXk7Cj4+PiDCoMKgwqDCoMKgIHN0cnVjdCBkY21pX2ZyYW1lc2l6ZSBzZF9mc2l6ZTsKPj4+ IMKgwqDCoMKgwqAgc3RydWN0IHY0bDJfcGl4X2Zvcm1hdCAqcGl4ID0gJmYtPmZtdC5waXg7Cj4+ PiAtwqDCoMKgIHN0cnVjdCB2NGwyX3N1YmRldl9wYWRfY29uZmlnIHBhZF9jZmc7Cj4+PiAtwqDC oMKgIHN0cnVjdCB2NGwyX3N1YmRldl9zdGF0ZSBwYWRfc3RhdGUgPSB7Cj4+PiAtwqDCoMKgwqDC oMKgwqAgLnBhZHMgPSAmcGFkX2NmZwo+Pj4gLcKgwqDCoMKgwqDCoMKgIH07Cj4+PiArwqDCoMKg IHN0cnVjdCB2NGwyX3N1YmRldl9zdGF0ZSAqc2Rfc3RhdGU7Cj4+PiDCoMKgwqDCoMKgIHN0cnVj dCB2NGwyX3N1YmRldl9mb3JtYXQgZm9ybWF0ID0gewo+Pj4gwqDCoMKgwqDCoMKgwqDCoMKgIC53 aGljaCA9IFY0TDJfU1VCREVWX0ZPUk1BVF9UUlksCj4+PiDCoMKgwqDCoMKgIH07Cj4+PiDCoMKg wqDCoMKgIGJvb2wgZG9fY3JvcDsKPj4+IMKgwqDCoMKgwqAgaW50IHJldDsKPj4+IMKgICvCoMKg wqAgLyoKPj4+ICvCoMKgwqDCoCAqIEZJWE1FOiBEcm9wIHRoaXMgY2FsbCwgZHJpdmVycyBhcmUg bm90IHN1cHBvc2VkIHRvIHVzZQo+Pj4gK8KgwqDCoMKgICogX192NGwyX3N1YmRldl9zdGF0ZV9h bGxvYygpLgo+Pj4gK8KgwqDCoMKgICovCj4+PiArwqDCoMKgIHNkX3N0YXRlID0gX192NGwyX3N1 YmRldl9zdGF0ZV9hbGxvYyhkY21pLT5zb3VyY2UsICJkY21pOnN0YXRlLT5sb2NrIiwgJmtleSk7 Cj4+PiArwqDCoMKgIGlmIChJU19FUlIoc2Rfc3RhdGUpKQo+Pj4gK8KgwqDCoMKgwqDCoMKgIHJl dHVybiBQVFJfRVJSKHNkX3N0YXRlKTsKPj4+ICsKPj4KPj4gSSd2ZSBiZWVuIHJlYWRpbmcgdGhl IGRpc2N1c3Npb24gZm9yIHRoZSB2MSBwYXRjaCwgYW5kIEkgc2VyaW91c2x5IGRvIG5vdCBsaWtl IHRoaXMuCj4+Cj4+IE15IGNvbW1lbnRzIGFyZSBub3Qgc3BlY2lmaWNhbGx5IGZvciB0aGlzIHBh dGNoLCBidXQgZm9yIGFsbCBjYXNlcyB3aGVyZQo+PiBfX3Y0bDJfc3ViZGV2X3N0YXRlX2FsbG9j IGlzIGNhbGxlZC4KPj4KPj4gSXQgaXMgbm93IHVzZWQgaW4gNCBkcml2ZXJzLCBzbyB0aGF0J3Mg bm8gbG9uZ2VyIGEgcmFyZSBjYXNlLCBhbmQgdGhlIGNvZGUgaXNuJ3QKPj4gZXhhY3RseSB0cml2 aWFsIGVpdGhlci4KPj4KPj4gSSB0aGluayBhIGhlbHBlciBmdW5jdGlvbiBtaWdodCBiZSBiZW5l ZmljaWFsLCBidXQgdGhlIHJlYWwgcHJvYmxlbSBpcyB3aXRoIHRoZQo+PiBjb21tZW50OiBpdCBk b2VzIG5vdCBleHBsYWluIHdoeSB5b3Ugc2hvdWxkbid0IHVzZSBpdCBhbmQgd2hhdCBuZWVkcyB0 byBiZSBkb25lCj4+IHRvIGZpeCBpdC4KPj4KPj4gTXkgc3VnZ2VzdGlvbiB3b3VsZCBiZSB0byBk b2N1bWVudCB0aGF0IGluIHRoZSBrZXJuZWxkb2MgZm9yIHRoaXMgZnVuY3Rpb24gaW4KPj4gbWVk aWEvdjRsMi1zdWJkZXYuaCwgYW5kIHRoZW4gcmVmZXIgdG8gdGhhdCBmcm9tIHRoaXMgY29tbWVu dCAoYW5kIHNpbWlsYXIgY29tbWVudHMKPj4gaW4gdGhlIG90aGVyIGRyaXZlcnMgdGhhdCB1c2Ug dGhpcykuCj4gCj4gV291bGQgaXQgYmUgT0sgaWYgSSBsZWZ0IHRoZSBjb3JlIHJld29yay9kb2N1 bWVudGF0aW9uIHRvIFRvbWkgYXMgYSBzdWJzZXF1ZW50IHBhdGNoIHRvIHRoaXMgb25lID8KClll cy4gSXQgd291bGQgYmUgbmljZSBpZiBUb21pIGNhbiBtYWtlIGEgcGF0Y2ggZml4aW5nIHRoZSBj b21tZW50cyBhcyBzdWdnZXN0ZWQgYWJvdmUsCmFuZCB0aGVuIHlvdXIgcGF0Y2ggY2FuIGdvIG9u IHRvcC4gQWRkaW5nIGEgaGVscGVyIGZ1bmN0aW9uIGNhbiBiZSBkb25lIGxhdGVyLCBpdCdzCm5v dCBteSBtYWluIGNvbmNlcm4uCgo+IAo+PiBBbmQgYW5vdGhlciBxdWVzdGlvbjogYXJlIG1vcmUg ZHJpdmVycyBhZmZlY3RlZCBieSB0aGlzPyBJcyBpdCBwb3NzaWJsZSB0bwo+PiBmaW5kIHRob3Nl IGFuZCBmaXggdGhlbSBhbGw/Cj4gCj4gUHJvYmFibHksIEkgb25seSByYW4gaW50byBpdCB3aXRo IHRoZSBEQ01JIHNvIGZhci4KClRvbWksIGRvIHlvdSBrbm93PwoKUmVnYXJkcywKCglIYW5zCgpf X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpsaW51eC1hcm0t a2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJhZGVhZC5vcmcK aHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51eC1hcm0ta2Vy bmVsCg==