Received: by 2002:a05:6358:16cc:b0:ea:6187:17c9 with SMTP id r12csp3517868rwl; Tue, 27 Dec 2022 10:18:12 -0800 (PST) X-Google-Smtp-Source: AMrXdXtnmBPInOtISPT4yS5wO2u/07Y2c67LMcheGeDQ6l5QjV0Xr5urXXQSR9ytXh1yHnTOZuMS X-Received: by 2002:a62:b617:0:b0:577:b52:4ec2 with SMTP id j23-20020a62b617000000b005770b524ec2mr22208105pff.29.1672165091810; Tue, 27 Dec 2022 10:18:11 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1672165091; cv=none; d=google.com; s=arc-20160816; b=JaVxGx2GYc1Hlw6AllGY2m0dSrLnmBW8uY/DOQ5PFBGV+VdtyG4SAsQWB58CD/e/bb h1hOxqc3dOLvMoN5dv2jJ/qSMfHqeqGXcZJnCGdUwa9CLxQymnR+BJ+/Ks2/uLxqBsjp L43pOfMp/IJp0o9MtmR1UakkmyFyMJsk034ZHypej3CdjzZrfJGw9PhqNjuYyquz+N3y wB6msw5klIykln/zVtrPixG4AVyngrJghUUJdleD4ZXkkZKuxd4deR5Elf40Sh3bh1v0 XG8m+bCZzwCOnNTR6v51YkWM+Bgwz/r709rDamWi7/8vybNF7zwwWyzQLMSDfn+0brZ5 cgxA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=MHwR92PUsUti3DE4Ni4XshEWXo/7aJNr+bF5POVVOYw=; b=dw6tBaqOi+SHS3usygNcWm38TUndIy4Lw7UPH0XvGthuM304tSL3A/oLwsJJ2LjRKD d04BVF+TVYItQy3NiKKHCLZsFHyo6mTGIJbOMTLcc19D8XElty37kP9VEODij1Gz6R9r 3hK4YFTePFAFDpacPRI5cu/ak3CrwnRb8xnG+QBayTIhYWm0hiBrJ6nlmpfKXITykq4r MiiKz84SqiknC0uxd6nymlsU7oL3bWjTLfJuUz5YeQwbwSqAXYY6HIQbN0SVyUEkpDjk O4b727nPIKshbfo7Geq7R3BtHvOIbqL8Y26kLB9gcZuJHFvwtgwBaJSLSapdlJEXmGrh JiQA== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=B4bGLAbP; 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 Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id j67-20020a625546000000b00577c17000e1si13608342pfb.280.2022.12.27.10.18.02; Tue, 27 Dec 2022 10:18:11 -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=B4bGLAbP; 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 S229632AbiL0SGf (ORCPT + 67 others); Tue, 27 Dec 2022 13:06:35 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54822 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229488AbiL0SGd (ORCPT ); Tue, 27 Dec 2022 13:06:33 -0500 Received: from msg-2.mailo.com (msg-2.mailo.com [213.182.54.12]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 243E5E41 for ; Tue, 27 Dec 2022 10:06:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mailo.com; s=mailo; t=1672164379; bh=zxrGV6ifTSulE8N2ayWJie6beCLzXxvXheTGblxTLLs=; h=X-EA-Auth:Date:From:To:Cc:Subject:Message-ID:References: MIME-Version:Content-Type:In-Reply-To; b=B4bGLAbPTHDpDzDsq2ZoF4DeF6HGS1ca9ihDIZcfWW+tCnThRA0v7BRSHhEOZGxWz kRuCYX2l1nRFP8ThXknOG+g1kL0LmPcriI5WSZMK3Y0oNiqLrqT+PkRyuiaVmwsY7E PKHjWJEx41k6jwVe1iB/Egr4DV1Ie9sqPh4oi990= Received: by b-1.in.mailobj.net [192.168.90.11] with ESMTP via ip-206.mailobj.net [213.182.55.206] Tue, 27 Dec 2022 19:06:19 +0100 (CET) X-EA-Auth: hdHTa+YVHebgj2OAtim6AI745HMESDa2jC0+qkCFj+4bzh86fKH9DBcOPTM07YbedQtiHDmjnZF3xVa34huFJF/71vlwbbK6 Date: Tue, 27 Dec 2022 23:36:13 +0530 From: Deepak R Varma To: Rodrigo Vivi Cc: Jani Nikula , Joonas Lahtinen , Tvrtko Ursulin , David Airlie , Daniel Vetter , intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Saurabh Singh Sengar , Praveen Kumar , Deepak R Varma Subject: Re: [PATCH] drm/i915/fbc: Avoid full proxy f_ops for FBC debug attributes Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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: X-Mailing-List: linux-kernel@vger.kernel.org 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 > (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 > > --- > > 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 > > > > > >