Re: [Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
> > except '"no\0\0yes" + v * 4' works a bit better. > > Is it a C code obfuscation contest? That would be: return &(v * 3)["no\0yes"]; :-) - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [Nouveau] [Intel-gfx] [PATCH 0/3] lib/string_helpers: Add a few string helpers
On Wed, Jan 19, 2022 at 12:53:43PM -0800, Lucas De Marchi wrote: > On Wed, Jan 19, 2022 at 05:15:02PM +0100, Daniel Vetter wrote: > > On Wed, Jan 19, 2022 at 04:16:12PM +0200, Jani Nikula wrote: ... > > Yeah we can sed this anytime later we want to, but we need to get the foot > > in the door. There's also a pile more of these all over. > > > > Acked-by: Daniel Vetter > > > > on the series, maybe it helps? And yes let's merge this through drm-misc. > > Ok, it seems we are reaching some agreement here then: > - Change it to use str_ prefix Not sure about this (*), but have no strong argument against. Up to you. Ah, yes, Jani said about additional churn this change will make if goes together with this series. Perhaps it can be done separately? > - Wait -rc1 to avoid conflict > - Merge through drm-misc *) E.g. yesno() to me doesn't sound too bad to misunderstand its meaning, esp.when it's used as an argument to the printf() functions. -- With Best Regards, Andy Shevchenko
Re: [Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, Jan 19, 2022 at 04:00:17PM -0500, Steven Rostedt wrote: > On Wed, 19 Jan 2022 21:25:08 +0200 > Andy Shevchenko wrote: > > > > I say keep it one line! > > > > > > Reviewed-by: Steven Rostedt (Google) > > > > I believe Sakari strongly follows the 80 rule, which means... > > Checkpatch says "100" I think we need to simply update the docs (the > documentation always lags the code ;-) The idea of checkpatch change is for old code to avoid tons of patches to satisfy 80 rule in (mostly) staging code. Some maintainers started / have been using relaxed approach. -- With Best Regards, Andy Shevchenko
Re: [Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, 19 Jan 2022 21:25:08 +0200 Andy Shevchenko wrote: > > I say keep it one line! > > > > Reviewed-by: Steven Rostedt (Google) > > I believe Sakari strongly follows the 80 rule, which means... Checkpatch says "100" I think we need to simply update the docs (the documentation always lags the code ;-) bdc48fa11e46f -- Steve
Re: [Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, 19 Jan 2022 21:22:57 +0200
Andy Shevchenko wrote:
> On Wed, Jan 19, 2022 at 04:38:26PM +, David Laight wrote:
> > > > > > +static inline const char *yesno(bool v) { return v ? "yes" : "no";
> > > > > > }
> > >
> > > return "yes\0no" + v * 4;
> > >
> > > :-)
> >
> > except '"no\0\0yes" + v * 4' works a bit better.
>
> Is it a C code obfuscation contest?
>
return '/'/'/';
-- Steve
Re: [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: > >textdata 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 104906176 428652 68a6c ./drivers/gpu/drm/drm.ko > 411986 104906176 428652 68a6c ./drivers/gpu/drm/drm.ko.old > 1970292 1095152352 2082159 1fc56f ./drivers/gpu/drm/nouveau/nouveau.ko > 1970292 1095152352 2082159 1fc56f > ./drivers/gpu/drm/nouveau/nouveau.ko.old ... > #include > #include > #include > +#include + blank line? > +#include ... > 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 > + > #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
Re: [Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, Jan 19, 2022 at 04:38:26PM +, David Laight wrote:
> > > > > +static inline const char *yesno(bool v) { return v ? "yes" : "no"; }
> >
> > return "yes\0no" + v * 4;
> >
> > :-)
>
> except '"no\0\0yes" + v * 4' works a bit better.
Is it a C code obfuscation contest?
--
With Best Regards,
Andy Shevchenko
Re: [Nouveau] [RFC PATCH v3 2/3] drm: add support modifiers for drivers whose planes only support linear layout
On Wed, Jan 19, 2022 at 11:35:22AM +0900, Esaki Tomohito wrote:
> On 2022/01/18 18:53, Andy Shevchenko wrote:
> > On Mon, Jan 17, 2022 at 02:15:48PM +0900, Esaki Tomohito wrote:
> > > On 2022/01/14 23:16, Andy Shevchenko wrote:
> > > > On Fri, Jan 14, 2022 at 07:17:52PM +0900, Tomohito Esaki wrote:
> > > > > The LINEAR modifier is advertised as default if a driver doesn't
> > > > > specify
> > > > > modifiers.
> > > >
> > > > ...
> > > >
> > > > > + const uint64_t default_modifiers[] = {
> > > > > + DRM_FORMAT_MOD_LINEAR,
> > > > > + DRM_FORMAT_MOD_INVALID
> > > >
> > > > + Comma?
> > >
> > > There is no mention in the coding style about adding/removing a comma to
> > > the
> > > last element of an array. Is there a policy in drm driver?
> > >
> > > I think the advantage of adding a comma to the last element of an array is
> > > that diff is only one line when an element is added to the end.
> > > However since INVALID is always the last element in the modifiers array, I
> > > think it can be either in this case.
> > > If there is a policy, I will match it.
> >
> > Indeed, but there is a common sense. The idea behind (multi-line)
> > definitions
> > that when next time somebody will add an element in the array, there are
> > will
> > be:
> >
> > a) no additional churn (like in case of this patch, if the item will be
> > added
> > at the bottom;
> >
> > b) an element that may not be added behind the terminator, which will look
> > weird.
> >
> > That said, the question is if the element is terminator one or not, if not,
> > comma is better than no comma and vise versa.
>
> Ah I see. In this case, DRM_FORMAT_MOD_INVALID is terminator, so it
> should not have a comma.
Thanks for pointing this out. In this case we are good and any new item, AFAIU,
must be added before _INVALID one.
--
With Best Regards,
Andy Shevchenko
Re: [Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, Jan 19, 2022 at 10:06:35AM -0500, Steven Rostedt wrote: > On Wed, 19 Jan 2022 11:18:59 +0200 > Sakari Ailus wrote: > > On Tue, Jan 18, 2022 at 11:24:48PM -0800, Lucas De Marchi wrote: > > > @@ -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)); > > > > This would be better split on two lines. > > Really? Yuck! > > I thought the "max line size" guideline was going to grow to a 100, but I > still see it as 80. But anyway... > > cond->grant_log == > TOMOYO_GRANTLOG_YES > > is not readable at all. Not compared to > > cond->grant_log == TOMOYO_GRANTLOG_YES > > I say keep it one line! > > Reviewed-by: Steven Rostedt (Google) I believe Sakari strongly follows the 80 rule, which means... > > Reviewed-by: Sakari Ailus ...chose either of these tags and be happy with :-) -- With Best Regards, Andy Shevchenko
Re: [Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
> > > +static inline const char *yesno(bool v) { return v ? "yes" : "no"; }
return "yes\0no" + v * 4;
:-)
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers
On Wed, Jan 19, 2022 at 04:16:12PM +0200, Jani Nikula wrote: > On Wed, 19 Jan 2022, Petr Mladek 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.nik...@intel.com/ > >> 2) > >> https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevche...@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 > >>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 _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. Yeah we can sed this anytime later we want to, but we need to get the foot in the door. There's also a pile more of these all over. Acked-by: Daniel Vetter on the series, maybe it helps? And yes let's merge this through drm-misc. -Daniel > > > > > > >> e. One alternative to all of this suggested by Christian König > >>(43456ba7-c372-84cc-4949-dcb817188...@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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, 19 Jan 2022 11:18:59 +0200 Sakari Ailus wrote: > On Tue, Jan 18, 2022 at 11:24:48PM -0800, Lucas De Marchi wrote: > > @@ -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)); > > This would be better split on two lines. Really? Yuck! I thought the "max line size" guideline was going to grow to a 100, but I still see it as 80. But anyway... cond->grant_log == TOMOYO_GRANTLOG_YES is not readable at all. Not compared to cond->grant_log == TOMOYO_GRANTLOG_YES I say keep it one line! Reviewed-by: Steven Rostedt (Google) -- Steve > > Then, > > Reviewed-by: Sakari Ailus
Re: [Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, 19 Jan 2022 11:15:08 +0200
Andy Shevchenko wrote:
> > +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.
Agreed. Functions should always be of the normal format:
type func(params)
{
body;
}
Unless it is a stub function.
type func(params) { return 0; }
-- Steve
Re: [Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers
On Wed, 19 Jan 2022, Petr Mladek 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.nik...@intel.com/ >> 2) >> https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevche...@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 >>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 _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-dcb817188...@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
Re: [Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers
On Wednesday, January 19, 2022, 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. > nik...@intel.com/ > 2) https://lore.kernel.org/all/20210215142137.64476-1-andriy. > shevche...@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.43e0013f0c8c673a5c508...@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 >try to find a short one > > e. One alternative to all of this suggested by Christian König >(43456ba7-c372-84cc-4949-dcb817188...@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 > thanks > Lucas De Marchi > > Cc: Alex Deucher > Cc: Andrew Morton > Cc: Andy Shevchenko > Cc: Andy Shevchenko > Cc: Ben Skeggs > Cc: Christian König > Cc: Chris Wilson > Cc: Daniel Vetter > Cc: David Airlie > Cc: David S. Miller > Cc: Emma Anholt > Cc: Eryk Brol > Cc: Francis Laniel > Cc: Greg Kroah-Hartman > Cc: Harry Wentland > Cc: Jakub Kicinski > Cc: Jani Nikula > Cc: Joonas Lahtinen > Cc: Julia Lawall > Cc: Kentaro Takeda > Cc: Leo Li > Cc: Mikita Lipski > Cc: Petr Mladek > Cc: Rahul Lakkireddy > Cc: Raju Rangoju > Cc: Rasmus Villemoes > Cc: Rodrigo Vivi > Cc: Sakari Ailus > Cc: Sergey Senozhatsky > Cc: Steven Rostedt > Cc: Vishal Kulkarni > > 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
Re: [Nouveau] [PATCH 2/3] lib/string_helpers: Add helpers for enable[d]/disable[d]
On Wednesday, January 19, 2022, Lucas De Marchi
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
> ---
> 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
Re: [Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wednesday, January 19, 2022, Lucas De Marchi
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
> ---
> .../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
> #include
>
> #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(_user_ns, current_uid()),
>from_kgid(_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
> #include
>
Re: [Nouveau] [PATCH 0/3] lib/string_helpers: Add a few string helpers
On Tue, 18 Jan 2022, 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.nik...@intel.com/ > 2) > https://lore.kernel.org/all/20210215142137.64476-1-andriy.shevche...@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.43e0013f0c8c673a5c508...@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 >try to find a short one > > e. One alternative to all of this suggested by Christian König >(43456ba7-c372-84cc-4949-dcb817188...@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 > > 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 > Cc: Andrew Morton > Cc: Andy Shevchenko > Cc: Andy Shevchenko > Cc: Ben Skeggs > Cc: Christian König > Cc: Chris Wilson > Cc: Daniel Vetter > Cc: David Airlie > Cc: David S. Miller > Cc: Emma Anholt > Cc: Eryk Brol > Cc: Francis Laniel > Cc: Greg Kroah-Hartman > Cc: Harry Wentland > Cc: Jakub Kicinski > Cc: Jani Nikula > Cc: Joonas Lahtinen > Cc: Julia Lawall > Cc: Kentaro Takeda > Cc: Leo Li > Cc: Mikita Lipski > Cc: Petr Mladek > Cc: Rahul Lakkireddy > Cc: Raju Rangoju > Cc: Rasmus Villemoes > Cc: Rodrigo Vivi > Cc: Sakari Ailus > Cc: Sergey Senozhatsky > Cc: Steven Rostedt > Cc: Vishal Kulkarni > > 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
