Re: [Nouveau] [RFC PATCH v3 2/3] drm: add support modifiers for drivers whose planes only support linear layout
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?
> + };
Why not enum?
--
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 Fri, Jan 14, 2022 at 03:07:21PM +, Simon Ser wrote: > On Friday, January 14th, 2022 at 15:16, Andy Shevchenko > wrote: > > > Why not enum? > > There is no enum for DRM format modifiers. I'm not sure how this prevents to use enum in the code instead of const u64. Any specific reason for that? -- 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 Fri, Jan 14, 2022 at 03:42:54PM +, Simon Ser wrote: > On Friday, January 14th, 2022 at 16:17, Andy Shevchenko > wrote: > > > On Fri, Jan 14, 2022 at 03:07:21PM +, Simon Ser wrote: > > > On Friday, January 14th, 2022 at 15:16, Andy Shevchenko > > > wrote: > > > > > > > Why not enum? > > > > > > There is no enum for DRM format modifiers. > > > > I'm not sure how this prevents to use enum in the code instead of const u64. > > Any specific reason for that? > > I'm not sure how one would use an enum as the array item type, when there is > no > defined enum type. > > Moreover, all the rest of DRM uses uint64 for modifiers. Ah, I see now. This is an array that filled by predefined values. Thanks for explanation. -- With Best Regards, Andy Shevchenko
Re: [Nouveau] [RFC PATCH v4 0/3] Add support modifiers for drivers whose planes only support linear layout
On Tue, Jan 18, 2022 at 05:36:49PM +0900, Tomohito Esaki wrote: > Some drivers whose planes only support linear layout fb do not support format > modifiers. > These drivers should support modifiers, however the DRM core should handle > this > rather than open-coding in every driver. > > In this patch series, these drivers expose format modifiers based on the > following suggestion[1]. > > On Thu, Nov 18, 2021 at 01:02:11PM +, Daniel Stone wrote: > > I think the best way forward here is: > > - add a new mode_config.cannot_support_modifiers flag, and enable > > this in radeon (plus any other drivers in the same boat) > > - change drm_universal_plane_init() to advertise the LINEAR modifier > > when NULL is passed as the modifier list (including installing a > > default .format_mod_supported hook) > > - remove the mode_config.allow_fb_modifiers hook and always > > advertise modifier support, unless > > mode_config.cannot_support_modifiers is set FWIW, Reviewed-by: Andy Shevchenko > [1] > https://patchwork.kernel.org/project/linux-renesas-soc/patch/20190509054518.10781-1-e...@igel.co.jp/#24602575 > > v4: > * modify documentation for fb_modifiers_not_supported flag in kerneldoc > > v3: https://www.spinics.net/lists/dri-devel/msg329102.html > * change the order as follows: >1. add fb_modifiers_not_supported flag >2. add default modifiers >3. remove allow_fb_modifiers flag > * add a conditional disable in amdgpu_dm_plane_init() > > v2: https://www.spinics.net/lists/dri-devel/msg328939.html > * rebase to the latest master branch (5.16.0+) > + "drm/plane: Make format_mod_supported truly optional" patch [2] > [2] https://patchwork.freedesktop.org/patch/467940/?series=98255&rev=3 > > v1: https://www.spinics.net/lists/dri-devel/msg327352.html > * The initial patch set > > Tomohito Esaki (3): > drm: introduce fb_modifiers_not_supported flag in mode_config > drm: add support modifiers for drivers whose planes only support > linear layout > drm: remove allow_fb_modifiers > > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 6 ++--- > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 2 ++ > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 2 ++ > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 1 + > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 2 ++ > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++ > drivers/gpu/drm/drm_framebuffer.c | 6 ++--- > drivers/gpu/drm/drm_ioctl.c | 2 +- > drivers/gpu/drm/drm_plane.c | 22 +-- > drivers/gpu/drm/nouveau/nouveau_display.c | 6 +++-- > drivers/gpu/drm/radeon/radeon_display.c | 2 ++ > .../gpu/drm/selftests/test-drm_framebuffer.c | 1 - > include/drm/drm_mode_config.h | 18 +-- > include/drm/drm_plane.h | 3 +++ > 14 files changed, 43 insertions(+), 33 deletions(-) > > -- > 2.25.1 > -- 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 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.
--
With Best Regards,
Andy Shevchenko
Re: [Nouveau] [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
de[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
> #include
> #include
> +#include
> #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
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 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 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] [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 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] [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: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] [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 v2 09/11] drm: Convert open-coded yes/no strings to yesno()
On Wed, Jan 26, 2022 at 11:39 AM Lucas De Marchi wrote: > > linux/string_helpers.h provides a helper to return "yes"/"no" strings. > Replace the open coded versions with str_yes_no(). The places were > identified with the following semantic patch: > > @@ > expression b; > @@ > > - b ? "yes" : "no" > + str_yes_no(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 > 511493295 212 54656d580 virtio/virtio-gpu.ko.old > 511493295 212 54656d580 virtio/virtio-gpu.ko > 1441491 60340 800 1502631 16eda7 radeon/radeon.ko.old > 1441491 60340 800 1502631 16eda7 radeon/radeon.ko > 6125369 328538 34000 6487907 62ff63 amd/amdgpu/amdgpu.ko.old > 6125369 328538 34000 6487907 62ff63 amd/amdgpu/amdgpu.ko > 411986 104906176 428652 68a6c drm.ko.old > 411986 104906176 428652 68a6c drm.ko > 981291636 264 100029 186bd dp/drm_dp_helper.ko.old > 981291636 264 100029 186bd dp/drm_dp_helper.ko > 1973432 1096402352 2085424 1fd230 nouveau/nouveau.ko.old > 1973432 1096402352 2085424 1fd230 nouveau/nouveau.ko This probably won't change for modules, but if you compile in the linker may try to optimize it. Would be nice to see the old-new for `make allyesconfig` or equivalent. ... > seq_printf(m, "\tDP branch device present: %s\n", > - branch_device ? "yes" : "no"); > + str_yes_no(branch_device)); Can it be now on one line? Same Q for all similar cases in the entire series. -- With Best Regards, Andy Shevchenko
Re: [Nouveau] [PATCH v2 00/11] lib/string_helpers: Add a few string helpers
drivers/gpu/drm/i915/display/intel_dpll.c | 3 +- > drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 7 +- > drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 7 +- > drivers/gpu/drm/i915/display/intel_fbc.c | 4 +- > drivers/gpu/drm/i915/display/intel_fdi.c | 8 +- > drivers/gpu/drm/i915/display/intel_hdmi.c | 3 +- > drivers/gpu/drm/i915/display/intel_sprite.c | 6 +- > drivers/gpu/drm/i915/display/vlv_dsi_pll.c| 3 +- > .../gpu/drm/i915/gem/selftests/huge_pages.c | 9 +- > .../drm/i915/gem/selftests/i915_gem_context.c | 7 +- > drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 3 +- > drivers/gpu/drm/i915/gt/intel_engine_cs.c | 11 +- > .../drm/i915/gt/intel_execlists_submission.c | 7 +- > drivers/gpu/drm/i915/gt/intel_gt_pm.c | 3 +- > drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 52 ++-- > drivers/gpu/drm/i915/gt/intel_rc6.c | 5 +- > drivers/gpu/drm/i915/gt/intel_reset.c | 3 +- > drivers/gpu/drm/i915/gt/intel_rps.c | 13 +- > drivers/gpu/drm/i915/gt/intel_sseu.c | 9 +- > drivers/gpu/drm/i915/gt/intel_sseu_debugfs.c | 10 +- > drivers/gpu/drm/i915/gt/selftest_timeline.c | 3 +- > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 5 +- > drivers/gpu/drm/i915/gt/uc/intel_guc_log.c| 5 +- > drivers/gpu/drm/i915/gt/uc/intel_guc_rc.c | 6 +- > drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 4 +- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 14 +- > drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c | 20 +- > drivers/gpu/drm/i915/i915_debugfs.c | 17 +- > drivers/gpu/drm/i915/i915_driver.c| 4 +- > drivers/gpu/drm/i915/i915_gpu_error.c | 9 +- > drivers/gpu/drm/i915/i915_params.c| 5 +- > drivers/gpu/drm/i915/i915_utils.h | 21 +- > drivers/gpu/drm/i915/intel_device_info.c | 8 +- > drivers/gpu/drm/i915/intel_dram.c | 10 +- > drivers/gpu/drm/i915/intel_pm.c | 14 +- > drivers/gpu/drm/i915/pxp/intel_pxp_debugfs.c | 4 +- > drivers/gpu/drm/i915/selftests/i915_active.c | 3 +- > drivers/gpu/drm/i915/vlv_suspend.c| 3 +- > drivers/gpu/drm/nouveau/nvkm/subdev/i2c/aux.c | 5 +- > drivers/gpu/drm/radeon/atom.c | 3 +- > drivers/gpu/drm/v3d/v3d_debugfs.c | 11 +- > drivers/gpu/drm/virtio/virtgpu_debugfs.c | 4 +- > .../ethernet/chelsio/cxgb4/cxgb4_debugfs.c| 249 ++ > include/linux/string_helpers.h| 20 ++ > security/tomoyo/audit.c | 2 +- > security/tomoyo/common.c | 19 +- > security/tomoyo/common.h | 1 - > 60 files changed, 482 insertions(+), 373 deletions(-) > > -- > 2.34.1 > -- With Best Regards, Andy Shevchenko
Re: [Nouveau] [Intel-gfx] [PATCH v2 09/11] drm: Convert open-coded yes/no strings to yesno()
On Wed, Jan 26, 2022 at 02:43:45AM -0800, Lucas De Marchi wrote: > On Wed, Jan 26, 2022 at 12:12:50PM +0200, Andy Shevchenko wrote: > > On Wed, Jan 26, 2022 at 11:39 AM Lucas De Marchi > > wrote: ... > > > 411986 104906176 428652 68a6c drm.ko.old > > > 411986 104906176 428652 68a6c drm.ko > > > 981291636 264 100029 186bd dp/drm_dp_helper.ko.old > > > 981291636 264 100029 186bd dp/drm_dp_helper.ko > > > 1973432 1096402352 2085424 1fd230 nouveau/nouveau.ko.old > > > 1973432 1096402352 2085424 1fd230 nouveau/nouveau.ko > > > > This probably won't change for modules, but if you compile in the > > linker may try to optimize it. Would be nice to see the old-new for > > `make allyesconfig` or equivalent. > > just like it would already do, no? I can try and see what happens, but > my feeling is that we won't have any change. Maybe not or maybe a small win. Depends how compiler puts / linker sees that in two cases. (Yeah, likely it should be no differences if all instances are already caught by linker) -- With Best Regards, Andy Shevchenko
Re: [Nouveau] [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, Nov 23, 2020 at 10:39 PM James Bottomley wrote: > On Mon, 2020-11-23 at 19:56 +0100, Miguel Ojeda wrote: > > On Mon, Nov 23, 2020 at 4:58 PM James Bottomley > > wrote: ... > > But if we do the math, for an author, at even 1 minute per line > > change and assuming nothing can be automated at all, it would take 1 > > month of work. For maintainers, a couple of trivial lines is noise > > compared to many other patches. > > So you think a one line patch should take one minute to produce ... I > really don't think that's grounded in reality. I suppose a one line > patch only takes a minute to merge with b4 if no-one reviews or tests > it, but that's not really desirable. In my practice most of the one line patches were either to fix or to introduce quite interesting issues. 1 minute is 2-3 orders less than usually needed for such patches. That's why I don't like churn produced by people who often even didn't compile their useful contributions. -- With Best Regards, Andy Shevchenko ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
[PATCH v1 1/1] treewide: Align match_string() with sysfs_match_string()
Make two APIs look similar. Hence convert match_string() to be
a 2-argument macro. In order to avoid unneeded churn, convert
all users as well. There is no functional change intended.
Signed-off-by: Andy Shevchenko
---
Compile tested with `make allyesconfig` and `make allmodconfig`
on x86_64, arm, aarch64, powerpc64 (8 builds total).
I guess the best is to apply it to Linus' tree directly.
And now it seems a good timing as there are no new users
of this API either in v6.10-rcX, or in Linux Next.
But if you think differently, tell me.
arch/powerpc/xmon/xmon.c | 5 ++--
arch/x86/kernel/cpu/mtrr/if.c | 4 +--
crypto/asymmetric_keys/pkcs7_verify.c | 4 +--
drivers/acpi/scan.c | 4 +--
drivers/ata/pata_hpt366.c | 2 +-
drivers/ata/pata_hpt37x.c | 2 +-
drivers/base/property.c | 6 ++--
drivers/char/ipmi/ipmi_msghandler.c | 2 +-
drivers/char/ipmi/ipmi_si_hardcode.c | 2 +-
drivers/clk/bcm/clk-bcm2835.c | 4 +--
drivers/clk/rockchip/clk.c| 4 +--
drivers/clk/tegra/clk-tegra124-emc.c | 7 ++---
drivers/cpufreq/amd-pstate.c | 4 +--
drivers/cpufreq/intel_pstate.c| 2 +-
.../intel/qat/qat_common/adf_cfg_services.c | 5 ++--
.../gpu/drm/drm_panel_orientation_quirks.c| 2 +-
drivers/gpu/drm/i915/display/intel_pipe_crc.c | 2 +-
drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 4 +--
drivers/gpu/drm/nouveau/dispnv50/crc.c| 2 +-
drivers/hwmon/ltc4282.c | 14 -
drivers/hwmon/nct6775-platform.c | 6 ++--
drivers/hwtracing/intel_th/msu.c | 2 +-
drivers/i2c/busses/i2c-i801.c | 4 +--
drivers/leds/leds-sun50i-a100.c | 2 +-
drivers/mfd/omap-usb-host.c | 2 +-
drivers/mmc/host/sdhci-xenon-phy.c| 13 -
drivers/mtd/nand/raw/nand_macronix.c | 10 ++-
.../net/ethernet/chelsio/cxgb4/cudbg_lib.c| 6 ++--
.../net/wireless/intel/iwlwifi/mvm/debugfs.c | 2 +-
drivers/pci/pcie/aer.c| 2 +-
drivers/phy/mediatek/phy-mtk-tphy.c | 8 ++---
drivers/phy/tegra/xusb.c | 4 +--
drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 6 ++--
drivers/pinctrl/pinmux.c | 6 ++--
drivers/platform/x86/hp/hp-wmi.c | 29 +++
drivers/platform/x86/msi-ec.c | 4 +--
drivers/power/supply/ab8500_btemp.c | 2 +-
drivers/power/supply/ab8500_chargalg.c| 2 +-
drivers/power/supply/ab8500_charger.c | 2 +-
drivers/power/supply/ab8500_fg.c | 2 +-
drivers/staging/gdm724x/gdm_tty.c | 5 ++--
.../int340x_thermal/processor_thermal_rfim.c | 4 +--
.../processor_thermal_wt_req.c| 2 +-
drivers/usb/common/common.c | 8 ++---
drivers/usb/dwc3/dwc3-rtk.c | 2 +-
drivers/usb/typec/class.c | 14 -
drivers/usb/typec/tipd/core.c | 3 +-
drivers/video/fbdev/pxafb.c | 4 +--
fs/bcachefs/compress.c| 2 +-
fs/bcachefs/opts.c| 4 +--
fs/bcachefs/util.c| 4 +--
fs/ubifs/auth.c | 8 ++---
include/linux/string.h| 12 +++-
kernel/cgroup/rdma.c | 2 +-
kernel/sched/debug.c | 2 +-
kernel/trace/trace.c | 4 +--
kernel/trace/trace_osnoise.c | 4 +--
lib/dynamic_debug.c | 5 ++--
lib/string_helpers.c | 6 ++--
mm/mempolicy.c| 4 +--
mm/vmpressure.c | 4 +--
security/apparmor/lsm.c | 9 +++---
security/integrity/ima/ima_main.c | 2 +-
security/integrity/ima/ima_policy.c | 2 +-
sound/firewire/oxfw/oxfw.c| 2 +-
sound/pci/oxygen/oxygen_mixer.c | 2 +-
sound/soc/codecs/max98088.c | 2 +-
sound/soc/codecs/max98095.c | 2 +-
sound/soc/soc-dapm.c | 5 ++--
69 files changed, 150 insertions(+), 174 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index bd4813bad317..f479cc62674a 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -3478,8 +3478,7 @@ skipbl(void)
return c;
}
-#define N_PTREGS 44
-static const char *regnames[N_PTREGS] = {
+static const char *regnames[] = {
"r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
"r8",
Re: [Nouveau] [PATCH v1] ACPI: Switch to use generic UUID API
On Thu, 2017-05-04 at 12:21 +0300, Andy Shevchenko wrote: > acpi_evaluate_dsm() and friends take a pointer to a raw buffer of 16 > bytes. Instead we convert them to use uuid_le type. At the same time > we > convert current users. > > acpi_str_to_uuid() becomes useless after the conversion and it's safe > to > get rid of it. > > The conversion fixes a potential bug in int340x_thermal as well since > we have to use memcmp() on binary data. > > Cc: Rafael J. Wysocki > Cc: Mika Westerberg > Cc: Borislav Petkov > Cc: Dan Williams > Cc: Amir Goldstein > Cc: Jarkko Sakkinen > Cc: Jani Nikula > Cc: Ben Skeggs > Cc: Benjamin Tissoires > Cc: Joerg Roedel > Cc: Adrian Hunter > Cc: Yisen Zhuang > Cc: Bjorn Helgaas > Cc: Zhang Rui > Cc: Felipe Balbi > Cc: Mathias Nyman > Cc: Heikki Krogerus > Cc: Liam Girdwood > Cc: Mark Brown > Signed-off-by: Andy Shevchenko Thank you everyone who gave a tag to this. I'm going to split and rebase on top of Christoph's branch http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/uuid-type s followed by changing types and API calls accordingly (without changing a logic!). So, I would like to keep tags in place. If there is any objection, speak up now! Thanks! -- Andy Shevchenko Intel Finland Oy ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 1/3] backlight: always select BACKLIGHT_LCD_SUPPORT for BACKLIGHT_CLASS_DEVICE
On Wed, Jul 26, 2017 at 4:53 PM, Arnd Bergmann wrote: > randconfig builds occasionally produce this Kconfig warning: > > warning: (DRM_RADEON && DRM_AMDGPU && DRM_NOUVEAU && DRM_I915 && DRM_GMA500 > && DRM_SHMOBILE && DRM_TILCDC && DRM_FSL_DCU && DRM_TINYDRM && > DRM_PARADE_PS8622 && FB_BACKLIGHT && FB_ARMCLCD && FB_MX3 && USB_APPLEDISPLAY > && FB_OLPC_DCON && ACPI_CMPC && SAMSUNG_Q10) selects BACKLIGHT_CLASS_DEVICE > which has unmet direct dependencies (HAS_IOMEM && BACKLIGHT_LCD_SUPPORT) > > It turns out that amost all users of BACKLIGHT_CLASS_DEVICE also select > BACKLIGHT_LCD_SUPPORT, but not all of them do. This makes the remaining > ones behave like the others. > > It would probably be best to rework the way those two options related > entirely, but for now this takes the simpler and safer approach to > fix the warnings without introducing regressions. > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 80b87954f6dd..e0ca673bf564 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -785,6 +785,7 @@ config ACPI_CMPC > depends on RFKILL || RFKILL=n > select INPUT > select BACKLIGHT_CLASS_DEVICE > + select BACKLIGHT_LCD_SUPPORT > default n > help > Support for Intel Classmate PC ACPI devices, including some > @@ -1000,6 +1001,7 @@ config SAMSUNG_Q10 > tristate "Samsung Q10 Extras" > depends on ACPI > select BACKLIGHT_CLASS_DEVICE > + select BACKLIGHT_LCD_SUPPORT > ---help--- > This driver provides support for backlight control on Samsung Q10 > and related laptops, including Dell Latitude X200. Acked-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 21/33] drm/nouveau: use match_string() helper
On Mon, May 21, 2018 at 2:57 PM, Yisheng Xie wrote:
> match_string() returns the index of an array for a matching string,
> which can be used intead of open coded variant.
> if (nouveau_tv_norm) {
> + i = match_string(nv17_tv_norm_names,
> +num_tv_norms, nouveau_tv_norm);
Same comment for logical split, 2nd parameter looks better on the previous line.
> + if (i >= 0)
> + tv_enc->tv_norm = i;
> + else
> NV_WARN(drm, "Invalid TV norm setting \"%s\"\n",
> nouveau_tv_norm);
I would rather go with
if (i < 0)
NV_WARN
else
...
> }
--
With Best Regards,
Andy Shevchenko
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v2 11/21] drm/nouveau: use match_string() helper
On Thu, May 31, 2018 at 2:11 PM, Yisheng Xie wrote:
> match_string() returns the index of an array for a matching string,
> which can be used instead of open coded variant.
>
Reviewed-by: Andy Shevchenko
> Cc: Ben Skeggs
> Cc: David Airlie
> Cc: dri-de...@lists.freedesktop.org
> Cc: nouveau@lists.freedesktop.org
> Signed-off-by: Yisheng Xie
> ---
> v2:
> - handle err case before normal case - per Andy
>
> drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 13 +
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c
> b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c
> index 6d99f11..67ba2ac 100644
> --- a/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c
> +++ b/drivers/gpu/drm/nouveau/dispnv04/tvnv17.c
> @@ -644,16 +644,13 @@ static int nv17_tv_create_resources(struct drm_encoder
> *encoder,
> int i;
>
> if (nouveau_tv_norm) {
> - for (i = 0; i < num_tv_norms; i++) {
> - if (!strcmp(nv17_tv_norm_names[i], nouveau_tv_norm)) {
> - tv_enc->tv_norm = i;
> - break;
> - }
> - }
> -
> - if (i == num_tv_norms)
> + i = match_string(nv17_tv_norm_names,
> +num_tv_norms, nouveau_tv_norm);
> + if (i < 0)
> NV_WARN(drm, "Invalid TV norm setting \"%s\"\n",
> nouveau_tv_norm);
> + else
> + tv_enc->tv_norm = i;
> }
>
> drm_mode_create_tv_properties(dev, num_tv_norms, nv17_tv_norm_names);
> --
> 1.7.12.4
>
--
With Best Regards,
Andy Shevchenko
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH 0/5] Thunderbolt GPU fixes
On Fri, Mar 10, 2017 at 2:07 PM, Lukas Wunner wrote:
> On Thu, Mar 09, 2017 at 04:03:47PM +0100, Daniel Vetter wrote:
>> On Fri, Feb 24, 2017 at 08:19:45PM +0100, Lukas Wunner wrote:
>> > Fix Thunderbolt-related issues in apple-gmux and vga_switcheroo:
>> >
>> > Patch [1/5] ("Recognize Thunderbolt devices") has already been subjected
>> > to a fair amount of scrutiny over at linux-pci@, I've submitted it 5 times
>> > total since May 2016. With luck it may be in ack-able shape now.
>> >
>> > Patch [2/5] amends apple-gmux to handle combined DP/Thunderbolt ports
>> > properly on newer MacBook Pros.
>> >
>> > Patches [3/5] to [5/5] avoid registering external Thunderbolt GPUs with
>> > vga_switcheroo: Dave Airlie designed vga_switcheroo to register GPUs
>> > unconditionally. So if a desktop box has multiple GPUs, vga_switcheroo
>> > will see more than one discrete GPU but that's not a problem because on
>> > desktop boxes no handler is registered and thus vga_switcheroo_enable()
>> > is never called. Hybrid graphics laptops on the other hand do register
>> > a handler, but are assumed to never register more than one discrete GPU.
>> > However once a Thunderbolt eGPU is attached to a hybrid graphics laptop,
>> > that assumption is no longer true and things go south when vga_switcheroo
>> > runtime suspends the external discrete GPU and then calls the handler to
>> > cut power to the internal discrete GPU. The driver for the internal GPU
>> > will sit there puzzled and typically cause a lockup.
> [snip]
>> > I've pushed the present series to GitHub in case anyone prefers reviewing
>> > it in a GUI:
>> > https://github.com/l1k/linux/commits/thunderbolt_gpu_v1
>>
>> For merging, should I smash this all into drm-misc? The only thing outside
>> is the apple-gmux driver ...
>
> Merging through drm-misc would be lovely. However I've prepared a v2 of
> patch [1/5] to address Bjorn's comments (amended the commit message and a
> code comment). I'll respin the series this evening and include the acks
> I've collected so far.
>
> @Darren & Andy:
> Please ack patch [5/5] of this series, barring any objections.
>
> I'll move the apple-gmux patch to the end of the series, so merging that
> one can be postponed until Darren and Andy find the time to look at it.
Yeah, please resend. It's probably buried in the pile of deleted mails.
Though I have still patchwork ticket.
Okay, I have looked at it and I'm fine with the code. Hope this
doesn't break anything.
Acked-by: Andy Shevchenko
--
With Best Regards,
Andy Shevchenko
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
[Nouveau] [PATCH v1] ACPI: Switch to use generic UUID API
acpi_evaluate_dsm() and friends take a pointer to a raw buffer of 16
bytes. Instead we convert them to use uuid_le type. At the same time we
convert current users.
acpi_str_to_uuid() becomes useless after the conversion and it's safe to
get rid of it.
The conversion fixes a potential bug in int340x_thermal as well since
we have to use memcmp() on binary data.
Cc: Rafael J. Wysocki
Cc: Mika Westerberg
Cc: Borislav Petkov
Cc: Dan Williams
Cc: Amir Goldstein
Cc: Jarkko Sakkinen
Cc: Jani Nikula
Cc: Ben Skeggs
Cc: Benjamin Tissoires
Cc: Joerg Roedel
Cc: Adrian Hunter
Cc: Yisen Zhuang
Cc: Bjorn Helgaas
Cc: Zhang Rui
Cc: Felipe Balbi
Cc: Mathias Nyman
Cc: Heikki Krogerus
Cc: Liam Girdwood
Cc: Mark Brown
Signed-off-by: Andy Shevchenko
---
drivers/acpi/acpi_extlog.c | 10 +++---
drivers/acpi/bus.c | 29 ++--
drivers/acpi/nfit/core.c | 40 +++---
drivers/acpi/nfit/nfit.h | 3 +-
drivers/acpi/utils.c | 4 +--
drivers/char/tpm/tpm_crb.c | 9 +++--
drivers/char/tpm/tpm_ppi.c | 20 +--
drivers/gpu/drm/i915/intel_acpi.c | 14 +++-
drivers/gpu/drm/nouveau/nouveau_acpi.c | 20 +--
drivers/gpu/drm/nouveau/nvkm/subdev/mxm/base.c | 9 +++--
drivers/hid/i2c-hid/i2c-hid.c | 9 +++--
drivers/iommu/dmar.c | 11 +++---
drivers/mmc/host/sdhci-pci-core.c | 9 +++--
drivers/net/ethernet/hisilicon/hns/hns_dsaf_misc.c | 15
drivers/pci/pci-acpi.c | 11 +++---
drivers/pci/pci-label.c| 4 +--
drivers/thermal/int340x_thermal/int3400_thermal.c | 8 ++---
drivers/usb/dwc3/dwc3-pci.c| 6 ++--
drivers/usb/host/xhci-pci.c| 9 +++--
drivers/usb/misc/ucsi.c| 2 +-
drivers/usb/typec/typec_wcove.c| 4 +--
include/acpi/acpi_bus.h| 9 ++---
include/linux/acpi.h | 4 +--
include/linux/pci-acpi.h | 2 +-
sound/soc/intel/skylake/skl-nhlt.c | 7 ++--
tools/testing/nvdimm/test/iomap.c | 2 +-
tools/testing/nvdimm/test/nfit.c | 2 +-
27 files changed, 116 insertions(+), 156 deletions(-)
diff --git a/drivers/acpi/acpi_extlog.c b/drivers/acpi/acpi_extlog.c
index 502ea4dc2080..69d6140b6afa 100644
--- a/drivers/acpi/acpi_extlog.c
+++ b/drivers/acpi/acpi_extlog.c
@@ -182,17 +182,17 @@ static int extlog_print(struct notifier_block *nb,
unsigned long val,
static bool __init extlog_get_l1addr(void)
{
- u8 uuid[16];
+ uuid_le uuid;
acpi_handle handle;
union acpi_object *obj;
- acpi_str_to_uuid(extlog_dsm_uuid, uuid);
-
+ if (uuid_le_to_bin(extlog_dsm_uuid, &uuid))
+ return false;
if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
return false;
- if (!acpi_check_dsm(handle, uuid, EXTLOG_DSM_REV, 1 << EXTLOG_FN_ADDR))
+ if (!acpi_check_dsm(handle, &uuid, EXTLOG_DSM_REV, 1 << EXTLOG_FN_ADDR))
return false;
- obj = acpi_evaluate_dsm_typed(handle, uuid, EXTLOG_DSM_REV,
+ obj = acpi_evaluate_dsm_typed(handle, &uuid, EXTLOG_DSM_REV,
EXTLOG_FN_ADDR, NULL, ACPI_TYPE_INTEGER);
if (!obj) {
return false;
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 784bda663d16..e8130a4873e9 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -196,42 +196,19 @@ static void acpi_print_osc_error(acpi_handle handle,
pr_debug("\n");
}
-acpi_status acpi_str_to_uuid(char *str, u8 *uuid)
-{
- int i;
- static int opc_map_to_uuid[16] = {6, 4, 2, 0, 11, 9, 16, 14, 19, 21,
- 24, 26, 28, 30, 32, 34};
-
- if (strlen(str) != 36)
- return AE_BAD_PARAMETER;
- for (i = 0; i < 36; i++) {
- if (i == 8 || i == 13 || i == 18 || i == 23) {
- if (str[i] != '-')
- return AE_BAD_PARAMETER;
- } else if (!isxdigit(str[i]))
- return AE_BAD_PARAMETER;
- }
- for (i = 0; i < 16; i++) {
- uuid[i] = hex_to_bin(str[opc_map_to_uuid[i]]) << 4;
- uuid[i] |= hex_to_bin(str[opc_map_to_uuid[i] + 1]);
- }
- return AE_OK;
-}
-EXPORT_SYMBOL_GPL(acpi_str_to_uuid);
-
acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
{
acpi_status status;
struct acpi_object_list input;
union acpi_object in_params[4];
union a
Re: [Nouveau] [PATCH v1] ACPI: Switch to use generic UUID API
On Fri, 2017-05-05 at 10:06 +0300, Amir Goldstein wrote: > On Fri, May 5, 2017 at 9:20 AM, Dan Williams > wrote: > > On Thu, May 4, 2017 at 2:21 AM, Andy Shevchenko > > wrote: > > > for (i = 0; i < NFIT_UUID_MAX; i++) > > > - if (memcmp(to_nfit_uuid(i), spa->range_guid, 16) > > > == 0) > > > + if (!uuid_le_cmp_pp(to_nfit_uuid(i), (uuid_le > > > *)spa->range_guid)) > > > > What is _cmp_pp? Why not _compare? Dan, it's a typo. In this patch it should be just ..._cmp(), which is already a part of API. > > > > I second that. > > Andy, Amir, just to be clear. This patch can be applied without any addons to an existing API. Above is just a typo due to rebase in my tree. I will replace it to just uuid_le_cmp(). > I much rather that you sort out uuid helpers in a way that will > satisfy the filesystem > needs (just provide the helpers don't need to convert filesystems > code). > The only reason I took a swing at hoisting the xfs uuid helpers is > because it didn't > seem like your proposal was going to be posted soon or wasn't going to > satisfy > the filesystems use case. > > My opinion now, is that your suggestion is probably much closer to the > real deal > than mine. > > IMO, you should acknowledge that the common use case for filesystems > is > to handle an opaque char[16] which most likely holds a uuid_be and you > should provide 'neutral' helpers to satisfy this use case. > > The simplest would be to typedef uuid_t to struct uuid_be and to name > 'neutral' > helpers' uuid_cmp/uuid_copy(uuid_t *, uuid_t *), similar to my > proposal. > I think with this semantic change, our proposals can reach common > grounds > and satisfy a wider group of users (i.e. filesystem developers). > > Christoph also suggested a similar treatment to typedef guid_t to > struct uuid_le. > I don't know the use cases enough to comment on that. We may go this way. But I wouldn't prevent current users of uuid_le to continue using it without conversion (it may be done case by case after we settle an API) So, summarize what Christoph said it will look like typedef uuid_be uuid_t; typedef uuid_le guid_t uuid_cmp() / uuid_copy() / uuid_to_bin() / etc guid_cmp() / guid_copy() / guid_to_bin() / etc Correct? Christoph? -- Andy Shevchenko Intel Finland Oy ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
