Message ID | Y6qmNW6cOHjGwn03@qemulion |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4e01:0:0:0:0:0 with SMTP id p1csp1281313wrt; Tue, 27 Dec 2022 00:17:01 -0800 (PST) X-Google-Smtp-Source: AMrXdXtKxWyMmzS1fudrl2hUZqQyTZB55yaRwTNCBqCFFCzEHjeWgwtMDg7H+LqVT47aj2tWDKhg X-Received: by 2002:a17:906:c2cb:b0:842:32e9:f1e9 with SMTP id ch11-20020a170906c2cb00b0084232e9f1e9mr13614975ejb.69.1672129021617; Tue, 27 Dec 2022 00:17:01 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1672129021; cv=none; d=google.com; s=arc-20160816; b=gTIfZPdsvxHAFuo22k5I9xt74bIrxUXzNWBvqG2Tgvd11rHbyypPwX/3XeWweECT8F xwvWLUmS/8LGWjN6sB3nyGi2JOAibMn4wkeug3hxMn6RWvPPoFLg3pq/YOG2c0sya6U6 n/t3Rc2Q+gYbWfu3JDGaUxp+LRRuprHGNFvYOZOXfipSucV4fNqskdCdk0j18OG+gJwn eXQ4aaxgKHIXjxBXStXY3WM5PUdu6ewzOvFC/O8RuVTt61uDZ+Wk0giWRO15EA1jgSpV M5+uCNZGxP5DkaobGIiZw6AVego2btHucPJmAuaoQCMESbAh+BgPWffJU+IIBy9i2bZU ZiKw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-disposition:mime-version:message-id :subject:cc:to:from:date:dkim-signature; bh=BEBRi7o7havXJLgq4Lhd4ZqFEDBVoqgePGRsV7KOwsM=; b=lqC5u+FXOKvYg3Ux3EZWWaXiAyCsp/eHosSmBt8ipCWkhQPUUCot5uodRFb+cACo8V tdcKUGDHK57+6iUpUHzMoI2TBC1WzlJDDPxcY1utLEotzQ1Z+IukZWYjXd9m1Nrs3IJB CDMyep+IwaGTdWfz7wJ039UL5arI200hZ4hlI43A2A7wMbMQmYteDUs0ucZ45l0tWGab xMEBYP8kJUxvCxwcqAK63t9i4GNq43GlIknetsAG1+hJcj5fzvcF/phItubmDWmP2MS6 LE+4NnkYo0RT9txM8dqcBYDPczXfV05OqKpLF4cj0rWHMpdun+P2e7qsPkjPX3EZ38Ea vvTw== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=QxVY8yjo; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id hz4-20020a1709072ce400b007c163a17f44si10890994ejc.995.2022.12.27.00.16.37; Tue, 27 Dec 2022 00:17:01 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=QxVY8yjo; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229825AbiL0IBW (ORCPT <rfc822;eddaouddi.ayoub@gmail.com> + 99 others); Tue, 27 Dec 2022 03:01:22 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55420 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229614AbiL0IBT (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 27 Dec 2022 03:01:19 -0500 Received: from msg-2.mailo.com (msg-2.mailo.com [213.182.54.12]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 44628114F for <linux-kernel@vger.kernel.org>; Tue, 27 Dec 2022 00:01:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mailo.com; s=mailo; t=1672128061; bh=L7owGmbT1ZxgBvi6fbS8mXVWDglqw6W6df4BA5PYtMM=; h=X-EA-Auth:Date:From:To:Cc:Subject:Message-ID:MIME-Version: Content-Type; b=QxVY8yjoKzb0OTh6OmIxWa+uJk9lbvdtIva6tG/qKJiCZplp9WSDt6le7+JUI+CnL FFs4xPIeVx42lFD3oov338vMc6A6DtGnSXp5zpDA6+RSIJwgiz4vs1aQTtwSZRT700 3PUVumlKzgM2VxuTq1ewiY3ZdkGCGj7dzhY+i73k= Received: by b-5.in.mailobj.net [192.168.90.15] with ESMTP via ip-206.mailobj.net [213.182.55.206] Tue, 27 Dec 2022 09:01:01 +0100 (CET) X-EA-Auth: 055MqznYGGdxPOOuTU1LvLH5vLFB+IXYkeGlM26BeEaHaWLHphe/AO32mkbpjHKL2tQxrFN4WIgzNiFSMaanFHsHZ6uFhWf9 Date: Tue, 27 Dec 2022 13:30:53 +0530 From: Deepak R Varma <drv@mailo.com> To: Jani Nikula <jani.nikula@linux.intel.com>, Joonas Lahtinen <joonas.lahtinen@linux.intel.com>, Rodrigo Vivi <rodrigo.vivi@intel.com>, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>, David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Cc: Saurabh Singh Sengar <ssengar@microsoft.com>, Praveen Kumar <kumarpraveen@linux.microsoft.com>, Deepak R Varma <drv@mailo.com> Subject: [PATCH] drm/i915/fbc: Avoid full proxy f_ops for FBC debug attributes Message-ID: <Y6qmNW6cOHjGwn03@qemulion> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1753354360996867594?= X-GMAIL-MSGID: =?utf-8?q?1753354360996867594?= |
Series |
drm/i915/fbc: Avoid full proxy f_ops for FBC debug attributes
|
|
Commit Message
Deepak R Varma
Dec. 27, 2022, 8 a.m. UTC
Using DEFINE_SIMPLE_ATTRIBUTE macro with the debugfs_create_file()
function adds the overhead of introducing a proxy file operation
functions to wrap the original read/write inside file removal protection
functions. This adds significant overhead in terms of introducing and
managing the proxy factory file operations structure and function
wrapping at runtime.
As a replacement, a combination of DEFINE_DEBUGFS_ATTRIBUTE macro paired
with debugfs_create_file_unsafe() is suggested to be used instead. The
DEFINE_DEBUGFS_ATTRIBUTE utilises debugfs_file_get() and
debugfs_file_put() wrappers to protect the original read and write
function calls for the debug attributes. There is no need for any
runtime proxy file operations to be managed by the debugfs core.
This Change is reported by the debugfs_simple_attr.cocci Coccinelle
semantic patch.
Signed-off-by: Deepak R Varma <drv@mailo.com>
---
drivers/gpu/drm/i915/display/intel_fbc.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
--
2.34.1
Comments
On Tue, Dec 27, 2022 at 01:30:53PM +0530, Deepak R Varma wrote: > Using DEFINE_SIMPLE_ATTRIBUTE macro with the debugfs_create_file() > function adds the overhead of introducing a proxy file operation > functions to wrap the original read/write inside file removal protection > functions. This adds significant overhead in terms of introducing and > managing the proxy factory file operations structure and function > wrapping at runtime. > As a replacement, a combination of DEFINE_DEBUGFS_ATTRIBUTE macro paired > with debugfs_create_file_unsafe() is suggested to be used instead. The > DEFINE_DEBUGFS_ATTRIBUTE utilises debugfs_file_get() and > debugfs_file_put() wrappers to protect the original read and write > function calls for the debug attributes. There is no need for any > runtime proxy file operations to be managed by the debugfs core. > > This Change is reported by the debugfs_simple_attr.cocci Coccinelle > semantic patch. I just checked here with $ make coccicheck M=drivers/gpu/drm/i915/ MODE=context COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci The part reported by the this script is the s/SIMPLE/DEBUGFS but the change to the unsafe option is not. This commit message is not explaining why the unsafe is the suggested or who suggested it. If you remove the unsafe part feel free to resend adding: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> (to both patches, this and the drrs one. Also, it looks like you could contribute with other 2 patches: drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c:64:0-23: WARNING: pxp_terminate_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE drivers/gpu/drm/i915/gvt/debugfs.c:150:0-23: WARNING: vgpu_scan_nonprivbb_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > Signed-off-by: Deepak R Varma <drv@mailo.com> > --- > drivers/gpu/drm/i915/display/intel_fbc.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > index b5ee5ea0d010..4b481e2f908b 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -1809,10 +1809,10 @@ static int intel_fbc_debugfs_false_color_set(void *data, u64 val) > return 0; > } > > -DEFINE_SIMPLE_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > - intel_fbc_debugfs_false_color_get, > - intel_fbc_debugfs_false_color_set, > - "%llu\n"); > +DEFINE_DEBUGFS_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > + intel_fbc_debugfs_false_color_get, > + intel_fbc_debugfs_false_color_set, > + "%llu\n"); > > static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > struct dentry *parent) > @@ -1821,8 +1821,8 @@ static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > fbc, &intel_fbc_debugfs_status_fops); > > if (fbc->funcs->set_false_color) > - debugfs_create_file("i915_fbc_false_color", 0644, parent, > - fbc, &intel_fbc_debugfs_false_color_fops); > + debugfs_create_file_unsafe("i915_fbc_false_color", 0644, parent, > + fbc, &intel_fbc_debugfs_false_color_fops); > } > > void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc) > -- > 2.34.1 > > >
On Tue, Dec 27, 2022 at 12:13:56PM -0500, Rodrigo Vivi wrote: > On Tue, Dec 27, 2022 at 01:30:53PM +0530, Deepak R Varma wrote: > > Using DEFINE_SIMPLE_ATTRIBUTE macro with the debugfs_create_file() > > function adds the overhead of introducing a proxy file operation > > functions to wrap the original read/write inside file removal protection > > functions. This adds significant overhead in terms of introducing and > > managing the proxy factory file operations structure and function > > wrapping at runtime. > > As a replacement, a combination of DEFINE_DEBUGFS_ATTRIBUTE macro paired > > with debugfs_create_file_unsafe() is suggested to be used instead. The > > DEFINE_DEBUGFS_ATTRIBUTE utilises debugfs_file_get() and > > debugfs_file_put() wrappers to protect the original read and write > > function calls for the debug attributes. There is no need for any > > runtime proxy file operations to be managed by the debugfs core. > > > > This Change is reported by the debugfs_simple_attr.cocci Coccinelle > > semantic patch. > > I just checked here with > $ make coccicheck M=drivers/gpu/drm/i915/ MODE=context COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci Hello Rodrigo, Thank you so much for your review and feedback on the patch proposal. > > The part reported by the this script is the s/SIMPLE/DEBUGFS > but the change to the unsafe option is not. If you look at the original commit of this coccinelle file, it calls out the need for pairing debugfs_create_file_unsafe() as well. Please review this commitID: 5103068eaca2: ("debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage") Based on my review of the code, the functions debugfs_create_file() and debugfs_create_file_unsafe(), both internally call __debugfs_create_file(). However, they pass debugfs_full_proxy_file_operations and debugfs_open_proxy_file_operations respectively to it. The former represents the full proxy factory, where as the later one is lightweight open proxy implementation of the file operations structure. > > This commit message is not explaining why the unsafe is the suggested > or who suggested it. If you find the response above accurate, I will include these details about the _unsafe() function in my commit message in v2. > > If you remove the unsafe part feel free to resend adding: Please confirm you still believe switching to _unsafe() is not necessary. > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > (to both patches, this and the drrs one. > > Also, it looks like you could contribute with other 2 patches: > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c:64:0-23: WARNING: pxp_terminate_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE > drivers/gpu/drm/i915/gvt/debugfs.c:150:0-23: WARNING: vgpu_scan_nonprivbb_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE Yes, these are on my list. Was waiting for a feedback on the first submission before I send more similar patches. Appreciate your time and the feedback. Regards, ./drv > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > --- > > drivers/gpu/drm/i915/display/intel_fbc.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > index b5ee5ea0d010..4b481e2f908b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > @@ -1809,10 +1809,10 @@ static int intel_fbc_debugfs_false_color_set(void *data, u64 val) > > return 0; > > } > > > > -DEFINE_SIMPLE_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > > - intel_fbc_debugfs_false_color_get, > > - intel_fbc_debugfs_false_color_set, > > - "%llu\n"); > > +DEFINE_DEBUGFS_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > > + intel_fbc_debugfs_false_color_get, > > + intel_fbc_debugfs_false_color_set, > > + "%llu\n"); > > > > static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > > struct dentry *parent) > > @@ -1821,8 +1821,8 @@ static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > > fbc, &intel_fbc_debugfs_status_fops); > > > > if (fbc->funcs->set_false_color) > > - debugfs_create_file("i915_fbc_false_color", 0644, parent, > > - fbc, &intel_fbc_debugfs_false_color_fops); > > + debugfs_create_file_unsafe("i915_fbc_false_color", 0644, parent, > > + fbc, &intel_fbc_debugfs_false_color_fops); > > } > > > > void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc) > > -- > > 2.34.1 > > > > > >
On Tue, Dec 27, 2022 at 11:36:13PM +0530, Deepak R Varma wrote: > On Tue, Dec 27, 2022 at 12:13:56PM -0500, Rodrigo Vivi wrote: > > On Tue, Dec 27, 2022 at 01:30:53PM +0530, Deepak R Varma wrote: > > > Using DEFINE_SIMPLE_ATTRIBUTE macro with the debugfs_create_file() > > > function adds the overhead of introducing a proxy file operation > > > functions to wrap the original read/write inside file removal protection > > > functions. This adds significant overhead in terms of introducing and > > > managing the proxy factory file operations structure and function > > > wrapping at runtime. > > > As a replacement, a combination of DEFINE_DEBUGFS_ATTRIBUTE macro paired > > > with debugfs_create_file_unsafe() is suggested to be used instead. The > > > DEFINE_DEBUGFS_ATTRIBUTE utilises debugfs_file_get() and > > > debugfs_file_put() wrappers to protect the original read and write > > > function calls for the debug attributes. There is no need for any > > > runtime proxy file operations to be managed by the debugfs core. > > > > > > This Change is reported by the debugfs_simple_attr.cocci Coccinelle > > > semantic patch. > > > > I just checked here with > > $ make coccicheck M=drivers/gpu/drm/i915/ MODE=context COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > Hello Rodrigo, > Thank you so much for your review and feedback on the patch proposal. > > > > > The part reported by the this script is the s/SIMPLE/DEBUGFS > > but the change to the unsafe option is not. > > If you look at the original commit of this coccinelle file, it calls out the > need for pairing debugfs_create_file_unsafe() as well. Please review this > > commitID: 5103068eaca2: ("debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage") +Nicolai and Julia. It looks like coccinelle got right the - DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); + DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); but it failed badly on - debugfs_create_file(name, mode, parent, data, &dsa_fops) + debugfs_create_file_unsafe(name, mode, parent, data, &dsa_fops) > > Based on my review of the code, the functions debugfs_create_file() and > debugfs_create_file_unsafe(), both internally call __debugfs_create_file(). > However, they pass debugfs_full_proxy_file_operations and > debugfs_open_proxy_file_operations respectively to it. The former represents the > full proxy factory, where as the later one is lightweight open proxy > implementation of the file operations structure. > > > > > This commit message is not explaining why the unsafe is the suggested > > or who suggested it. > > If you find the response above accurate, I will include these details about > the _unsafe() function in my commit message in v2. > > > > > If you remove the unsafe part feel free to resend adding: > > Please confirm you still believe switching to _unsafe() is not necessary. Based on the coccinelle commit it looks like you are right, but cocinelle just failed to detect the case. Let's see what Nicolai and Julia respond before we move with any patch here. > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > (to both patches, this and the drrs one. > > > > Also, it looks like you could contribute with other 2 patches: > > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c:64:0-23: WARNING: pxp_terminate_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > drivers/gpu/drm/i915/gvt/debugfs.c:150:0-23: WARNING: vgpu_scan_nonprivbb_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > Yes, these are on my list. Was waiting for a feedback on the first submission > before I send more similar patches. > > Appreciate your time and the feedback. > > > Regards, > ./drv > > > > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_fbc.c | 12 ++++++------ > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > > index b5ee5ea0d010..4b481e2f908b 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > @@ -1809,10 +1809,10 @@ static int intel_fbc_debugfs_false_color_set(void *data, u64 val) > > > return 0; > > > } > > > > > > -DEFINE_SIMPLE_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > > > - intel_fbc_debugfs_false_color_get, > > > - intel_fbc_debugfs_false_color_set, > > > - "%llu\n"); > > > +DEFINE_DEBUGFS_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > > > + intel_fbc_debugfs_false_color_get, > > > + intel_fbc_debugfs_false_color_set, > > > + "%llu\n"); > > > > > > static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > > > struct dentry *parent) > > > @@ -1821,8 +1821,8 @@ static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > > > fbc, &intel_fbc_debugfs_status_fops); > > > > > > if (fbc->funcs->set_false_color) > > > - debugfs_create_file("i915_fbc_false_color", 0644, parent, > > > - fbc, &intel_fbc_debugfs_false_color_fops); > > > + debugfs_create_file_unsafe("i915_fbc_false_color", 0644, parent, > > > + fbc, &intel_fbc_debugfs_false_color_fops); > > > } > > > > > > void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc) > > > -- > > > 2.34.1 > > > > > > > > > > >
On Wed, Dec 28, 2022 at 06:18:12AM -0500, Rodrigo Vivi wrote: > On Tue, Dec 27, 2022 at 11:36:13PM +0530, Deepak R Varma wrote: > > On Tue, Dec 27, 2022 at 12:13:56PM -0500, Rodrigo Vivi wrote: > > > On Tue, Dec 27, 2022 at 01:30:53PM +0530, Deepak R Varma wrote: > > > > Using DEFINE_SIMPLE_ATTRIBUTE macro with the debugfs_create_file() > > > > function adds the overhead of introducing a proxy file operation > > > > functions to wrap the original read/write inside file removal protection > > > > functions. This adds significant overhead in terms of introducing and > > > > managing the proxy factory file operations structure and function > > > > wrapping at runtime. > > > > As a replacement, a combination of DEFINE_DEBUGFS_ATTRIBUTE macro paired > > > > with debugfs_create_file_unsafe() is suggested to be used instead. The > > > > DEFINE_DEBUGFS_ATTRIBUTE utilises debugfs_file_get() and > > > > debugfs_file_put() wrappers to protect the original read and write > > > > function calls for the debug attributes. There is no need for any > > > > runtime proxy file operations to be managed by the debugfs core. > > > > > > > > This Change is reported by the debugfs_simple_attr.cocci Coccinelle > > > > semantic patch. > > > > > > I just checked here with > > > $ make coccicheck M=drivers/gpu/drm/i915/ MODE=context COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > Hello Rodrigo, > > Thank you so much for your review and feedback on the patch proposal. > > > > > > > > The part reported by the this script is the s/SIMPLE/DEBUGFS > > > but the change to the unsafe option is not. > > > > If you look at the original commit of this coccinelle file, it calls out the > > need for pairing debugfs_create_file_unsafe() as well. Please review this > > > > commitID: 5103068eaca2: ("debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage") > > +Nicolai and Julia. > > It looks like coccinelle got right the > - DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); > + DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); > > but it failed badly on > - debugfs_create_file(name, mode, parent, data, &dsa_fops) > + debugfs_create_file_unsafe(name, mode, parent, data, &dsa_fops) > > > > > Based on my review of the code, the functions debugfs_create_file() and > > debugfs_create_file_unsafe(), both internally call __debugfs_create_file(). > > However, they pass debugfs_full_proxy_file_operations and > > debugfs_open_proxy_file_operations respectively to it. The former represents the > > full proxy factory, where as the later one is lightweight open proxy > > implementation of the file operations structure. > > > > > > > > This commit message is not explaining why the unsafe is the suggested > > > or who suggested it. > > > > If you find the response above accurate, I will include these details about > > the _unsafe() function in my commit message in v2. > > > > > > > > If you remove the unsafe part feel free to resend adding: > > > > Please confirm you still believe switching to _unsafe() is not necessary. > > Based on the coccinelle commit it looks like you are right, but cocinelle > just failed to detect the case. Let's see what Nicolai and Julia respond > before we move with any patch here. Hello Nicolai and Julia, Can you please review this proposed patch and the feedback comments from Rodrigo please? Thank you, ./drv > > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > (to both patches, this and the drrs one. > > > > > > Also, it looks like you could contribute with other 2 patches: > > > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c:64:0-23: WARNING: pxp_terminate_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > > drivers/gpu/drm/i915/gvt/debugfs.c:150:0-23: WARNING: vgpu_scan_nonprivbb_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > > > Yes, these are on my list. Was waiting for a feedback on the first submission > > before I send more similar patches. > > > > Appreciate your time and the feedback. > > > > > > Regards, > > ./drv > > > > > > > > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_fbc.c | 12 ++++++------ > > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > index b5ee5ea0d010..4b481e2f908b 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > @@ -1809,10 +1809,10 @@ static int intel_fbc_debugfs_false_color_set(void *data, u64 val) > > > > return 0; > > > > } > > > > > > > > -DEFINE_SIMPLE_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > > > > - intel_fbc_debugfs_false_color_get, > > > > - intel_fbc_debugfs_false_color_set, > > > > - "%llu\n"); > > > > +DEFINE_DEBUGFS_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > > > > + intel_fbc_debugfs_false_color_get, > > > > + intel_fbc_debugfs_false_color_set, > > > > + "%llu\n"); > > > > > > > > static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > > > > struct dentry *parent) > > > > @@ -1821,8 +1821,8 @@ static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > > > > fbc, &intel_fbc_debugfs_status_fops); > > > > > > > > if (fbc->funcs->set_false_color) > > > > - debugfs_create_file("i915_fbc_false_color", 0644, parent, > > > > - fbc, &intel_fbc_debugfs_false_color_fops); > > > > + debugfs_create_file_unsafe("i915_fbc_false_color", 0644, parent, > > > > + fbc, &intel_fbc_debugfs_false_color_fops); > > > > } > > > > > > > > void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc) > > > > -- > > > > 2.34.1 > > > > > > > > > > > > > > > > >
On Tue, 3 Jan 2023, Deepak R Varma wrote: > On Wed, Dec 28, 2022 at 06:18:12AM -0500, Rodrigo Vivi wrote: > > On Tue, Dec 27, 2022 at 11:36:13PM +0530, Deepak R Varma wrote: > > > On Tue, Dec 27, 2022 at 12:13:56PM -0500, Rodrigo Vivi wrote: > > > > On Tue, Dec 27, 2022 at 01:30:53PM +0530, Deepak R Varma wrote: > > > > > Using DEFINE_SIMPLE_ATTRIBUTE macro with the debugfs_create_file() > > > > > function adds the overhead of introducing a proxy file operation > > > > > functions to wrap the original read/write inside file removal protection > > > > > functions. This adds significant overhead in terms of introducing and > > > > > managing the proxy factory file operations structure and function > > > > > wrapping at runtime. > > > > > As a replacement, a combination of DEFINE_DEBUGFS_ATTRIBUTE macro paired > > > > > with debugfs_create_file_unsafe() is suggested to be used instead. The > > > > > DEFINE_DEBUGFS_ATTRIBUTE utilises debugfs_file_get() and > > > > > debugfs_file_put() wrappers to protect the original read and write > > > > > function calls for the debug attributes. There is no need for any > > > > > runtime proxy file operations to be managed by the debugfs core. > > > > > > > > > > This Change is reported by the debugfs_simple_attr.cocci Coccinelle > > > > > semantic patch. > > > > > > > > I just checked here with > > > > $ make coccicheck M=drivers/gpu/drm/i915/ MODE=context COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > > > Hello Rodrigo, > > > Thank you so much for your review and feedback on the patch proposal. > > > > > > > > > > > The part reported by the this script is the s/SIMPLE/DEBUGFS > > > > but the change to the unsafe option is not. > > > > > > If you look at the original commit of this coccinelle file, it calls out the > > > need for pairing debugfs_create_file_unsafe() as well. Please review this > > > > > > commitID: 5103068eaca2: ("debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage") > > > > +Nicolai and Julia. > > > > It looks like coccinelle got right the > > - DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); > > + DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); > > > > but it failed badly on > > - debugfs_create_file(name, mode, parent, data, &dsa_fops) > > + debugfs_create_file_unsafe(name, mode, parent, data, &dsa_fops) > > > > > > > > Based on my review of the code, the functions debugfs_create_file() and > > > debugfs_create_file_unsafe(), both internally call __debugfs_create_file(). > > > However, they pass debugfs_full_proxy_file_operations and > > > debugfs_open_proxy_file_operations respectively to it. The former represents the > > > full proxy factory, where as the later one is lightweight open proxy > > > implementation of the file operations structure. > > > > > > > > > > > This commit message is not explaining why the unsafe is the suggested > > > > or who suggested it. > > > > > > If you find the response above accurate, I will include these details about > > > the _unsafe() function in my commit message in v2. > > > > > > > > > > > If you remove the unsafe part feel free to resend adding: > > > > > > Please confirm you still believe switching to _unsafe() is not necessary. > > > > Based on the coccinelle commit it looks like you are right, but cocinelle > > just failed to detect the case. Let's see what Nicolai and Julia respond > > before we move with any patch here. > > Hello Nicolai and Julia, > Can you please review this proposed patch and the feedback comments from Rodrigo > please? I'm not an expert on this issue. If the semantic patch needs to change in some way, I would be happy to take any improvements. julia > > Thank you, > ./drv > > > > > > > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > (to both patches, this and the drrs one. > > > > > > > > Also, it looks like you could contribute with other 2 patches: > > > > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c:64:0-23: WARNING: pxp_terminate_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > > > drivers/gpu/drm/i915/gvt/debugfs.c:150:0-23: WARNING: vgpu_scan_nonprivbb_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > > > > > Yes, these are on my list. Was waiting for a feedback on the first submission > > > before I send more similar patches. > > > > > > Appreciate your time and the feedback. > > > > > > > > > Regards, > > > ./drv > > > > > > > > > > > > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_fbc.c | 12 ++++++------ > > > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > index b5ee5ea0d010..4b481e2f908b 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > @@ -1809,10 +1809,10 @@ static int intel_fbc_debugfs_false_color_set(void *data, u64 val) > > > > > return 0; > > > > > } > > > > > > > > > > -DEFINE_SIMPLE_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > > > > > - intel_fbc_debugfs_false_color_get, > > > > > - intel_fbc_debugfs_false_color_set, > > > > > - "%llu\n"); > > > > > +DEFINE_DEBUGFS_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > > > > > + intel_fbc_debugfs_false_color_get, > > > > > + intel_fbc_debugfs_false_color_set, > > > > > + "%llu\n"); > > > > > > > > > > static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > > > > > struct dentry *parent) > > > > > @@ -1821,8 +1821,8 @@ static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > > > > > fbc, &intel_fbc_debugfs_status_fops); > > > > > > > > > > if (fbc->funcs->set_false_color) > > > > > - debugfs_create_file("i915_fbc_false_color", 0644, parent, > > > > > - fbc, &intel_fbc_debugfs_false_color_fops); > > > > > + debugfs_create_file_unsafe("i915_fbc_false_color", 0644, parent, > > > > > + fbc, &intel_fbc_debugfs_false_color_fops); > > > > > } > > > > > > > > > > void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc) > > > > > -- > > > > > 2.34.1 > > > > > > > > > > > > > > > > > > > > > > > > > >
On Wed, Jan 04, 2023 at 06:51:37PM +0100, Julia Lawall wrote: > > > On Tue, 3 Jan 2023, Deepak R Varma wrote: > > > On Wed, Dec 28, 2022 at 06:18:12AM -0500, Rodrigo Vivi wrote: > > > On Tue, Dec 27, 2022 at 11:36:13PM +0530, Deepak R Varma wrote: > > > > On Tue, Dec 27, 2022 at 12:13:56PM -0500, Rodrigo Vivi wrote: > > > > > On Tue, Dec 27, 2022 at 01:30:53PM +0530, Deepak R Varma wrote: > > > > > > Using DEFINE_SIMPLE_ATTRIBUTE macro with the debugfs_create_file() > > > > > > function adds the overhead of introducing a proxy file operation > > > > > > functions to wrap the original read/write inside file removal protection > > > > > > functions. This adds significant overhead in terms of introducing and > > > > > > managing the proxy factory file operations structure and function > > > > > > wrapping at runtime. > > > > > > As a replacement, a combination of DEFINE_DEBUGFS_ATTRIBUTE macro paired > > > > > > with debugfs_create_file_unsafe() is suggested to be used instead. The > > > > > > DEFINE_DEBUGFS_ATTRIBUTE utilises debugfs_file_get() and > > > > > > debugfs_file_put() wrappers to protect the original read and write > > > > > > function calls for the debug attributes. There is no need for any > > > > > > runtime proxy file operations to be managed by the debugfs core. > > > > > > > > > > > > This Change is reported by the debugfs_simple_attr.cocci Coccinelle > > > > > > semantic patch. > > > > > > > > > > I just checked here with > > > > > $ make coccicheck M=drivers/gpu/drm/i915/ MODE=context COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > > > > > Hello Rodrigo, > > > > Thank you so much for your review and feedback on the patch proposal. > > > > > > > > > > > > > > The part reported by the this script is the s/SIMPLE/DEBUGFS > > > > > but the change to the unsafe option is not. > > > > > > > > If you look at the original commit of this coccinelle file, it calls out the > > > > need for pairing debugfs_create_file_unsafe() as well. Please review this > > > > > > > > commitID: 5103068eaca2: ("debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage") > > > > > > +Nicolai and Julia. > > > > > > It looks like coccinelle got right the > > > - DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); > > > + DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); > > > > > > but it failed badly on > > > - debugfs_create_file(name, mode, parent, data, &dsa_fops) > > > + debugfs_create_file_unsafe(name, mode, parent, data, &dsa_fops) > > > > > > > > > > > Based on my review of the code, the functions debugfs_create_file() and > > > > debugfs_create_file_unsafe(), both internally call __debugfs_create_file(). > > > > However, they pass debugfs_full_proxy_file_operations and > > > > debugfs_open_proxy_file_operations respectively to it. The former represents the > > > > full proxy factory, where as the later one is lightweight open proxy > > > > implementation of the file operations structure. > > > > > > > > > > > > > > This commit message is not explaining why the unsafe is the suggested > > > > > or who suggested it. > > > > > > > > If you find the response above accurate, I will include these details about > > > > the _unsafe() function in my commit message in v2. > > > > > > > > > > > > > > If you remove the unsafe part feel free to resend adding: > > > > > > > > Please confirm you still believe switching to _unsafe() is not necessary. > > > > > > Based on the coccinelle commit it looks like you are right, but cocinelle > > > just failed to detect the case. Let's see what Nicolai and Julia respond > > > before we move with any patch here. > > > > Hello Nicolai and Julia, > > Can you please review this proposed patch and the feedback comments from Rodrigo > > please? > > I'm not an expert on this issue. If the semantic patch needs to change in > some way, I would be happy to take any improvements. Hi Julia, thanks for helping here. So, my question is why this make coccicheck M=drivers/gpu/drm/i915/ MODE=context COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci didn't catch this chunck: - debugfs_create_file("i915_fbc_false_color", 0644, parent, - fbc, &intel_fbc_debugfs_false_color_fops); + debugfs_create_file_unsafe("i915_fbc_false_color", 0644, parent, + fbc, &intel_fbc_debugfs_false_color_fops); When I run it it only catches and replaces this: - DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); + DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); But looking to the .cocci script or at least to its description, I believe it should catch both cases. But if it is not a bug in the cocci script, then I'd like to hear from Nicolai why. And have this documented in the script. Thanks, Rodrigo. > > julia > > > > > > Thank you, > > ./drv > > > > > > > > > > > > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > (to both patches, this and the drrs one. > > > > > > > > > > Also, it looks like you could contribute with other 2 patches: > > > > > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c:64:0-23: WARNING: pxp_terminate_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > > > > drivers/gpu/drm/i915/gvt/debugfs.c:150:0-23: WARNING: vgpu_scan_nonprivbb_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > > > > > > > Yes, these are on my list. Was waiting for a feedback on the first submission > > > > before I send more similar patches. > > > > > > > > Appreciate your time and the feedback. > > > > > > > > > > > > Regards, > > > > ./drv > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/display/intel_fbc.c | 12 ++++++------ > > > > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > > index b5ee5ea0d010..4b481e2f908b 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > > @@ -1809,10 +1809,10 @@ static int intel_fbc_debugfs_false_color_set(void *data, u64 val) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > -DEFINE_SIMPLE_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > > > > > > - intel_fbc_debugfs_false_color_get, > > > > > > - intel_fbc_debugfs_false_color_set, > > > > > > - "%llu\n"); > > > > > > +DEFINE_DEBUGFS_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > > > > > > + intel_fbc_debugfs_false_color_get, > > > > > > + intel_fbc_debugfs_false_color_set, > > > > > > + "%llu\n"); > > > > > > > > > > > > static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > > > > > > struct dentry *parent) > > > > > > @@ -1821,8 +1821,8 @@ static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > > > > > > fbc, &intel_fbc_debugfs_status_fops); > > > > > > > > > > > > if (fbc->funcs->set_false_color) > > > > > > - debugfs_create_file("i915_fbc_false_color", 0644, parent, > > > > > > - fbc, &intel_fbc_debugfs_false_color_fops); > > > > > > + debugfs_create_file_unsafe("i915_fbc_false_color", 0644, parent, > > > > > > + fbc, &intel_fbc_debugfs_false_color_fops); > > > > > > } > > > > > > > > > > > > void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc) > > > > > > -- > > > > > > 2.34.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
> Hi Julia, thanks for helping here. > > So, my question is why this > > make coccicheck M=drivers/gpu/drm/i915/ MODE=context COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > didn't catch this chunck: > > - debugfs_create_file("i915_fbc_false_color", 0644, parent, > - fbc, &intel_fbc_debugfs_false_color_fops); > + debugfs_create_file_unsafe("i915_fbc_false_color", 0644, parent, > + fbc, &intel_fbc_debugfs_false_color_fops); > > When I run it it only catches and replaces this: > > - DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); > + DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); There is something strange in your question. You have MODE=context but you show the output for MODE=patch. The rule dcf matches a call to debugfs_create_file, and the context rule matching DEFINE_SIMPLE_ATTRIBUTE is only activated if dcf succeeds. So when the context rule gives a report, there is always a corresponding call to debugfs_create_file in the same file, it is just not highlighted. So the request is that it should be highlighted as well? julia > > But looking to the .cocci script or at least to its description, > I believe it should catch both cases. > > But if it is not a bug in the cocci script, then I'd like to hear > from Nicolai why. And have this documented in the script. > > Thanks, > Rodrigo. > > > > > julia > > > > > > > > > > Thank you, > > > ./drv > > > > > > > > > > > > > > > > > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > (to both patches, this and the drrs one. > > > > > > > > > > > > Also, it looks like you could contribute with other 2 patches: > > > > > > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c:64:0-23: WARNING: pxp_terminate_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > > > > > drivers/gpu/drm/i915/gvt/debugfs.c:150:0-23: WARNING: vgpu_scan_nonprivbb_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE > > > > > > > > > > Yes, these are on my list. Was waiting for a feedback on the first submission > > > > > before I send more similar patches. > > > > > > > > > > Appreciate your time and the feedback. > > > > > > > > > > > > > > > Regards, > > > > > ./drv > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/display/intel_fbc.c | 12 ++++++------ > > > > > > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > > > index b5ee5ea0d010..4b481e2f908b 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > > > @@ -1809,10 +1809,10 @@ static int intel_fbc_debugfs_false_color_set(void *data, u64 val) > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > -DEFINE_SIMPLE_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > > > > > > > - intel_fbc_debugfs_false_color_get, > > > > > > > - intel_fbc_debugfs_false_color_set, > > > > > > > - "%llu\n"); > > > > > > > +DEFINE_DEBUGFS_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, > > > > > > > + intel_fbc_debugfs_false_color_get, > > > > > > > + intel_fbc_debugfs_false_color_set, > > > > > > > + "%llu\n"); > > > > > > > > > > > > > > static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > > > > > > > struct dentry *parent) > > > > > > > @@ -1821,8 +1821,8 @@ static void intel_fbc_debugfs_add(struct intel_fbc *fbc, > > > > > > > fbc, &intel_fbc_debugfs_status_fops); > > > > > > > > > > > > > > if (fbc->funcs->set_false_color) > > > > > > > - debugfs_create_file("i915_fbc_false_color", 0644, parent, > > > > > > > - fbc, &intel_fbc_debugfs_false_color_fops); > > > > > > > + debugfs_create_file_unsafe("i915_fbc_false_color", 0644, parent, > > > > > > > + fbc, &intel_fbc_debugfs_false_color_fops); > > > > > > > } > > > > > > > > > > > > > > void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc) > > > > > > > -- > > > > > > > 2.34.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Thu, Jan 05, 2023 at 09:13:35AM +0100, Julia Lawall wrote: > > Hi Julia, thanks for helping here. > > > > So, my question is why this > > > > make coccicheck M=drivers/gpu/drm/i915/ MODE=context COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > didn't catch this chunck: > > > > - debugfs_create_file("i915_fbc_false_color", 0644, parent, > > - fbc, &intel_fbc_debugfs_false_color_fops); > > + debugfs_create_file_unsafe("i915_fbc_false_color", 0644, parent, > > + fbc, &intel_fbc_debugfs_false_color_fops); > > > > When I run it it only catches and replaces this: > > > > - DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); > > + DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); > > There is something strange in your question. You have MODE=context but > you show the output for MODE=patch. The rule dcf matches a call to > debugfs_create_file, and the context rule matching DEFINE_SIMPLE_ATTRIBUTE > is only activated if dcf succeeds. So when the context rule gives a > report, there is always a corresponding call to debugfs_create_file in the > same file, it is just not highlighted. So the request is that it should > be highlighted as well? Hello Rodrigo, Not trying to speak for you, but I think Julia's comment appears to be the correct interpretation of your observation. Would you mind confirming/clarifying and suggest next steps for this proposal? Thank you, ./drv > > julia >
On Sun, Jan 08, 2023 at 01:33:41AM +0530, Deepak R Varma wrote: > On Thu, Jan 05, 2023 at 09:13:35AM +0100, Julia Lawall wrote: > > > Hi Julia, thanks for helping here. > > > > > > So, my question is why this > > > > > > make coccicheck M=drivers/gpu/drm/i915/ MODE=context COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > > > didn't catch this chunck: > > > > > > - debugfs_create_file("i915_fbc_false_color", 0644, parent, > > > - fbc, &intel_fbc_debugfs_false_color_fops); > > > + debugfs_create_file_unsafe("i915_fbc_false_color", 0644, parent, > > > + fbc, &intel_fbc_debugfs_false_color_fops); > > > > > > When I run it it only catches and replaces this: > > > > > > - DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); > > > + DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); > > > > There is something strange in your question. You have MODE=context but > > you show the output for MODE=patch. The rule dcf matches a call to > > debugfs_create_file, and the context rule matching DEFINE_SIMPLE_ATTRIBUTE > > is only activated if dcf succeeds. So when the context rule gives a > > report, there is always a corresponding call to debugfs_create_file in the > > same file, it is just not highlighted. So the request is that it should > > be highlighted as well? > > Hello Rodrigo, > Not trying to speak for you, but I think Julia's comment appears to be the > correct interpretation of your observation. Would you mind confirming/clarifying > and suggest next steps for this proposal? doh! newby coccinelle user detected! My bad, sorry! make coccicheck M=drivers/gpu/drm/i915/ MODE=patch COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci do shows everything. So, could you please mention this line in the commit message so we don't forget that? Also could you please provide patches for the other cases? 1 patch for each file is desirable in this case since it touches different areas. > > Thank you, > ./drv > > > > > julia > > > >
On Mon, Jan 09, 2023 at 02:06:13PM -0500, Rodrigo Vivi wrote: > On Sun, Jan 08, 2023 at 01:33:41AM +0530, Deepak R Varma wrote: > > On Thu, Jan 05, 2023 at 09:13:35AM +0100, Julia Lawall wrote: > > > > Hi Julia, thanks for helping here. > > > > > > > > So, my question is why this > > > > > > > > make coccicheck M=drivers/gpu/drm/i915/ MODE=context COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > > > > > > > didn't catch this chunck: > > > > > > > > - debugfs_create_file("i915_fbc_false_color", 0644, parent, > > > > - fbc, &intel_fbc_debugfs_false_color_fops); > > > > + debugfs_create_file_unsafe("i915_fbc_false_color", 0644, parent, > > > > + fbc, &intel_fbc_debugfs_false_color_fops); > > > > > > > > When I run it it only catches and replaces this: > > > > > > > > - DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); > > > > + DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt); > > > > > > There is something strange in your question. You have MODE=context but > > > you show the output for MODE=patch. The rule dcf matches a call to > > > debugfs_create_file, and the context rule matching DEFINE_SIMPLE_ATTRIBUTE > > > is only activated if dcf succeeds. So when the context rule gives a > > > report, there is always a corresponding call to debugfs_create_file in the > > > same file, it is just not highlighted. So the request is that it should > > > be highlighted as well? > > > > Hello Rodrigo, > > Not trying to speak for you, but I think Julia's comment appears to be the > > correct interpretation of your observation. Would you mind confirming/clarifying > > and suggest next steps for this proposal? > > doh! newby coccinelle user detected! My bad, sorry! > > make coccicheck M=drivers/gpu/drm/i915/ MODE=patch COCCI=./scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci > > do shows everything. > > So, could you please mention this line in the commit message so we don't forget that? Sure, I will do that. > > Also could you please provide patches for the other cases? > 1 patch for each file is desirable in this case since it touches different areas. Sounds good. I will separate patches one per file and send in a series as appropriate. Thank you, ./drv > > > > > Thank you, > > ./drv > > > > > > > > julia > > > > > > >
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c index b5ee5ea0d010..4b481e2f908b 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -1809,10 +1809,10 @@ static int intel_fbc_debugfs_false_color_set(void *data, u64 val) return 0; } -DEFINE_SIMPLE_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, - intel_fbc_debugfs_false_color_get, - intel_fbc_debugfs_false_color_set, - "%llu\n"); +DEFINE_DEBUGFS_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, + intel_fbc_debugfs_false_color_get, + intel_fbc_debugfs_false_color_set, + "%llu\n"); static void intel_fbc_debugfs_add(struct intel_fbc *fbc, struct dentry *parent) @@ -1821,8 +1821,8 @@ static void intel_fbc_debugfs_add(struct intel_fbc *fbc, fbc, &intel_fbc_debugfs_status_fops); if (fbc->funcs->set_false_color) - debugfs_create_file("i915_fbc_false_color", 0644, parent, - fbc, &intel_fbc_debugfs_false_color_fops); + debugfs_create_file_unsafe("i915_fbc_false_color", 0644, parent, + fbc, &intel_fbc_debugfs_false_color_fops); } void intel_fbc_crtc_debugfs_add(struct intel_crtc *crtc)