thr3ads.net - Nouveau - [Nouveau] [PATCH 2/3] lib/string_helpers: Add helpers for enable[d]/disable[d] [Jan 2022]

If this information is useful, please help other people find it:
Share via:

Lucas De Marchi

2022-Jan-19 07:24 UTC

[Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers

Add some helpers under lib/string_helpers.h so they can be used
throughout the kernel. When I started doing this there were 2 other
previous attempts I know of, not counting the iterations each of them
had:

1) https://lore.kernel.org/all/20191023131308.9420-1-jani.nikula at intel.com/
2) https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevchenko at
linux.intel.com/#t

Going through the comments I tried to find some common ground and
justification for what is in here, addressing some of the concerns
raised.

a. This version should be a drop-in replacement for what is currently in
   the tree, with no change in behavior or binary size. For binary
   size what I checked wat that the linked objects in the end have the
   same size (gcc 11). From comments in the previous attempts this seems
   also the case for earlier compiler versions

b. I didn't change the function name to choice_* as suggested by Andrew
   Morton in 20191023155619.43e0013f0c8c673a5c508c1e at linux-foundation.org
   because other people argumented in favor of shorter names for these
   simple helpers - if they are long and people simply not use due to
   that, we failed

c. Use string_helper.h for these helpers - pulling string.h in the
   compilations units was one of the concerns and I think re-using this
   already existing header is better than creating a new string-choice.h

d. This doesn't bring onoff() helper as there are some places in the
   kernel with onoff as variable - another name is probably needed for
   this function in order not to shadow the variable, or those variables
   could be renamed.  Or if people wanting  <someprefix>
   try to find a short one

e. One alternative to all of this suggested by Christian K?nig
   (43456ba7-c372-84cc-4949-dcb817188e21 at amd.com) would be to add a
   printk format. But besides the comment, he also seemed to like
   the common function. This brought the argument from others that the
   simple yesno()/enabledisable() already used in the code is easier to
   remember and use than e.g. %py[DOY]

Last patch also has some additional conversion of open coded cases. I
preferred starting with drm/ since this is "closer to home".

I hope this is a good summary of the previous attempts and a way we can
move forward.

Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my
proposal is to take first 2 patches either through mm tree or maybe
vsprintf. Last patch can be taken later through drm.

thanks
Lucas De Marchi

Cc: Alex Deucher <alexander.deucher at amd.com>
Cc: Andrew Morton <akpm at linux-foundation.org>
Cc: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
Cc: Andy Shevchenko <andy.shevchenko at gmail.com>
Cc: Ben Skeggs <bskeggs at redhat.com>
Cc: Christian K?nig <christian.koenig at amd.com>
Cc: Chris Wilson <chris at chris-wilson.co.uk>
Cc: Daniel Vetter <daniel at ffwll.ch>
Cc: David Airlie <airlied at linux.ie>
Cc: David S. Miller <davem at davemloft.net>
Cc: Emma Anholt <emma at anholt.net>
Cc: Eryk Brol <eryk.brol at amd.com>
Cc: Francis Laniel <laniel_francis at privacyrequired.com>
Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
Cc: Harry Wentland <harry.wentland at amd.com>
Cc: Jakub Kicinski <kuba at kernel.org>
Cc: Jani Nikula <jani.nikula at linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
Cc: Julia Lawall <julia.lawall at lip6.fr>
Cc: Kentaro Takeda <takedakn at nttdata.co.jp>
Cc: Leo Li <sunpeng.li at amd.com>
Cc: Mikita Lipski <mikita.lipski at amd.com>
Cc: Petr Mladek <pmladek at suse.com>
Cc: Rahul Lakkireddy <rahul.lakkireddy at chelsio.com>
Cc: Raju Rangoju <rajur at chelsio.com>
Cc: Rasmus Villemoes <linux at rasmusvillemoes.dk>
Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
Cc: Sakari Ailus <sakari.ailus at linux.intel.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
Cc: Steven Rostedt <rostedt at goodmis.org>
Cc: Vishal Kulkarni <vishal at chelsio.com>

Lucas De Marchi (3):
  lib/string_helpers: Consolidate yesno() implementation
  lib/string_helpers: Add helpers for enable[d]/disable[d]
  drm: Convert open yes/no strings to yesno()

 drivers/gpu/drm/amd/amdgpu/atom.c              |  3 ++-
 .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c  |  6 +-----
 drivers/gpu/drm/drm_client_modeset.c           |  3 ++-
 drivers/gpu/drm/drm_dp_helper.c                |  3 ++-
 drivers/gpu/drm/drm_gem.c                      |  3 ++-
 drivers/gpu/drm/i915/i915_utils.h              | 15 ---------------
 drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c  |  4 +++-
 drivers/gpu/drm/radeon/atom.c                  |  3 ++-
 drivers/gpu/drm/v3d/v3d_debugfs.c              | 11 ++++++-----
 drivers/gpu/drm/virtio/virtgpu_debugfs.c       |  3 ++-
 .../net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 -----------
 include/linux/string_helpers.h                 |  4 ++++
 security/tomoyo/audit.c                        |  2 +-
 security/tomoyo/common.c                       | 18 ++++--------------
 security/tomoyo/common.h                       |  1 -
 15 files changed, 31 insertions(+), 59 deletions(-)

-- 
2.34.1

Lucas De Marchi

2022-Jan-19 07:24 UTC

head link

[Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation

There are a few implementations of yesno() in the tree. Consolidate them
in include/linux/string_helpers.h.  Quite a few users of open coded
yesno() could later be converted to the new function:

$ git grep '?\s*"yes"\s*' | wc -l
286
$ git grep '?\s*"no"\s*' | wc -l
20

The inlined function should keep the const strings local to each
compilation unit, the same way it's now, thus not changing the current
behavior.

Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
---
 .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c  |  6 +-----
 drivers/gpu/drm/i915/i915_utils.h              |  5 -----
 .../net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 -----------
 include/linux/string_helpers.h                 |  2 ++
 security/tomoyo/audit.c                        |  2 +-
 security/tomoyo/common.c                       | 18 ++++--------------
 security/tomoyo/common.h                       |  1 -
 7 files changed, 8 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
index 9d43ecb1f692..b59760f91bf6 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
@@ -23,6 +23,7 @@
  *
  */
 
+#include <linux/string_helpers.h>
 #include <linux/uaccess.h>
 
 #include "dc.h"
@@ -49,11 +50,6 @@ struct dmub_debugfs_trace_entry {
 	uint32_t param1;
 };
 
-static inline const char *yesno(bool v)
-{
-	return v ? "yes" : "no";
-}
-
 /* parse_write_buffer_into_params - Helper function to parse debugfs write
buffer into an array
  *
  * Function takes in attributes passed to debugfs write entry
diff --git a/drivers/gpu/drm/i915/i915_utils.h
b/drivers/gpu/drm/i915/i915_utils.h
index 7a5925072466..2a8781cc648b 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -414,11 +414,6 @@ wait_remaining_ms_from_jiffies(unsigned long
timestamp_jiffies, int to_wait_ms)
 #define MBps(x) KBps(1000 * (x))
 #define GBps(x) ((u64)1000 * MBps((x)))
 
-static inline const char *yesno(bool v)
-{
-	return v ? "yes" : "no";
-}
-
 static inline const char *onoff(bool v)
 {
 	return v ? "on" : "off";
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
index 7d49fd4edc9e..61a04d7abc1f 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
@@ -2015,17 +2015,6 @@ static const struct file_operations rss_debugfs_fops = {
 /* RSS Configuration.
  */
 
-/* Small utility function to return the strings "yes" or
"no" if the supplied
- * argument is non-zero.
- */
-static const char *yesno(int x)
-{
-	static const char *yes = "yes";
-	static const char *no = "no";
-
-	return x ? yes : no;
-}
-
 static int rss_config_show(struct seq_file *seq, void *v)
 {
 	struct adapter *adapter = seq->private;
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 4ba39e1403b2..e980dec05d31 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -102,4 +102,6 @@ char *kstrdup_quotable_file(struct file *file, gfp_t gfp);
 
 void kfree_strarray(char **array, size_t n);
 
+static inline const char *yesno(bool v) { return v ? "yes" :
"no"; }
+
 #endif
diff --git a/security/tomoyo/audit.c b/security/tomoyo/audit.c
index d79bf07e16be..1458e27361e8 100644
--- a/security/tomoyo/audit.c
+++ b/security/tomoyo/audit.c
@@ -166,7 +166,7 @@ static char *tomoyo_print_header(struct tomoyo_request_info
*r)
 		       "#%04u/%02u/%02u %02u:%02u:%02u# profile=%u mode=%s granted=%s
(global-pid=%u) task={ pid=%u ppid=%u uid=%u gid=%u euid=%u egid=%u suid=%u
sgid=%u fsuid=%u fsgid=%u }",
 		       stamp.year, stamp.month, stamp.day, stamp.hour,
 		       stamp.min, stamp.sec, r->profile, tomoyo_mode[r->mode],
-		       tomoyo_yesno(r->granted), gpid, tomoyo_sys_getpid(),
+		       yesno(r->granted), gpid, tomoyo_sys_getpid(),
 		       tomoyo_sys_getppid(),
 		       from_kuid(&init_user_ns, current_uid()),
 		       from_kgid(&init_user_ns, current_gid()),
diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
index 5c64927bf2b3..304ed0f426dd 100644
--- a/security/tomoyo/common.c
+++ b/security/tomoyo/common.c
@@ -8,6 +8,7 @@
 #include <linux/uaccess.h>
 #include <linux/slab.h>
 #include <linux/security.h>
+#include <linux/string_helpers.h>
 #include "common.h"
 
 /* String table for operation mode. */
@@ -174,16 +175,6 @@ static bool tomoyo_manage_by_non_root;
 
 /* Utility functions. */
 
-/**
- * tomoyo_yesno - Return "yes" or "no".
- *
- * @value: Bool value.
- */
-const char *tomoyo_yesno(const unsigned int value)
-{
-	return value ? "yes" : "no";
-}
-
 /**
  * tomoyo_addprintf - strncat()-like-snprintf().
  *
@@ -730,8 +721,8 @@ static void tomoyo_print_config(struct tomoyo_io_buffer
*head, const u8 config)
 {
 	tomoyo_io_printf(head, "={ mode=%s grant_log=%s reject_log=%s }\n",
 			 tomoyo_mode[config & 3],
-			 tomoyo_yesno(config & TOMOYO_CONFIG_WANT_GRANT_LOG),
-			 tomoyo_yesno(config & TOMOYO_CONFIG_WANT_REJECT_LOG));
+			 yesno(config & TOMOYO_CONFIG_WANT_GRANT_LOG),
+			 yesno(config & TOMOYO_CONFIG_WANT_REJECT_LOG));
 }
 
 /**
@@ -1354,8 +1345,7 @@ static bool tomoyo_print_condition(struct tomoyo_io_buffer
*head,
 	case 3:
 		if (cond->grant_log != TOMOYO_GRANTLOG_AUTO)
 			tomoyo_io_printf(head, " grant_log=%s",
-					 tomoyo_yesno(cond->grant_log =-						      TOMOYO_GRANTLOG_YES));
+					 yesno(cond->grant_log == TOMOYO_GRANTLOG_YES));
 		tomoyo_set_lf(head);
 		return true;
 	}
diff --git a/security/tomoyo/common.h b/security/tomoyo/common.h
index 85246b9df7ca..ca285f362705 100644
--- a/security/tomoyo/common.h
+++ b/security/tomoyo/common.h
@@ -959,7 +959,6 @@ char *tomoyo_read_token(struct tomoyo_acl_param *param);
 char *tomoyo_realpath_from_path(const struct path *path);
 char *tomoyo_realpath_nofollow(const char *pathname);
 const char *tomoyo_get_exe(void);
-const char *tomoyo_yesno(const unsigned int value);
 const struct tomoyo_path_info *tomoyo_compare_name_union
 (const struct tomoyo_path_info *name, const struct tomoyo_name_union *ptr);
 const struct tomoyo_path_info *tomoyo_get_domainname
-- 
2.34.1

Lucas De Marchi

2022-Jan-19 07:24 UTC

head link

[Nouveau] [PATCH 2/3] lib/string_helpers: Add helpers for enable[d]/disable[d]

Follow the yes/no logic and add helpers for enabled/disabled and
enable/disable - those are not so common throughout the kernel,
but they give a nice way to reuse the strings to log things as
enabled/disabled or enable/disable.

Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
---
 drivers/gpu/drm/i915/i915_utils.h | 10 ----------
 include/linux/string_helpers.h    |  2 ++
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_utils.h
b/drivers/gpu/drm/i915/i915_utils.h
index 2a8781cc648b..cbec79bae0d2 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -419,16 +419,6 @@ static inline const char *onoff(bool v)
 	return v ? "on" : "off";
 }
 
-static inline const char *enabledisable(bool v)
-{
-	return v ? "enable" : "disable";
-}
-
-static inline const char *enableddisabled(bool v)
-{
-	return v ? "enabled" : "disabled";
-}
-
 void add_taint_for_CI(struct drm_i915_private *i915, unsigned int taint);
 static inline void __add_taint_for_CI(unsigned int taint)
 {
diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index e980dec05d31..e4b82f364ee1 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -103,5 +103,7 @@ char *kstrdup_quotable_file(struct file *file, gfp_t gfp);
 void kfree_strarray(char **array, size_t n);
 
 static inline const char *yesno(bool v) { return v ? "yes" :
"no"; }
+static inline const char *enabledisable(bool v) { return v ? "enable"
: "disable"; }
+static inline const char *enableddisabled(bool v) { return v ?
"enabled" : "disabled"; }
 
 #endif
-- 
2.34.1

Lucas De Marchi

2022-Jan-19 07:24 UTC

head link

[Nouveau] [PATCH 3/3] drm: Convert open yes/no strings to yesno()

linux/string_helpers.h provides a helper to return
"yes"/"no"
strings. Replace the open coded versions with yesno(). The places were
identified with the following semantic patch:

	@@
	expression b;
	@@

	- b ? "yes" : "no"
	+ yesno(b)

Then the includes were added, so we include-what-we-use, and parenthesis
adjusted in drivers/gpu/drm/v3d/v3d_debugfs.c. After the conversion we
still see the same binary sizes:

   text    data     bss     dec     hex filename
1442171   60344     800 1503315  16f053 ./drivers/gpu/drm/radeon/radeon.ko
1442171   60344     800 1503315  16f053 ./drivers/gpu/drm/radeon/radeon.ko.old
5985991  324439   33808 6344238  60ce2e ./drivers/gpu/drm/amd/amdgpu/amdgpu.ko
5985991  324439   33808 6344238  60ce2e
./drivers/gpu/drm/amd/amdgpu/amdgpu.ko.old
 411986   10490    6176  428652   68a6c ./drivers/gpu/drm/drm.ko
 411986   10490    6176  428652   68a6c ./drivers/gpu/drm/drm.ko.old
1970292  109515    2352 2082159  1fc56f ./drivers/gpu/drm/nouveau/nouveau.ko
1970292  109515    2352 2082159  1fc56f ./drivers/gpu/drm/nouveau/nouveau.ko.old

Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
---
 drivers/gpu/drm/amd/amdgpu/atom.c             |  3 ++-
 drivers/gpu/drm/drm_client_modeset.c          |  3 ++-
 drivers/gpu/drm/drm_dp_helper.c               |  3 ++-
 drivers/gpu/drm/drm_gem.c                     |  3 ++-
 drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c |  4 +++-
 drivers/gpu/drm/radeon/atom.c                 |  3 ++-
 drivers/gpu/drm/v3d/v3d_debugfs.c             | 11 ++++++-----
 drivers/gpu/drm/virtio/virtgpu_debugfs.c      |  3 ++-
 8 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c
b/drivers/gpu/drm/amd/amdgpu/atom.c
index 6fa2229b7229..3d7d0f4cfc05 100644
--- a/drivers/gpu/drm/amd/amdgpu/atom.c
+++ b/drivers/gpu/drm/amd/amdgpu/atom.c
@@ -25,6 +25,7 @@
 #include <linux/module.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/string_helpers.h>
 #include <asm/unaligned.h>
 
 #include <drm/drm_util.h>
@@ -740,7 +741,7 @@ static void atom_op_jump(atom_exec_context *ctx, int *ptr,
int arg)
 		break;
 	}
 	if (arg != ATOM_COND_ALWAYS)
-		SDEBUG("   taken: %s\n", execute ? "yes" :
"no");
+		SDEBUG("   taken: %s\n", yesno(execute));
 	SDEBUG("   target: 0x%04X\n", target);
 	if (execute) {
 		if (ctx->last_jump == (ctx->start + target)) {
diff --git a/drivers/gpu/drm/drm_client_modeset.c
b/drivers/gpu/drm/drm_client_modeset.c
index ced09c7c06f9..3c55156a75fa 100644
--- a/drivers/gpu/drm/drm_client_modeset.c
+++ b/drivers/gpu/drm/drm_client_modeset.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/string_helpers.h>
 
 #include <drm/drm_atomic.h>
 #include <drm/drm_client.h>
@@ -241,7 +242,7 @@ static void drm_client_connectors_enabled(struct
drm_connector **connectors,
 		connector = connectors[i];
 		enabled[i] = drm_connector_enabled(connector, true);
 		DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id,
-			      connector->display_info.non_desktop ? "non desktop" :
enabled[i] ? "yes" : "no");
+			      connector->display_info.non_desktop ? "non desktop" :
yesno(enabled[i]));
 
 		any_enabled |= enabled[i];
 	}
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 4d0d1e8e51fa..f600616839f3 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -28,6 +28,7 @@
 #include <linux/module.h>
 #include <linux/sched.h>
 #include <linux/seq_file.h>
+#include <linux/string_helpers.h>
 
 #include <drm/drm_dp_helper.h>
 #include <drm/drm_print.h>
@@ -1122,7 +1123,7 @@ void drm_dp_downstream_debug(struct seq_file *m,
 	bool branch_device = drm_dp_is_branch(dpcd);
 
 	seq_printf(m, "\tDP branch device present: %s\n",
-		   branch_device ? "yes" : "no");
+		   yesno(branch_device));
 
 	if (!branch_device)
 		return;
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 4dcdec6487bb..6436876341bb 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -39,6 +39,7 @@
 #include <linux/dma-buf-map.h>
 #include <linux/mem_encrypt.h>
 #include <linux/pagevec.h>
+#include <linux/string_helpers.h>
 
 #include <drm/drm.h>
 #include <drm/drm_device.h>
@@ -1145,7 +1146,7 @@ void drm_gem_print_info(struct drm_printer *p, unsigned
int indent,
 			  drm_vma_node_start(&obj->vma_node));
 	drm_printf_indent(p, indent, "size=%zu\n", obj->size);
 	drm_printf_indent(p, indent, "imported=%s\n",
-			  obj->import_attach ? "yes" : "no");
+			  yesno(obj->import_attach));
 
 	if (obj->funcs->print_info)
 		obj->funcs->print_info(p, indent, obj);
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c
index a11637b0f6cc..d39a9c1a2a6e 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c
@@ -21,6 +21,8 @@
  *
  * Authors: Ben Skeggs
  */
+#include <linux/string_helpers.h>
+
 #include "aux.h"
 #include "pad.h"
 
@@ -94,7 +96,7 @@ void
 nvkm_i2c_aux_monitor(struct nvkm_i2c_aux *aux, bool monitor)
 {
 	struct nvkm_i2c_pad *pad = aux->pad;
-	AUX_TRACE(aux, "monitor: %s", monitor ? "yes" :
"no");
+	AUX_TRACE(aux, "monitor: %s", yesno(monitor));
 	if (monitor)
 		nvkm_i2c_pad_mode(pad, NVKM_I2C_PAD_AUX);
 	else
diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c
index f15b20da5315..77ef7d136530 100644
--- a/drivers/gpu/drm/radeon/atom.c
+++ b/drivers/gpu/drm/radeon/atom.c
@@ -25,6 +25,7 @@
 #include <linux/module.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/string_helpers.h>
 
 #include <asm/unaligned.h>
 
@@ -722,7 +723,7 @@ static void atom_op_jump(atom_exec_context *ctx, int *ptr,
int arg)
 		break;
 	}
 	if (arg != ATOM_COND_ALWAYS)
-		SDEBUG("   taken: %s\n", execute ? "yes" :
"no");
+		SDEBUG("   taken: %s\n", yesno(execute));
 	SDEBUG("   target: 0x%04X\n", target);
 	if (execute) {
 		if (ctx->last_jump == (ctx->start + target)) {
diff --git a/drivers/gpu/drm/v3d/v3d_debugfs.c
b/drivers/gpu/drm/v3d/v3d_debugfs.c
index e76b24bb8828..22c23f3a691e 100644
--- a/drivers/gpu/drm/v3d/v3d_debugfs.c
+++ b/drivers/gpu/drm/v3d/v3d_debugfs.c
@@ -6,6 +6,7 @@
 #include <linux/debugfs.h>
 #include <linux/pm_runtime.h>
 #include <linux/seq_file.h>
+#include <linux/string_helpers.h>
 
 #include <drm/drm_debugfs.h>
 
@@ -148,15 +149,15 @@ static int v3d_v3d_debugfs_ident(struct seq_file *m, void
*unused)
 		   V3D_GET_FIELD(ident3, V3D_HUB_IDENT3_IPREV),
 		   V3D_GET_FIELD(ident3, V3D_HUB_IDENT3_IPIDX));
 	seq_printf(m, "MMU:        %s\n",
-		   (ident2 & V3D_HUB_IDENT2_WITH_MMU) ? "yes" :
"no");
+		   yesno(ident2 & V3D_HUB_IDENT2_WITH_MMU));
 	seq_printf(m, "TFU:        %s\n",
-		   (ident1 & V3D_HUB_IDENT1_WITH_TFU) ? "yes" :
"no");
+		   yesno(ident1 & V3D_HUB_IDENT1_WITH_TFU));
 	seq_printf(m, "TSY:        %s\n",
-		   (ident1 & V3D_HUB_IDENT1_WITH_TSY) ? "yes" :
"no");
+		   yesno(ident1 & V3D_HUB_IDENT1_WITH_TSY));
 	seq_printf(m, "MSO:        %s\n",
-		   (ident1 & V3D_HUB_IDENT1_WITH_MSO) ? "yes" :
"no");
+		   yesno(ident1 & V3D_HUB_IDENT1_WITH_MSO));
 	seq_printf(m, "L3C:        %s (%dkb)\n",
-		   (ident1 & V3D_HUB_IDENT1_WITH_L3C) ? "yes" : "no",
+		   yesno(ident1 & V3D_HUB_IDENT1_WITH_L3C),
 		   V3D_GET_FIELD(ident2, V3D_HUB_IDENT2_L3C_NKB));
 
 	for (core = 0; core < cores; core++) {
diff --git a/drivers/gpu/drm/virtio/virtgpu_debugfs.c
b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
index b6954e2f75e6..c7f675721840 100644
--- a/drivers/gpu/drm/virtio/virtgpu_debugfs.c
+++ b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
@@ -22,6 +22,7 @@
  * OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
  * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
  */
+#include <linux/string_helpers.h>
 
 #include <drm/drm_debugfs.h>
 #include <drm/drm_file.h>
@@ -31,7 +32,7 @@
 static void virtio_gpu_add_bool(struct seq_file *m, const char *name,
 				bool value)
 {
-	seq_printf(m, "%-16s : %s\n", name, value ? "yes" :
"no");
+	seq_printf(m, "%-16s : %s\n", name, yesno(value));
 }
 
 static void virtio_gpu_add_int(struct seq_file *m, const char *name, int value)
-- 
2.34.1

Jani Nikula

2022-Jan-19 08:02 UTC

head link

[Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers

On Tue, 18 Jan 2022, Lucas De Marchi <lucas.demarchi at intel.com>
wrote:> Add some helpers under lib/string_helpers.h so they can be used
> throughout the kernel. When I started doing this there were 2 other
> previous attempts I know of, not counting the iterations each of them
> had:
>
> 1) https://lore.kernel.org/all/20191023131308.9420-1-jani.nikula at
intel.com/
> 2) https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevchenko at
linux.intel.com/#t
>
> Going through the comments I tried to find some common ground and
> justification for what is in here, addressing some of the concerns
> raised.
>
> a. This version should be a drop-in replacement for what is currently in
>    the tree, with no change in behavior or binary size. For binary
>    size what I checked wat that the linked objects in the end have the
>    same size (gcc 11). From comments in the previous attempts this seems
>    also the case for earlier compiler versions
>
> b. I didn't change the function name to choice_* as suggested by Andrew
>    Morton in 20191023155619.43e0013f0c8c673a5c508c1e at
linux-foundation.org
>    because other people argumented in favor of shorter names for these
>    simple helpers - if they are long and people simply not use due to
>    that, we failed
>
> c. Use string_helper.h for these helpers - pulling string.h in the
>    compilations units was one of the concerns and I think re-using this
>    already existing header is better than creating a new string-choice.h
>
> d. This doesn't bring onoff() helper as there are some places in the
>    kernel with onoff as variable - another name is probably needed for
>    this function in order not to shadow the variable, or those variables
>    could be renamed.  Or if people wanting  <someprefix>
>    try to find a short one
>
> e. One alternative to all of this suggested by Christian K?nig
>    (43456ba7-c372-84cc-4949-dcb817188e21 at amd.com) would be to add a
>    printk format. But besides the comment, he also seemed to like
>    the common function. This brought the argument from others that the
>    simple yesno()/enabledisable() already used in the code is easier to
>    remember and use than e.g. %py[DOY]
>
> Last patch also has some additional conversion of open coded cases. I
> preferred starting with drm/ since this is "closer to home".
>
> I hope this is a good summary of the previous attempts and a way we can
> move forward.
Thanks for picking this up again. I agree with the approach here.

Acked-by: Jani Nikula <jani.nikula at intel.com>
>
> Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my
> proposal is to take first 2 patches either through mm tree or maybe
> vsprintf. Last patch can be taken later through drm.
>
> thanks
> Lucas De Marchi
>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Andrew Morton <akpm at linux-foundation.org>
> Cc: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> Cc: Andy Shevchenko <andy.shevchenko at gmail.com>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: Christian K?nig <christian.koenig at amd.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: David Airlie <airlied at linux.ie>
> Cc: David S. Miller <davem at davemloft.net>
> Cc: Emma Anholt <emma at anholt.net>
> Cc: Eryk Brol <eryk.brol at amd.com>
> Cc: Francis Laniel <laniel_francis at privacyrequired.com>
> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> Cc: Harry Wentland <harry.wentland at amd.com>
> Cc: Jakub Kicinski <kuba at kernel.org>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Julia Lawall <julia.lawall at lip6.fr>
> Cc: Kentaro Takeda <takedakn at nttdata.co.jp>
> Cc: Leo Li <sunpeng.li at amd.com>
> Cc: Mikita Lipski <mikita.lipski at amd.com>
> Cc: Petr Mladek <pmladek at suse.com>
> Cc: Rahul Lakkireddy <rahul.lakkireddy at chelsio.com>
> Cc: Raju Rangoju <rajur at chelsio.com>
> Cc: Rasmus Villemoes <linux at rasmusvillemoes.dk>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Sakari Ailus <sakari.ailus at linux.intel.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
> Cc: Steven Rostedt <rostedt at goodmis.org>
> Cc: Vishal Kulkarni <vishal at chelsio.com>
>
> Lucas De Marchi (3):
>   lib/string_helpers: Consolidate yesno() implementation
>   lib/string_helpers: Add helpers for enable[d]/disable[d]
>   drm: Convert open yes/no strings to yesno()
>
>  drivers/gpu/drm/amd/amdgpu/atom.c              |  3 ++-
>  .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c  |  6 +-----
>  drivers/gpu/drm/drm_client_modeset.c           |  3 ++-
>  drivers/gpu/drm/drm_dp_helper.c                |  3 ++-
>  drivers/gpu/drm/drm_gem.c                      |  3 ++-
>  drivers/gpu/drm/i915/i915_utils.h              | 15 ---------------
>  drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c  |  4 +++-
>  drivers/gpu/drm/radeon/atom.c                  |  3 ++-
>  drivers/gpu/drm/v3d/v3d_debugfs.c              | 11 ++++++-----
>  drivers/gpu/drm/virtio/virtgpu_debugfs.c       |  3 ++-
>  .../net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 -----------
>  include/linux/string_helpers.h                 |  4 ++++
>  security/tomoyo/audit.c                        |  2 +-
>  security/tomoyo/common.c                       | 18 ++++--------------
>  security/tomoyo/common.h                       |  1 -
>  15 files changed, 31 insertions(+), 59 deletions(-)
-- 
Jani Nikula, Intel Open Source Graphics Center

Andy Shevchenko

2022-Jan-19 09:15 UTC

head link

[Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation

On Wednesday, January 19, 2022, Lucas De Marchi <lucas.demarchi at
intel.com>
wrote:
> There are a few implementations of yesno() in the tree. Consolidate them
> in include/linux/string_helpers.h.  Quite a few users of open coded
> yesno() could later be converted to the new function:
>
> $ git grep '?\s*"yes"\s*' | wc -l
> 286
> $ git grep '?\s*"no"\s*' | wc -l
> 20
>
> The inlined function should keep the const strings local to each
> compilation unit, the same way it's now, thus not changing the current
> behavior.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
>  .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c  |  6 +-----
>  drivers/gpu/drm/i915/i915_utils.h              |  5 -----
>  .../net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 -----------
>  include/linux/string_helpers.h                 |  2 ++
>  security/tomoyo/audit.c                        |  2 +-
>  security/tomoyo/common.c                       | 18 ++++--------------
>  security/tomoyo/common.h                       |  1 -
>  7 files changed, 8 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> index 9d43ecb1f692..b59760f91bf6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> @@ -23,6 +23,7 @@
>   *
>   */
>
> +#include <linux/string_helpers.h>
>  #include <linux/uaccess.h>
>
>  #include "dc.h"
> @@ -49,11 +50,6 @@ struct dmub_debugfs_trace_entry {
>         uint32_t param1;
>  };
>
> -static inline const char *yesno(bool v)
> -{
> -       return v ? "yes" : "no";
> -}
> -
>  /* parse_write_buffer_into_params - Helper function to parse debugfs
> write buffer into an array
>   *
>   * Function takes in attributes passed to debugfs write entry
> diff --git a/drivers/gpu/drm/i915/i915_utils.h
> b/drivers/gpu/drm/i915/i915_utils.h
> index 7a5925072466..2a8781cc648b 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -414,11 +414,6 @@ wait_remaining_ms_from_jiffies(unsigned long
> timestamp_jiffies, int to_wait_ms)
>  #define MBps(x) KBps(1000 * (x))
>  #define GBps(x) ((u64)1000 * MBps((x)))
>
> -static inline const char *yesno(bool v)
> -{
> -       return v ? "yes" : "no";
> -}
> -
>  static inline const char *onoff(bool v)
>  {
>         return v ? "on" : "off";
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
> b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
> index 7d49fd4edc9e..61a04d7abc1f 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c
> @@ -2015,17 +2015,6 @@ static const struct file_operations
> rss_debugfs_fops = {
>  /* RSS Configuration.
>   */
>
> -/* Small utility function to return the strings "yes" or
"no" if the
> supplied
> - * argument is non-zero.
> - */
> -static const char *yesno(int x)
> -{
> -       static const char *yes = "yes";
> -       static const char *no = "no";
> -
> -       return x ? yes : no;
> -}
> -
>  static int rss_config_show(struct seq_file *seq, void *v)
>  {
>         struct adapter *adapter = seq->private;
> diff --git a/include/linux/string_helpers.h b/include/linux/string_
> helpers.h
> index 4ba39e1403b2..e980dec05d31 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -102,4 +102,6 @@ char *kstrdup_quotable_file(struct file *file, gfp_t
> gfp);
>
>  void kfree_strarray(char **array, size_t n);
>
> +static inline const char *yesno(bool v) { return v ? "yes" :
"no"; }


Perhaps keep it on 4 lines? Yes, yes/no is short, but if we add others
(enable/disable) it will not be possible to keep on one line. And hence
style will be broken among similar functions.


Also it needs to be rebased and resend after -rc1, I expect conflict here.


> +
>  #endif
> diff --git a/security/tomoyo/audit.c b/security/tomoyo/audit.c
> index d79bf07e16be..1458e27361e8 100644
> --- a/security/tomoyo/audit.c
> +++ b/security/tomoyo/audit.c
> @@ -166,7 +166,7 @@ static char *tomoyo_print_header(struct
> tomoyo_request_info *r)
>                        "#%04u/%02u/%02u %02u:%02u:%02u# profile=%u
mode=%s
> granted=%s (global-pid=%u) task={ pid=%u ppid=%u uid=%u gid=%u euid=%u
> egid=%u suid=%u sgid=%u fsuid=%u fsgid=%u }",
>                        stamp.year, stamp.month, stamp.day, stamp.hour,
>                        stamp.min, stamp.sec, r->profile,
> tomoyo_mode[r->mode],
> -                      tomoyo_yesno(r->granted), gpid,
tomoyo_sys_getpid(),
> +                      yesno(r->granted), gpid, tomoyo_sys_getpid(),
>                        tomoyo_sys_getppid(),
>                        from_kuid(&init_user_ns, current_uid()),
>                        from_kgid(&init_user_ns, current_gid()),
> diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
> index 5c64927bf2b3..304ed0f426dd 100644
> --- a/security/tomoyo/common.c
> +++ b/security/tomoyo/common.c
> @@ -8,6 +8,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/slab.h>
>  #include <linux/security.h>
> +#include <linux/string_helpers.h>
>  #include "common.h"
>
>  /* String table for operation mode. */
> @@ -174,16 +175,6 @@ static bool tomoyo_manage_by_non_root;
>
>  /* Utility functions. */
>
> -/**
> - * tomoyo_yesno - Return "yes" or "no".
> - *
> - * @value: Bool value.
> - */
> -const char *tomoyo_yesno(const unsigned int value)
> -{
> -       return value ? "yes" : "no";
> -}
> -
>  /**
>   * tomoyo_addprintf - strncat()-like-snprintf().
>   *
> @@ -730,8 +721,8 @@ static void tomoyo_print_config(struct
> tomoyo_io_buffer *head, const u8 config)
>  {
>         tomoyo_io_printf(head, "={ mode=%s grant_log=%s reject_log=%s
}\n",
>                          tomoyo_mode[config & 3],
> -                        tomoyo_yesno(config &
> TOMOYO_CONFIG_WANT_GRANT_LOG),
> -                        tomoyo_yesno(config &
> TOMOYO_CONFIG_WANT_REJECT_LOG));
> +                        yesno(config & TOMOYO_CONFIG_WANT_GRANT_LOG),
> +                        yesno(config &
TOMOYO_CONFIG_WANT_REJECT_LOG));
>  }
>
>  /**
> @@ -1354,8 +1345,7 @@ static bool tomoyo_print_condition(struct
> tomoyo_io_buffer *head,
>         case 3:
>                 if (cond->grant_log != TOMOYO_GRANTLOG_AUTO)
>                         tomoyo_io_printf(head, " grant_log=%s",
> -                                        tomoyo_yesno(cond->grant_log
=> -
>  TOMOYO_GRANTLOG_YES));
> +                                        yesno(cond->grant_log =>
TOMOYO_GRANTLOG_YES));
>                 tomoyo_set_lf(head);
>                 return true;
>         }
> diff --git a/security/tomoyo/common.h b/security/tomoyo/common.h
> index 85246b9df7ca..ca285f362705 100644
> --- a/security/tomoyo/common.h
> +++ b/security/tomoyo/common.h
> @@ -959,7 +959,6 @@ char *tomoyo_read_token(struct tomoyo_acl_param
> *param);
>  char *tomoyo_realpath_from_path(const struct path *path);
>  char *tomoyo_realpath_nofollow(const char *pathname);
>  const char *tomoyo_get_exe(void);
> -const char *tomoyo_yesno(const unsigned int value);
>  const struct tomoyo_path_info *tomoyo_compare_name_union
>  (const struct tomoyo_path_info *name, const struct tomoyo_name_union
> *ptr);
>  const struct tomoyo_path_info *tomoyo_get_domainname
> --
> 2.34.1
>
>
-- 
With Best Regards,
Andy Shevchenko
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20220119/73331cd5/attachment-0001.htm>

Andy Shevchenko

2022-Jan-19 09:20 UTC

head link

[Nouveau] [PATCH 2/3] lib/string_helpers: Add helpers for enable[d]/disable[d]

On Wednesday, January 19, 2022, Lucas De Marchi <lucas.demarchi at
intel.com>
wrote:
> Follow the yes/no logic and add helpers for enabled/disabled and
> enable/disable - those are not so common throughout the kernel,
> but they give a nice way to reuse the strings to log things as
> enabled/disabled or enable/disable.
>
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_utils.h | 10 ----------
>  include/linux/string_helpers.h    |  2 ++
>  2 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_utils.h
> b/drivers/gpu/drm/i915/i915_utils.h
> index 2a8781cc648b..cbec79bae0d2 100644
> --- a/drivers/gpu/drm/i915/i915_utils.h
> +++ b/drivers/gpu/drm/i915/i915_utils.h
> @@ -419,16 +419,6 @@ static inline const char *onoff(bool v)
>         return v ? "on" : "off";
>  }
>
> -static inline const char *enabledisable(bool v)
> -{
> -       return v ? "enable" : "disable";
> -}
> -
> -static inline const char *enableddisabled(bool v)
> -{
> -       return v ? "enabled" : "disabled";
> -}
> -
>  void add_taint_for_CI(struct drm_i915_private *i915, unsigned int taint);
>  static inline void __add_taint_for_CI(unsigned int taint)
>  {
> diff --git a/include/linux/string_helpers.h b/include/linux/string_
> helpers.h
> index e980dec05d31..e4b82f364ee1 100644
> --- a/include/linux/string_helpers.h
> +++ b/include/linux/string_helpers.h
> @@ -103,5 +103,7 @@ char *kstrdup_quotable_file(struct file *file, gfp_t
> gfp);
>  void kfree_strarray(char **array, size_t n);
>
>  static inline const char *yesno(bool v) { return v ? "yes" :
"no"; }
> +static inline const char *enabledisable(bool v) { return v ?
"enable" :
> "disable"; }
> +static inline const char *enableddisabled(bool v) { return v ?
"enabled"
> : "disabled"; }

Looks not readable even if takes 80 characters. Please, keep original style.


I believe you wanted to have nice negative statistics from day 1, then you
may add more patches in the series to cleanup more users.



>
>  #endif
> --
> 2.34.1
>
>
-- 
With Best Regards,
Andy Shevchenko
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20220119/b3b0eef2/attachment.htm>

Andy Shevchenko

2022-Jan-19 09:28 UTC

head link

[Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers

On Wednesday, January 19, 2022, Lucas De Marchi <lucas.demarchi at
intel.com>
wrote:
> Add some helpers under lib/string_helpers.h so they can be used
> throughout the kernel. When I started doing this there were 2 other
> previous attempts I know of, not counting the iterations each of them
> had:
>
> 1) https://lore.kernel.org/all/20191023131308.9420-1-jani.
> nikula at intel.com/
> 2) https://lore.kernel.org/all/20210215142137.64476-1-andriy.
> shevchenko at linux.intel.com/#t
>
> Going through the comments I tried to find some common ground and
> justification for what is in here, addressing some of the concerns
> raised.
>
> a. This version should be a drop-in replacement for what is currently in
>    the tree, with no change in behavior or binary size. For binary
>    size what I checked wat that the linked objects in the end have the
>    same size (gcc 11). From comments in the previous attempts this seems
>    also the case for earlier compiler versions
>
> b. I didn't change the function name to choice_* as suggested by Andrew
>    Morton in 20191023155619.43e0013f0c8c673a5c508c1e at
linux-foundation.org
>    because other people argumented in favor of shorter names for these
>    simple helpers - if they are long and people simply not use due to
>    that, we failed
>
> c. Use string_helper.h for these helpers - pulling string.h in the
>    compilations units was one of the concerns and I think re-using this
>    already existing header is better than creating a new string-choice.h
>
> d. This doesn't bring onoff() helper as there are some places in the
>    kernel with onoff as variable - another name is probably needed for
>    this function in order not to shadow the variable, or those variables
>    could be renamed.  Or if people wanting  <someprefix>
>    try to find a short one
>
> e. One alternative to all of this suggested by Christian K?nig
>    (43456ba7-c372-84cc-4949-dcb817188e21 at amd.com) would be to add a
>    printk format. But besides the comment, he also seemed to like
>    the common function. This brought the argument from others that the
>    simple yesno()/enabledisable() already used in the code is easier to
>    remember and use than e.g. %py[DOY]
>
> Last patch also has some additional conversion of open coded cases. I
> preferred starting with drm/ since this is "closer to home".
>
> I hope this is a good summary of the previous attempts and a way we can
> move forward.
>
> Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my
> proposal is to take first 2 patches either through mm tree or maybe
> vsprintf. Last patch can be taken later through drm.


I believe the best if we go via drm-misc with the entire series.

I have couple of comments, after addressing them:

Reviewed-by: Andy Shevchenko <andy.shevchenko at gmail.com>

> thanks
> Lucas De Marchi
>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Andrew Morton <akpm at linux-foundation.org>
> Cc: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> Cc: Andy Shevchenko <andy.shevchenko at gmail.com>
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: Christian K?nig <christian.koenig at amd.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Cc: David Airlie <airlied at linux.ie>
> Cc: David S. Miller <davem at davemloft.net>
> Cc: Emma Anholt <emma at anholt.net>
> Cc: Eryk Brol <eryk.brol at amd.com>
> Cc: Francis Laniel <laniel_francis at privacyrequired.com>
> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> Cc: Harry Wentland <harry.wentland at amd.com>
> Cc: Jakub Kicinski <kuba at kernel.org>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Julia Lawall <julia.lawall at lip6.fr>
> Cc: Kentaro Takeda <takedakn at nttdata.co.jp>
> Cc: Leo Li <sunpeng.li at amd.com>
> Cc: Mikita Lipski <mikita.lipski at amd.com>
> Cc: Petr Mladek <pmladek at suse.com>
> Cc: Rahul Lakkireddy <rahul.lakkireddy at chelsio.com>
> Cc: Raju Rangoju <rajur at chelsio.com>
> Cc: Rasmus Villemoes <linux at rasmusvillemoes.dk>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Sakari Ailus <sakari.ailus at linux.intel.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky at gmail.com>
> Cc: Steven Rostedt <rostedt at goodmis.org>
> Cc: Vishal Kulkarni <vishal at chelsio.com>
>
> Lucas De Marchi (3):
>   lib/string_helpers: Consolidate yesno() implementation
>   lib/string_helpers: Add helpers for enable[d]/disable[d]
>   drm: Convert open yes/no strings to yesno()
>
>  drivers/gpu/drm/amd/amdgpu/atom.c              |  3 ++-
>  .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c  |  6 +-----
>  drivers/gpu/drm/drm_client_modeset.c           |  3 ++-
>  drivers/gpu/drm/drm_dp_helper.c                |  3 ++-
>  drivers/gpu/drm/drm_gem.c                      |  3 ++-
>  drivers/gpu/drm/i915/i915_utils.h              | 15 ---------------
>  drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c  |  4 +++-
>  drivers/gpu/drm/radeon/atom.c                  |  3 ++-
>  drivers/gpu/drm/v3d/v3d_debugfs.c              | 11 ++++++-----
>  drivers/gpu/drm/virtio/virtgpu_debugfs.c       |  3 ++-
>  .../net/ethernet/chelsio/cxgb4/cxgb4_debugfs.c | 11 -----------
>  include/linux/string_helpers.h                 |  4 ++++
>  security/tomoyo/audit.c                        |  2 +-
>  security/tomoyo/common.c                       | 18 ++++--------------
>  security/tomoyo/common.h                       |  1 -
>  15 files changed, 31 insertions(+), 59 deletions(-)
>
> --
> 2.34.1
>
>
-- 
With Best Regards,
Andy Shevchenko
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20220119/0151106a/attachment.htm>

Petr Mladek

2022-Jan-19 13:18 UTC

head link

[Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers

On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote:> Add some helpers under lib/string_helpers.h so they can be used
> throughout the kernel. When I started doing this there were 2 other
> previous attempts I know of, not counting the iterations each of them
> had:
> 
> 1) https://lore.kernel.org/all/20191023131308.9420-1-jani.nikula at
intel.com/
> 2) https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevchenko at
linux.intel.com/#t
> 
> Going through the comments I tried to find some common ground and
> justification for what is in here, addressing some of the concerns
> raised.
> 
> d. This doesn't bring onoff() helper as there are some places in the
>    kernel with onoff as variable - another name is probably needed for
>    this function in order not to shadow the variable, or those variables
>    could be renamed.  Or if people wanting  <someprefix>
>    try to find a short one
I would call it str_on_off().

And I would actually suggest to use the same style also for
the other helpers.

The "str_" prefix would make it clear that it is something with
string. There are other <prefix>_on_off() that affect some
functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off().

The dash '_' would significantly help to parse the name. yesno() and
onoff() are nicely short and kind of acceptable. But "enabledisable()"
is a puzzle.

IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good
compromise.

The main motivation should be code readability. You write the
code once. But many people will read it many times. Open coding
is sometimes better than misleading macro names.

That said, I do not want to block this patchset. If others like
it... ;-)

> e. One alternative to all of this suggested by Christian K?nig
>    (43456ba7-c372-84cc-4949-dcb817188e21 at amd.com) would be to add a
>    printk format. But besides the comment, he also seemed to like
>    the common function. This brought the argument from others that the
>    simple yesno()/enabledisable() already used in the code is easier to
>    remember and use than e.g. %py[DOY]
Thanks for not going this way :-)
> Last patch also has some additional conversion of open coded cases. I
> preferred starting with drm/ since this is "closer to home".
> 
> I hope this is a good summary of the previous attempts and a way we can
> move forward.
> 
> Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my
> proposal is to take first 2 patches either through mm tree or maybe
> vsprintf. Last patch can be taken later through drm.
I agree with Andy that it should go via drm tree. It would make it
easier to handle potential conflicts.

Just in case, you decide to go with str_yes_no() or something similar.
Mass changes are typically done at the end on the merge window.
The best solution is when it can be done by a script.

Best Regards,
Petr

Jani Nikula

2022-Jan-19 14:16 UTC

head link

[Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers

On Wed, 19 Jan 2022, Petr Mladek <pmladek at suse.com>
wrote:> On Tue 2022-01-18 23:24:47, Lucas De Marchi wrote:
>> Add some helpers under lib/string_helpers.h so they can be used
>> throughout the kernel. When I started doing this there were 2 other
>> previous attempts I know of, not counting the iterations each of them
>> had:
>> 
>> 1) https://lore.kernel.org/all/20191023131308.9420-1-jani.nikula at
intel.com/
>> 2) https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevchenko
at linux.intel.com/#t
>> 
>> Going through the comments I tried to find some common ground and
>> justification for what is in here, addressing some of the concerns
>> raised.
>> 
>> d. This doesn't bring onoff() helper as there are some places in
the
>>    kernel with onoff as variable - another name is probably needed for
>>    this function in order not to shadow the variable, or those
variables
>>    could be renamed.  Or if people wanting  <someprefix>
>>    try to find a short one
>
> I would call it str_on_off().
>
> And I would actually suggest to use the same style also for
> the other helpers.
>
> The "str_" prefix would make it clear that it is something with
> string. There are other <prefix>_on_off() that affect some
> functionality, e.g. mute_led_on_off(), e1000_vlan_filter_on_off().
>
> The dash '_' would significantly help to parse the name. yesno()
and
> onoff() are nicely short and kind of acceptable. But
"enabledisable()"
> is a puzzle.
>
> IMHO, str_yes_no(), str_on_off(), str_enable_disable() are a good
> compromise.
>
> The main motivation should be code readability. You write the
> code once. But many people will read it many times. Open coding
> is sometimes better than misleading macro names.
>
> That said, I do not want to block this patchset. If others like
> it... ;-)
I don't mind the names either way. Adding the prefix and dashes is
helpful in that it's possible to add the functions first and convert
users at leisure, though with a bunch of churn, while using names that
collide with existing ones requires the changes to happen in one go.

What I do mind is grinding this series to a halt once again. I sent a
handful of versions of this three years ago, with inconclusive
bikeshedding back and forth, eventually threw my hands up in disgust,
and walked away.
>
>
>> e. One alternative to all of this suggested by Christian K?nig
>>    (43456ba7-c372-84cc-4949-dcb817188e21 at amd.com) would be to add a
>>    printk format. But besides the comment, he also seemed to like
>>    the common function. This brought the argument from others that the
>>    simple yesno()/enabledisable() already used in the code is easier to
>>    remember and use than e.g. %py[DOY]
>
> Thanks for not going this way :-)
>
>> Last patch also has some additional conversion of open coded cases. I
>> preferred starting with drm/ since this is "closer to home".
>> 
>> I hope this is a good summary of the previous attempts and a way we can
>> move forward.
>> 
>> Andrew Morton, Petr Mladek, Andy Shevchenko: if this is accepted, my
>> proposal is to take first 2 patches either through mm tree or maybe
>> vsprintf. Last patch can be taken later through drm.
>
> I agree with Andy that it should go via drm tree. It would make it
> easier to handle potential conflicts.
>
> Just in case, you decide to go with str_yes_no() or something similar.
> Mass changes are typically done at the end on the merge window.
> The best solution is when it can be done by a script.
>
> Best Regards,
> Petr
-- 
Jani Nikula, Intel Open Source Graphics Center

Andy Shevchenko

2022-Jan-19 19:30 UTC

head link

[Nouveau] [PATCH 3/3] drm: Convert open yes/no strings to yesno()

On Tue, Jan 18, 2022 at 11:24:50PM -0800, Lucas De Marchi
wrote:> linux/string_helpers.h provides a helper to return
"yes"/"no"
> strings. Replace the open coded versions with yesno(). The places were
> identified with the following semantic patch:
> 
> 	@@
> 	expression b;
> 	@@
> 
> 	- b ? "yes" : "no"
> 	+ yesno(b)
> 
> Then the includes were added, so we include-what-we-use, and parenthesis
> adjusted in drivers/gpu/drm/v3d/v3d_debugfs.c. After the conversion we
> still see the same binary sizes:
> 
>    text    data     bss     dec     hex filename
> 1442171   60344     800 1503315  16f053 ./drivers/gpu/drm/radeon/radeon.ko
> 1442171   60344     800 1503315  16f053
./drivers/gpu/drm/radeon/radeon.ko.old
> 5985991  324439   33808 6344238  60ce2e
./drivers/gpu/drm/amd/amdgpu/amdgpu.ko
> 5985991  324439   33808 6344238  60ce2e
./drivers/gpu/drm/amd/amdgpu/amdgpu.ko.old
>  411986   10490    6176  428652   68a6c ./drivers/gpu/drm/drm.ko
>  411986   10490    6176  428652   68a6c ./drivers/gpu/drm/drm.ko.old
> 1970292  109515    2352 2082159  1fc56f
./drivers/gpu/drm/nouveau/nouveau.ko
> 1970292  109515    2352 2082159  1fc56f
./drivers/gpu/drm/nouveau/nouveau.ko.old
...
>  #include <linux/module.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
> +#include <linux/string_helpers.h>
+ blank line?
> +#include <linux/string_helpers.h>
...
>  	seq_printf(m, "\tDP branch device present: %s\n",
> -		   branch_device ? "yes" : "no");
> +		   yesno(branch_device));
Now it's possible to keep this on one line.

...
>  	drm_printf_indent(p, indent, "imported=%s\n",
> -			  obj->import_attach ? "yes" : "no");
> +			  yesno(obj->import_attach));
81 here, but anyway, ditto!

...
>   */
+blank line here?
> +#include <linux/string_helpers.h>
> +
>  #include "aux.h"
>  #include "pad.h"
...
>  	seq_printf(m, "MMU:        %s\n",
> -		   (ident2 & V3D_HUB_IDENT2_WITH_MMU) ? "yes" :
"no");
> +		   yesno(ident2 & V3D_HUB_IDENT2_WITH_MMU));
>  	seq_printf(m, "TFU:        %s\n",
> -		   (ident1 & V3D_HUB_IDENT1_WITH_TFU) ? "yes" :
"no");
> +		   yesno(ident1 & V3D_HUB_IDENT1_WITH_TFU));
>  	seq_printf(m, "TSY:        %s\n",
> -		   (ident1 & V3D_HUB_IDENT1_WITH_TSY) ? "yes" :
"no");
> +		   yesno(ident1 & V3D_HUB_IDENT1_WITH_TSY));
>  	seq_printf(m, "MSO:        %s\n",
> -		   (ident1 & V3D_HUB_IDENT1_WITH_MSO) ? "yes" :
"no");
> +		   yesno(ident1 & V3D_HUB_IDENT1_WITH_MSO));
>  	seq_printf(m, "L3C:        %s (%dkb)\n",
> -		   (ident1 & V3D_HUB_IDENT1_WITH_L3C) ? "yes" :
"no",
> +		   yesno(ident1 & V3D_HUB_IDENT1_WITH_L3C),
>  		   V3D_GET_FIELD(ident2, V3D_HUB_IDENT2_L3C_NKB));
I believe it's fine to join back to have less LOCs (yes, it will be 83 or
so,
but I believe in these cases it's very much okay).

-- 
With Best Regards,
Andy Shevchenko

Nouveau - Jan 2022 - [PATCH 2/3] lib/string_helpers: Add helpers for enable[d]/disable[d]

[Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers

[Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation

[Nouveau] [PATCH 2/3] lib/string_helpers: Add helpers for enable[d]/disable[d]

[Nouveau] [PATCH 3/3] drm: Convert open yes/no strings to yesno()

[Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers

[Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation

[Nouveau] [PATCH 2/3] lib/string_helpers: Add helpers for enable[d]/disable[d]

[Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers

[Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers

[Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers

[Nouveau] [PATCH 3/3] drm: Convert open yes/no strings to yesno()