2023-07-26 21:09:20

by Sean Christopherson

[permalink] [raw]
Subject: [PATCH v2] Revert "debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage"

Remove coccinelle's recommendation to use DEFINE_DEBUGFS_ATTRIBUTE()
instead of DEFINE_SIMPLE_ATTRIBUTE(). Regardless of whether or not the
"significant overhead" incurred by debugfs_create_file() is actually
meaningful, warnings from the script have led to a rash of low-quality
patches that have sowed confusion and consumed maintainer time for little
to no benefit. There have been no less than four attempts to "fix" KVM,
and a quick search on lore shows that KVM is not alone.

This reverts commit 5103068eaca290f890a30aae70085fac44cecaf6.

Link: https://lore.kernel.org/all/[email protected]
Link: https://lore.kernel.org/all/[email protected]
Link: https://lkml.kernel.org/r/20230706072954.4881-1-duminjie%40vivo.com
Link: https://lore.kernel.org/all/[email protected]
Link: https://lore.kernel.org/all/Y2ENJJ1YiSg5oHiy@orome
Link: https://lore.kernel.org/all/[email protected]
Suggested-by: Paolo Bonzini <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Sean Christopherson <[email protected]>
---

v2: Fix a copy+paste goof in the changelog, add Greg's Ack.

.../api/debugfs/debugfs_simple_attr.cocci | 68 -------------------
1 file changed, 68 deletions(-)
delete mode 100644 scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci

diff --git a/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci b/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
deleted file mode 100644
index 7c312310547c..000000000000
--- a/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
+++ /dev/null
@@ -1,68 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/// Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE
-/// for debugfs files.
-///
-//# Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
-//# imposes some significant overhead as compared to
-//# DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().
-//
-// Copyright (C): 2016 Nicolai Stange
-// Options: --no-includes
-//
-
-virtual context
-virtual patch
-virtual org
-virtual report
-
-@dsa@
-declarer name DEFINE_SIMPLE_ATTRIBUTE;
-identifier dsa_fops;
-expression dsa_get, dsa_set, dsa_fmt;
-position p;
-@@
-DEFINE_SIMPLE_ATTRIBUTE@p(dsa_fops, dsa_get, dsa_set, dsa_fmt);
-
-@dcf@
-expression name, mode, parent, data;
-identifier dsa.dsa_fops;
-@@
-debugfs_create_file(name, mode, parent, data, &dsa_fops)
-
-
-@context_dsa depends on context && dcf@
-declarer name DEFINE_DEBUGFS_ATTRIBUTE;
-identifier dsa.dsa_fops;
-expression dsa.dsa_get, dsa.dsa_set, dsa.dsa_fmt;
-@@
-* DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
-
-
-@patch_dcf depends on patch expression@
-expression name, mode, parent, data;
-identifier dsa.dsa_fops;
-@@
-- debugfs_create_file(name, mode, parent, data, &dsa_fops)
-+ debugfs_create_file_unsafe(name, mode, parent, data, &dsa_fops)
-
-@patch_dsa depends on patch_dcf && patch@
-identifier dsa.dsa_fops;
-expression dsa.dsa_get, dsa.dsa_set, dsa.dsa_fmt;
-@@
-- DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
-+ DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
-
-
-@script:python depends on org && dcf@
-fops << dsa.dsa_fops;
-p << dsa.p;
-@@
-msg="%s should be defined with DEFINE_DEBUGFS_ATTRIBUTE" % (fops)
-coccilib.org.print_todo(p[0], msg)
-
-@script:python depends on report && dcf@
-fops << dsa.dsa_fops;
-p << dsa.p;
-@@
-msg="WARNING: %s should be defined with DEFINE_DEBUGFS_ATTRIBUTE" % (fops)
-coccilib.report.print_report(p[0], msg)

base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
--
2.41.0.487.g6d72f3e995-goog



2023-07-26 21:55:02

by Julia Lawall

[permalink] [raw]
Subject: Re: [PATCH v2] Revert "debugfs, coccinelle: check for obsolete DEFINE_SIMPLE_ATTRIBUTE() usage"



On Wed, 26 Jul 2023, Sean Christopherson wrote:

> Remove coccinelle's recommendation to use DEFINE_DEBUGFS_ATTRIBUTE()
> instead of DEFINE_SIMPLE_ATTRIBUTE(). Regardless of whether or not the
> "significant overhead" incurred by debugfs_create_file() is actually
> meaningful, warnings from the script have led to a rash of low-quality
> patches that have sowed confusion and consumed maintainer time for little
> to no benefit. There have been no less than four attempts to "fix" KVM,
> and a quick search on lore shows that KVM is not alone.
>
> This reverts commit 5103068eaca290f890a30aae70085fac44cecaf6.

Applied.

>
> Link: https://lore.kernel.org/all/[email protected]
> Link: https://lore.kernel.org/all/[email protected]
> Link: https://lkml.kernel.org/r/20230706072954.4881-1-duminjie%40vivo.com
> Link: https://lore.kernel.org/all/[email protected]
> Link: https://lore.kernel.org/all/Y2ENJJ1YiSg5oHiy@orome
> Link: https://lore.kernel.org/all/[email protected]
> Suggested-by: Paolo Bonzini <[email protected]>
> Acked-by: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
>
> v2: Fix a copy+paste goof in the changelog, add Greg's Ack.
>
> .../api/debugfs/debugfs_simple_attr.cocci | 68 -------------------
> 1 file changed, 68 deletions(-)
> delete mode 100644 scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
>
> diff --git a/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci b/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
> deleted file mode 100644
> index 7c312310547c..000000000000
> --- a/scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci
> +++ /dev/null
> @@ -1,68 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/// Use DEFINE_DEBUGFS_ATTRIBUTE rather than DEFINE_SIMPLE_ATTRIBUTE
> -/// for debugfs files.
> -///
> -//# Rationale: DEFINE_SIMPLE_ATTRIBUTE + debugfs_create_file()
> -//# imposes some significant overhead as compared to
> -//# DEFINE_DEBUGFS_ATTRIBUTE + debugfs_create_file_unsafe().
> -//
> -// Copyright (C): 2016 Nicolai Stange
> -// Options: --no-includes
> -//
> -
> -virtual context
> -virtual patch
> -virtual org
> -virtual report
> -
> -@dsa@
> -declarer name DEFINE_SIMPLE_ATTRIBUTE;
> -identifier dsa_fops;
> -expression dsa_get, dsa_set, dsa_fmt;
> -position p;
> -@@
> -DEFINE_SIMPLE_ATTRIBUTE@p(dsa_fops, dsa_get, dsa_set, dsa_fmt);
> -
> -@dcf@
> -expression name, mode, parent, data;
> -identifier dsa.dsa_fops;
> -@@
> -debugfs_create_file(name, mode, parent, data, &dsa_fops)
> -
> -
> -@context_dsa depends on context && dcf@
> -declarer name DEFINE_DEBUGFS_ATTRIBUTE;
> -identifier dsa.dsa_fops;
> -expression dsa.dsa_get, dsa.dsa_set, dsa.dsa_fmt;
> -@@
> -* DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
> -
> -
> -@patch_dcf depends on patch expression@
> -expression name, mode, parent, data;
> -identifier dsa.dsa_fops;
> -@@
> -- debugfs_create_file(name, mode, parent, data, &dsa_fops)
> -+ debugfs_create_file_unsafe(name, mode, parent, data, &dsa_fops)
> -
> -@patch_dsa depends on patch_dcf && patch@
> -identifier dsa.dsa_fops;
> -expression dsa.dsa_get, dsa.dsa_set, dsa.dsa_fmt;
> -@@
> -- DEFINE_SIMPLE_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
> -+ DEFINE_DEBUGFS_ATTRIBUTE(dsa_fops, dsa_get, dsa_set, dsa_fmt);
> -
> -
> -@script:python depends on org && dcf@
> -fops << dsa.dsa_fops;
> -p << dsa.p;
> -@@
> -msg="%s should be defined with DEFINE_DEBUGFS_ATTRIBUTE" % (fops)
> -coccilib.org.print_todo(p[0], msg)
> -
> -@script:python depends on report && dcf@
> -fops << dsa.dsa_fops;
> -p << dsa.p;
> -@@
> -msg="WARNING: %s should be defined with DEFINE_DEBUGFS_ATTRIBUTE" % (fops)
> -coccilib.report.print_report(p[0], msg)
>
> base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
> --
> 2.41.0.487.g6d72f3e995-goog
>
>