Re: [PATCH] drm/nouveau/nvif: Avoid build error due to potential integer overflows
On Sat, 2024-05-18 at 11:23 -0700, Guenter Roeck wrote:
> On 5/18/24 10:32, Kees Cook wrote:
>
[]
> > I think the INT_MAX test is actually better in this case because
> > nvif_object_ioctl()'s size argument is u32:
> >
> > ret = nvif_object_ioctl(object, args, sizeof(*args) + size, NULL);
> >
> >
> > So that could wrap around, even though the allocation may not.
> >
> > Better yet, since "sizeof(*args) + size" is repeated 3 times in the
> > function, I'd recommend:
> >
> > ...
> > u32 args_size;
> >
> > if (check_add_overflow(sizeof(*args), size, _size))
> > return -ENOMEM;
> > if (args_size > sizeof(stack)) {
> > if (!(args = kmalloc(args_size, GFP_KERNEL)))
trivia:
More typical kernel style would use separate alloc and test
args = kmalloc(args_size, GFP_KERNEL);
if (!args)
> > return -ENOMEM;
> > } else {
> > args = (void *)stack;
> > }
> > ...
> > ret = nvif_object_ioctl(object, args, args_size, NULL);
> >
> > This will catch the u32 overflow to nvif_object_ioctl(), catch an
> > allocation underflow on 32-bits systems, and make the code more
> > readable. :)
> >
>
> Makes sense. I'll change that and send v2.
>
> Thanks,
> Guenter
>
>
Re: [PATCH 1/4] gpu/drm: Add SPDX-license-Identifier tag
On Tue, 2024-04-02 at 22:43 +, Nicolas Devos wrote: > This commit fixes following checkpatch warning: > WARNING: Missing or malformed SPDX-License-Identifier tag NAK without specific agreement from Intel. The existing license in the file is neither GPL nor MIT > > Signed-off-by: Nicolas Devos > --- > drivers/gpu/drm/drm_connector.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index b0516505f7ae..40350256b1f6 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1,3 +1,4 @@ > +// SPDX-License-Identifier: GPL-2.0 or MIT > /* > * Copyright (c) 2016 Intel Corporation > *
Re: [PATCH v7 3/9] drm/plane: Add drm_for_each_primary_visible_plane macro
On Mon, 2024-01-08 at 11:24 +0100, Jocelyn Falempe wrote: > Hi checkpatch maintainers, > > This patch gives me the following checkpatch error: > > ERROR: Macros with complex values should be enclosed in parentheses > #30: FILE: include/drm/drm_plane.h:959: > +#define drm_for_each_primary_visible_plane(plane, dev) \ > + list_for_each_entry((plane), &(dev)->mode_config.plane_list, head) \ > + for_each_if((plane)->type == DRM_PLANE_TYPE_PRIMARY && \ > + (plane)->state && \ > + (plane)->state->fb && \ > + (plane)->state->visible) > > total: 1 errors, 0 warnings, 21 lines checked > > I think this requirement cannot work when you use list_for_each kind of > macros. > Do you have any suggestion ? > checkpatch is a brainless regex script. Ignore it when it's stupid.
Re: [PATCH v5 22/22] checkpatch: reword long-line warn about commit-msg
On Tue, 2023-08-01 at 17:35 -0600, Jim Cromie wrote:
> Reword the warning to complain about line length 1st, since thats
> whats actually tested.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3272,7 +3272,7 @@ sub process {
> # A Fixes:, link or signature tag line
> $commit_log_possible_stack_dump)) {
> WARN("COMMIT_LOG_LONG_LINE",
> - "Possible unwrapped commit description (prefer a
> maximum 75 chars per line)\n" . $herecurr);
> + "Prefer a maximum 75 chars per line (possible
> unwrapped commit description?)\n" . $herecurr);
> $commit_log_long_line = 1;
> }
I don't think this adds any clarity. Anyone else?
Re: [PATCH v4 0/5] docs & checkpatch: allow Closes tags with links
On Mon, 2023-04-03 at 18:23 +0200, Matthieu Baerts wrote: > Since v6.3, checkpatch.pl now complains about the use of "Closes:" tags > followed by a link [1]. It also complains if a "Reported-by:" tag is > followed by a "Closes:" one [2]. All these patches seems sensible, thanks. Assuming Linus approves the use of "Closes:" Acked-by: Joe Perches > As detailed in the first patch, this "Closes:" tag is used for a bit of > time, mainly by DRM and MPTCP subsystems. It is used by some bug > trackers to automate the closure of issues when a patch is accepted. > It is even planned to use this tag with bugzilla.kernel.org [3]. > > The first patch updates the documentation to explain what is this > "Closes:" tag and how/when to use it. The second patch modifies > checkpatch.pl to stop complaining about it. > > The DRM maintainers and their mailing list have been added in Cc as they > are probably interested by these two patches as well. > > [1] > https://lore.kernel.org/all/3b036087d80b8c0e07a46a1dbaaf4ad0d018f8d5.1674217480.git.li...@leemhuis.info/ > [2] > https://lore.kernel.org/all/bb5dfd55ea2026303ab2296f4a6df3da7dd64006.1674217480.git.li...@leemhuis.info/ > [3] > https://lore.kernel.org/linux-doc/20230315181205.f3av7h6owqzzw64p@meerkat.local/ > > Signed-off-by: Matthieu Baerts > --- > Note: After having re-read the comments from the v1, it is still unclear > to me if this "Closes:" can be accepted or not. But because it seems > that the future Bugzilla bot for kernel.org and regzbot would like to > use it as well, I'm sending here new versions. I'm sorry if I > misunderstood the comments from v1. Please tell me if I did. > > Changes in v4: > - Patches 1/5, 3/5 and 4/5 have been added to ask using the "Closes" tag > instead of the "Link" one for any bug reports. (Thorsten) > - The Fixes tags have been removed from patch 4/5. (Joe) > - The "Reported-by being followed by a link tag" check is now only > looking for the tag, not the URL which is done elsewhere in patch 5/5. > (Thorsten) > - A new patch has been added to fix a small issues in checkpatch.pl when > checking if "Reported-by:" tag is on the last line. > - Link to v3: > https://lore.kernel.org/r/20230314-doc-checkpatch-closes-tag-v3-0-d1bdcf31c...@tessares.net > > Changes in v3: > - Patch 1/4 now allow using the "Closes" tag with any kind of bug > reports, as long as the link is public. (Thorsten) > - The former patch 2/2 has been split in two: first to use a list for > the different "link" tags (Joe). Then to allow the 'Closes' tag. > - A new patch has been added to let checkpatch.pl checking if "Closes" > and "Links" are used with a URL. > - Link to v2: > https://lore.kernel.org/r/20230314-doc-checkpatch-closes-tag-v2-0-f4a417861...@tessares.net > > Changes in v2: > - The text on patch 1/2 has been reworked thanks to Jon, Bagas and > Thorsten. See the individual changelog on the patch for more details. > - Private bug trackers and invalid URLs are clearly marked as forbidden > to avoid being misused. (Linus) > - Rebased on top of Linus' repo. > - Link to v1: > https://lore.kernel.org/r/20230314-doc-checkpatch-closes-tag-v1-0-1b83072e9...@tessares.net > > --- > Matthieu Baerts (5): > docs: process: allow Closes tags with links > checkpatch: don't print the next line if not defined > checkpatch: use a list of "link" tags > checkpatch: allow Closes tags with links > checkpatch: check for misuse of the link tags > > Documentation/process/5.Posting.rst | 22 ++ > Documentation/process/submitting-patches.rst | 26 +++-- > scripts/checkpatch.pl| 43 > ++-- > 3 files changed, 70 insertions(+), 21 deletions(-) > --- > base-commit: 7e364e56293bb98cae1b55fd835f5991c4e96e7d > change-id: 20230314-doc-checkpatch-closes-tag-1731b57556b1 > > Best regards,
Re: [PATCH v3 3/4] checkpatch: allow Closes tags with links
On Thu, 2023-03-30 at 20:13 +0200, Matthieu Baerts wrote:
> As a follow-up of a previous patch modifying the documentation to
> allow using the "Closes:" tag, checkpatch.pl is updated accordingly.
>
> checkpatch.pl now no longer complain when the "Closes:" tag is used by
> itself or after the "Reported-by:" tag.
>
> Fixes: 76f381bb77a0 ("checkpatch: warn when unknown tags are used for links")
> Fixes: d7f1d71e5ef6 ("checkpatch: warn when Reported-by: is not followed by
> Link:")
I don't think this _fixes_ anything.
I believe it's merely a new capability.
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/373
> Signed-off-by: Matthieu Baerts
> ---
> v3:
> - split into 2 patches: the previous one adds a list with all the
>"link" tags. This one only allows the "Closes" tag. (Joe Perches)
> - "Closes" is no longer printed between parenthesis. (Thorsten
>Leemhuis)
> ---
> scripts/checkpatch.pl | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 9d092ff4fc16..ca58c734ff22 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -620,7 +620,7 @@ our $signature_tags = qr{(?xi:
> Cc:
> )};
>
> -our @link_tags = qw(Link);
> +our @link_tags = qw(Link Closes);
>
> #Create a search and print patterns for all these strings to be used
> directly below
> our $link_tags_search = "";
>
Re: [PATCH v2 2/2] checkpatch: allow Closes tags with links
On Fri, 2023-03-24 at 19:52 +0100, Matthieu Baerts wrote:
> As a follow-up of the previous patch modifying the documentation to
> allow using the "Closes:" tag, checkpatch.pl is updated accordingly.
>
> checkpatch.pl now mentions the "Closes:" tag between brackets to express
> the fact it should be used only if it makes sense.
>
> While at it, checkpatch.pl will not complain if the "Closes" tag is used
> with a "long" line, similar to what is done with the "Link" tag.
>
> Fixes: 76f381bb77a0 ("checkpatch: warn when unknown tags are used for links")
> Fixes: d7f1d71e5ef6 ("checkpatch: warn when Reported-by: is not followed by
> Link:")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/373
> Signed-off-by: Matthieu Baerts
> ---
> scripts/checkpatch.pl | 16
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index bd44d12965c9..d6376e0b68cc 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3158,14 +3158,14 @@ sub process {
> }
> }
>
> -# check if Reported-by: is followed by a Link:
> +# check if Reported-by: is followed by a Link: (or Closes:) tag
> if ($sign_off =~ /^reported(?:|-and-tested)-by:$/i) {
> if (!defined $lines[$linenr]) {
> WARN("BAD_REPORTED_BY_LINK",
> - "Reported-by: should be
> immediately followed by Link: to the report\n" . $herecurr .
> $rawlines[$linenr] . "\n");
> - } elsif ($rawlines[$linenr] !~
> m{^link:\s*https?://}i) {
> + "Reported-by: should be
> immediately followed by Link: (or Closes:) to the report\n" . $herecurr .
> $rawlines[$linenr] . "\n");
> + } elsif ($rawlines[$linenr] !~
> m{^(link|closes):\s*https?://}i) {
Please do not use an unnecessary capture group.
(?:link|closes)
And because it's somewhat likely that _more_ of these keywords
could be added, perhaps use some array like deprecated_apis
> WARN("BAD_REPORTED_BY_LINK",
> - "Reported-by: should be
> immediately followed by Link: with a URL to the report\n" . $herecurr .
> $rawlines[$linenr] . "\n");
> + "Reported-by: should be
> immediately followed by Link: (or Closes:) with a URL to the report\n" .
> $herecurr . $rawlines[$linenr] . "\n");
> }
> }
> }
> @@ -3250,8 +3250,8 @@ sub process {
> # file delta changes
> $line =~ /^\s*(?:[\w\.\-\+]*\/)++[\w\.\-\+]+:/ ||
> # filename then :
> - $line =~ /^\s*(?:Fixes:|Link:|$signature_tags)/i ||
> - # A Fixes: or Link: line or signature
> tag line
> + $line =~ /^\s*(?:Fixes:|Link:|Closes:|$signature_tags)/i
> ||
> + # A Fixes:, Link:, Closes: or signature
> tag line
> $commit_log_possible_stack_dump)) {
> WARN("COMMIT_LOG_LONG_LINE",
>"Possible unwrapped commit description (prefer a
> maximum 75 chars per line)\n" . $herecurr);
> @@ -3266,13 +3266,13 @@ sub process {
>
> # Check for odd tags before a URI/URL
> if ($in_commit_log &&
> - $line =~ /^\s*(\w+):\s*http/ && $1 ne 'Link') {
> + $line =~ /^\s*(\w+):\s*http/ && $1 ne 'Link' && $1 ne
> 'Closes') {
> if ($1 =~ /^v(?:ersion)?\d+/i) {
> WARN("COMMIT_LOG_VERSIONING",
>"Patch version information should be after
> the --- line\n" . $herecurr);
> } else {
> WARN("COMMIT_LOG_USE_LINK",
> - "Unknown link reference '$1:', use 'Link:'
> instead\n" . $herecurr);
> + "Unknown link reference '$1:', use 'Link:'
> (or 'Closes:') instead\n" . $herecurr);
> }
> }
>
>
Re: [PATCH] drm/amd/display: Simplify same effect if/else blocks
On Fri, 2023-03-03 at 15:35 -0500, Harry Wentland wrote: > Actually I was wrong. Too many similar-looking snippets in this > function made me look at the wrong thing. This change is fine and > Reviewed-by: Harry Wentland
Re: [PATCH] drm/amd/display: Simplify same effect if/else blocks
On Sun, 2023-01-15 at 15:30 +0530, Deepak R Varma wrote:
> The if / else block code has same effect irrespective of the logical
> evaluation. Hence, simply the implementation by removing the unnecessary
> conditional evaluation. While at it, also fix the long line checkpatch
> complaint. Issue identified using cond_no_effect.cocci Coccinelle
> semantic patch script.
>
> Signed-off-by: Deepak R Varma
> ---
> Please note: The proposed change is compile tested only. If there are any
> inbuilt test cases that I should run for further verification, I will
> appreciate
> guidance about it. Thank you.
Preface: I do not know the code.
Perhaps Rodrigo Siqueira made a copy/paste error submitting the code for
commit 9114b55fabae ("drm/amd/display: Fix SubVP control flow in the MPO
context")
as the code prior to this change is identical.
Perhaps one of the false uses should be true or dependent on the
interdependent_update_lock state.
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c
> b/drivers/gpu/drm/amd/display/dc/core/dc.c
[]
> @@ -3470,14 +3470,9 @@ static void commit_planes_for_stream(struct dc *dc,
> /* Since phantom pipe programming is moved to
> post_unlock_program_front_end,
>* move the SubVP lock to after the phantom pipes have been
> setup
>*/
> - if (should_lock_all_pipes &&
> dc->hwss.interdependent_update_lock) {
> - if (dc->hwss.subvp_pipe_control_lock)
> - dc->hwss.subvp_pipe_control_lock(dc, context,
> false, should_lock_all_pipes, NULL, subvp_prev_use);
> - } else {
> - if (dc->hwss.subvp_pipe_control_lock)
> - dc->hwss.subvp_pipe_control_lock(dc, context,
> false, should_lock_all_pipes, NULL, subvp_prev_use);
> - }
> -
Perhaps something like:
if (dc->hwss.subvp_pipe_control_lock)
dc->hwss.subvp_pipe_control_lock(dc, context,
should_lock_all_pipes
&&
dc->hwss.interdependent_update_lock,
should_lock_all_pipes,
NULL, subvp_prev_use);
> + if (dc->hwss.subvp_pipe_control_lock)
> + dc->hwss.subvp_pipe_control_lock(dc, context, false,
> should_lock_all_pipes,
> + NULL, subvp_prev_use);
> return;
> }
>
Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Wed, 2022-10-12 at 21:29 +, David Laight wrote: > From: Joe Perches > > Sent: 12 October 2022 20:17 > > > > On Wed, 2022-10-05 at 23:48 +0200, Jason A. Donenfeld wrote: > > > The prandom_u32() function has been a deprecated inline wrapper around > > > get_random_u32() for several releases now, and compiles down to the > > > exact same code. Replace the deprecated wrapper with a direct call to > > > the real function. > > [] > > > diff --git a/drivers/infiniband/hw/cxgb4/cm.c > > > b/drivers/infiniband/hw/cxgb4/cm.c > > [] > > > @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep) > > > >com.remote_addr; > > > int ret; > > > enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type; > > > - u32 isn = (prandom_u32() & ~7UL) - 1; > > > + u32 isn = (get_random_u32() & ~7UL) - 1; > > > > trivia: > > > > There are somewhat odd size mismatches here. > > > > I had to think a tiny bit if random() returned a value from 0 to 7 > > and was promoted to a 64 bit value then truncated to 32 bit. > > > > Perhaps these would be clearer as ~7U and not ~7UL > > That makes no difference - the compiler will generate the same code. True, more or less. It's more a question for the reader. > The real question is WTF is the code doing? True. > The '& ~7u' clears the bottom 3 bits. > The '- 1' then sets the bottom 3 bits and decrements the > (random) high bits. Right. > So is the same as get_random_u32() | 7. True, it's effectively the same as the upper 29 bits are random anyway and the bottom 3 bits are always set. > But I bet the coder had something else in mind. Likely. And it was also likely copy/pasted a few times.
Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Wed, 2022-10-05 at 23:48 +0200, Jason A. Donenfeld wrote:
> The prandom_u32() function has been a deprecated inline wrapper around
> get_random_u32() for several releases now, and compiles down to the
> exact same code. Replace the deprecated wrapper with a direct call to
> the real function.
[]
> diff --git a/drivers/infiniband/hw/cxgb4/cm.c
> b/drivers/infiniband/hw/cxgb4/cm.c
[]
> @@ -734,7 +734,7 @@ static int send_connect(struct c4iw_ep *ep)
> >com.remote_addr;
> int ret;
> enum chip_type adapter_type = ep->com.dev->rdev.lldi.adapter_type;
> - u32 isn = (prandom_u32() & ~7UL) - 1;
> + u32 isn = (get_random_u32() & ~7UL) - 1;
trivia:
There are somewhat odd size mismatches here.
I had to think a tiny bit if random() returned a value from 0 to 7
and was promoted to a 64 bit value then truncated to 32 bit.
Perhaps these would be clearer as ~7U and not ~7UL
> struct net_device *netdev;
> u64 params;
>
> @@ -2469,7 +2469,7 @@ static int accept_cr(struct c4iw_ep *ep, struct sk_buff
> *skb,
> }
>
> if (!is_t4(adapter_type)) {
> - u32 isn = (prandom_u32() & ~7UL) - 1;
> + u32 isn = (get_random_u32() & ~7UL) - 1;
etc...
drivers/infiniband/hw/cxgb4/cm.c: u32 isn = (prandom_u32() & ~7UL) - 1;
drivers/infiniband/hw/cxgb4/cm.c: u32 isn = (prandom_u32() &
~7UL) - 1;
drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c:rpl5->iss =
cpu_to_be32((prandom_u32() & ~7UL) - 1);
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() &
~7UL) - 1;
drivers/scsi/cxgbi/cxgb4i/cxgb4i.c: u32 isn = (prandom_u32() &
~7UL) - 1;
drivers/target/iscsi/cxgbit/cxgbit_cm.c:rpl5->iss =
cpu_to_be32((prandom_u32() & ~7UL) - 1);
Re: [PATCH v9 09/10] leds: flash: mt6370: Add MediaTek MT6370 flashlight support
On Fri, 2022-09-23 at 14:34 -0700, Han Jingoo wrote: > On Wed, Sep 21, 2022 Andy Shevchenko wrote: > > > > On Wed, Sep 21, 2022 at 4:48 AM ChiaEn Wu wrote: > > > On Sun, Sep 18, 2022 at 3:22 AM Han Jingoo wrote: > > > > On Mon, Aug 29, 2022 ChiaEn Wu wrote: > > > > > > > +#define MT6370_ITORCH_MIN_uA 25000 > > > > > +#define MT6370_ITORCH_STEP_uA 12500 > > > > > +#define MT6370_ITORCH_MAX_uA 40 > > > > > +#define MT6370_ITORCH_DOUBLE_MAX_uA80 > > > > > +#define MT6370_ISTRB_MIN_uA5 > > > > > +#define MT6370_ISTRB_STEP_uA 12500 > > > > > +#define MT6370_ISTRB_MAX_uA150 > > > > > +#define MT6370_ISTRB_DOUBLE_MAX_uA 300 > > > > > > > > Use upper letters as below: > > > > For microseconds (and other -seconds) the common practice (I assume > > historically) is to use upper letters, indeed. But for current it's > > more natural to use small letters for unit multiplier as it's easier > > to read and understand. I think it's fine. see: commit 22735ce857a2d9f4e6eec37c36be3fcf9d21d154 Author: Joe Perches Date: Wed Jul 3 15:05:33 2013 -0700 checkpatch: ignore SI unit CamelCase variants like "_uV" Many existing variable names use SI like variants that should be otherwise obvious and acceptable.
Re: [PATCH] drm/print: cleanup coding style in drm_print.h
On Mon, 2022-09-05 at 11:49 +0300, Jani Nikula wrote: > On Mon, 05 Sep 2022, Jani Nikula wrote: > > On Mon, 05 Sep 2022, Jingyu Wang wrote: > > > Fix everything checkpatch.pl complained about in drm_print.h > > [...] > > > > static inline void > > > -drm_vprintf(struct drm_printer *p, const char *fmt, va_list *va) > > > +drm_vprintf(struct drm_printer *p, const char *fmt, va_list * va) > > > > Checkpatch is just confused here. Look at all the other params, why > > would you add an extra space here? > > Joe, can you help me out here please, I can't figure out why checkpatch > is complaining here: > > include/drm/drm_print.h:106: CHECK:SPACING: spaces preferred around that '*' > (ctx:WxV) > #106: FILE: include/drm/drm_print.h:106: > +drm_vprintf(struct drm_printer *p, const char *fmt, va_list *va) checkpatch doesn't know what a va_list is. It's similar to the FILE typedef that also causes this output. A recent link to add an exception for 'FILE' to checkpatch: https://lore.kernel.org/all/20220902111923.1488671-1-...@digikod.net/
Re: [PATCH v2 24/39] drm/i915: dvo_sil164.c: use SPDX header
On Fri, 2022-07-15 at 17:35 -0400, Rodrigo Vivi wrote: > On Wed, Jul 13, 2022 at 09:12:12AM +0100, Mauro Carvalho Chehab wrote: > > This file is licensed with MIT license. Change its license text > > to use SPDX. > > > > Signed-off-by: Mauro Carvalho Chehab > > Reviewed-by: Rodrigo Vivi Not exactly the MIT license as it's missing "or copyright holders" > > > --- > > > > To avoid mailbombing on a large number of people, only mailing lists were > > C/C on the cover. > > See [PATCH v2 00/39] at: > > https://lore.kernel.org/all/cover.1657699522.git.mche...@kernel.org/ > > > > drivers/gpu/drm/i915/display/dvo_sil164.c | 32 +-- > > 1 file changed, 6 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/dvo_sil164.c > > b/drivers/gpu/drm/i915/display/dvo_sil164.c > > index 0dfa0a0209ff..12974f7c9dc1 100644 > > --- a/drivers/gpu/drm/i915/display/dvo_sil164.c > > +++ b/drivers/gpu/drm/i915/display/dvo_sil164.c > > @@ -1,30 +1,10 @@ > > -/** > > +// SPDX-License-Identifier: MIT > > > > -Copyright © 2006 Dave Airlie > > - > > -All Rights Reserved. > > - > > -Permission is hereby granted, free of charge, to any person obtaining a > > -copy of this software and associated documentation files (the > > -"Software"), to deal in the Software without restriction, including > > -without limitation the rights to use, copy, modify, merge, publish, > > -distribute, sub license, and/or sell copies of the Software, and to > > -permit persons to whom the Software is furnished to do so, subject to > > -the following conditions: > > - > > -The above copyright notice and this permission notice (including the > > -next paragraph) shall be included in all copies or substantial portions > > -of the Software. > > - > > -THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS > > -OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > > -MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. > > -IN NO EVENT SHALL THE AUTHOR Missing "Authors or copyright holders" > > BE LIABLE FOR > > -ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, > > -TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE > > -SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. > > - > > -**/ > > +/* > > + * Copyright © 2006 Dave Airlie > > + * > > + * All Rights Reserved. > > + */ > > > > #include "intel_display_types.h" > > #include "intel_dvo_dev.h" > > -- > > 2.36.1 > >
Re: [PATCH] staging: fbtft: fix alignment should match open parenthesis
On Sat, 2022-06-25 at 19:00 -0700, David Reaver wrote: > Fix alignment of this line of code with the previous parenthesis, as > suggested by checkpatch.pl: [] > diff --git a/drivers/staging/fbtft/fb_tinylcd.c > b/drivers/staging/fbtft/fb_tinylcd.c [] > @@ -38,7 +38,7 @@ static int init_display(struct fbtft_par *par) > write_reg(par, 0xE5, 0x00); > write_reg(par, 0xF0, 0x36, 0xA5, 0x53); > write_reg(par, 0xE0, 0x00, 0x35, 0x33, 0x00, 0x00, 0x00, > -0x00, 0x35, 0x33, 0x00, 0x00, 0x00); > + 0x00, 0x35, 0x33, 0x00, 0x00, 0x00); It's probably better to ignore the message in this case as the first argument means something and the second and subsequent are the data being written via a specific macro using NUMARGS.
Re: [PATCH] drm/nouveau/mmu: Fix a typo
On Wed, 2022-06-22 at 09:52 +0800, Zhang Jiaming wrote: > There is a typo in comments. Change 'neeed' to 'need'. [] > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c > b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c [] > @@ -280,7 +280,7 @@ nvkm_vmm_unref_ptes(struct nvkm_vmm_iter *it, bool pfn, > u32 ptei, u32 ptes) > if (desc->type == SPT && (pgt->refs[0] || pgt->refs[1])) > nvkm_vmm_unref_sptes(it, pgt, desc, ptei, ptes); > > - /* PT no longer neeed? Destroy it. */ > + /* PT no longer need? Destroy it. */ needed
Re: [PATCH v3 4/4] drm/msm/adreno: Fix up formatting
On Sat, 2022-05-28 at 18:03 +0200, Konrad Dybcio wrote:
> Leading spaces are not something checkpatch likes, and it says so when
> they are present. Use tabs consistently to indent function body and
> unwrap a 83-char-long line, as 100 is cool nowadays.
unassociated trivia:
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
[]
> @@ -199,7 +199,7 @@ static inline int adreno_is_a420(struct adreno_gpu *gpu)
>
> static inline int adreno_is_a430(struct adreno_gpu *gpu)
> {
> - return gpu->revn == 430;
> + return gpu->revn == 430;
> }
looks like these could/should return bool
> static inline int adreno_is_a506(struct adreno_gpu *gpu)
> @@ -239,7 +239,7 @@ static inline int adreno_is_a540(struct adreno_gpu *gpu)
>
> static inline int adreno_is_a618(struct adreno_gpu *gpu)
> {
> - return gpu->revn == 618;
> + return gpu->revn == 618;
> }
etc...
Re: [PATCH] drm/i915: change node clearing from memset to initialization
On Sat, 2022-04-16 at 13:48 -0700, Tom Rix wrote:
> On 4/16/22 11:33 AM, Joe Perches wrote:
> > On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
> > > In insert_mappable_node(), the parameter node is
> > > cleared late in node's use with memset.
> > > insert_mappable_node() is a singleton, called only
> > > from i915_gem_gtt_prepare() which itself is only
> > > called by i915_gem_gtt_pread() and
> > > i915_gem_gtt_pwrite_fast() where the definition of
> > > node originates.
> > >
> > > Instead of using memset, initialize node to 0 at it's
> > > definitions.
> > trivia: /it's/its/
> >
> > Only reason _not_ to do this is memset is guaranteed to
> > zero any padding that might go to userspace.
> >
> > But it doesn't seem there is any padding anyway nor is
> > the struct available to userspace.
> >
> > So this seems fine though it might increase overall code
> > size a tiny bit.
> >
> > I do have a caveat: see below:
> >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > > b/drivers/gpu/drm/i915/i915_gem.c
> > []
> > > @@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct
> > > drm_i915_gem_object *obj,
> > > goto err_ww;
> > > } else if (!IS_ERR(vma)) {
> > > node->start = i915_ggtt_offset(vma);
> > > - node->flags = 0;
> > Why is this unneeded?
>
> node = {} initializes all of node's elements to 0, including this one.
true, but could the call to insert_mappable_node combined with the
retry goto in i915_gem_gtt_prepare set this to non-zero?
Re: [PATCH] drm/i915: change node clearing from memset to initialization
On Sat, 2022-04-16 at 13:23 -0400, Tom Rix wrote:
> In insert_mappable_node(), the parameter node is
> cleared late in node's use with memset.
> insert_mappable_node() is a singleton, called only
> from i915_gem_gtt_prepare() which itself is only
> called by i915_gem_gtt_pread() and
> i915_gem_gtt_pwrite_fast() where the definition of
> node originates.
>
> Instead of using memset, initialize node to 0 at it's
> definitions.
trivia: /it's/its/
Only reason _not_ to do this is memset is guaranteed to
zero any padding that might go to userspace.
But it doesn't seem there is any padding anyway nor is
the struct available to userspace.
So this seems fine though it might increase overall code
size a tiny bit.
I do have a caveat: see below:
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
[]
> @@ -328,7 +327,6 @@ static struct i915_vma *i915_gem_gtt_prepare(struct
> drm_i915_gem_object *obj,
> goto err_ww;
> } else if (!IS_ERR(vma)) {
> node->start = i915_ggtt_offset(vma);
> - node->flags = 0;
Why is this unneeded?
from: drm_mm_insert_node_in_range which can set node->flags
__set_bit(DRM_MM_NODE_ALLOCATED_BIT, >flags);
Re: [PATCH] drm/v3d: Use kvcalloc
On Sat, 2022-03-12 at 07:26 -0800, Harshit Mogalapalli wrote:
> kvcalloc is same as kvmalloc_array + __GFP_ZERO.
[]
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
[]
> @@ -308,9 +308,8 @@ v3d_lookup_bos(struct drm_device *dev,
> return -EINVAL;
> }
>
> - job->bo = kvmalloc_array(job->bo_count,
> - sizeof(struct drm_gem_cma_object *),
> - GFP_KERNEL | __GFP_ZERO);
> + job->bo = kvcalloc(job->bo_count, sizeof(struct drm_gem_cma_object *),
> +GFP_KERNEL);
> if (!job->bo) {
> DRM_DEBUG("Failed to allocate validated BO pointers\n");
> return -ENOMEM;
trivia:
The DRM_DEBUG could also be removed as the the alloc will do a
a dump_stack on failure.
Re: [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body
On Mon, 2022-02-28 at 14:24 +0300, Dan Carpenter wrote: > a multi-line indent gets curly braces for readability even though > it's not required by C. And then both sides would get curly braces. That's more your personal preference than a coding style guideline.
Re: [PATCH v7 7/7] MAINTAINERS: add maintainers for DRM LSDC driver
On Sun, 2022-02-13 at 22:16 +0800, Sui Jingfeng wrote: > From: suijingfeng [] > diff --git a/MAINTAINERS b/MAINTAINERS [] > @@ -6453,6 +6453,15 @@ T: git git://anongit.freedesktop.org/drm/drm-misc > F: drivers/gpu/drm/lima/ > F: include/uapi/drm/lima_drm.h > > +DRM DRIVERS FOR LOONGSON > +M: Sui Jingfeng > +L: dri-devel@lists.freedesktop.org > +R: Li Yi > +S: Maintained > +W: https://www.loongson.cn/ > +T: git git://anongit.freedesktop.org/drm/drm-misc > +F: drivers/gpu/drm/lsdc/ > + M then R then L please DRM DRIVERS FOR LOONGSON M: Sui Jingfeng R: Li Yi L: dri-devel@lists.freedesktop.org
Re: [PATCH] HPE BMC GXP SUPPORT
On Fri, 2022-02-04 at 12:31 +, Russell King (Oracle) wrote:
> On Fri, Feb 04, 2022 at 04:18:24AM -0800, Joe Perches wrote:
> > On Fri, 2022-02-04 at 12:05 +, Russell King (Oracle) wrote:
> > > On Wed, Feb 02, 2022 at 10:52:50AM -0600, nick.hawk...@hpe.com wrote:
> > > > + if (readb_relaxed(timer->control) & MASK_TCS_TC) {
> > > > + writeb_relaxed(MASK_TCS_TC, timer->control);
> > > > +
> > > > + event_handler = READ_ONCE(timer->evt.event_handler);
> > > > + if (event_handler)
> > > > + event_handler(>evt);
> > > > + return IRQ_HANDLED;
> > > > + } else {
> > > > + return IRQ_NONE;
> > > > + }
> > > > +}
> >
> > It's also less indented code and perhaps clearer to reverse the test
> >
> > if (!readb_relaxed(timer->control) & MASK_TCS_TC)
>
> This will need to be:
>
> if (!(readb_relaxed(timer->control) & MASK_TCS_TC))
right, thanks.
Re: [PATCH] HPE BMC GXP SUPPORT
On Fri, 2022-02-04 at 12:05 +, Russell King (Oracle) wrote:
> On Wed, Feb 02, 2022 at 10:52:50AM -0600, nick.hawk...@hpe.com wrote:
[]
> > diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
[]
> > +static irqreturn_t gxp_time_interrupt(int irq, void *dev_id)
> > +{
> > + struct gxp_timer *timer = dev_id;
> > + void (*event_handler)(struct clock_event_device *timer);
> > +
> > +
>
> One too many blank lines.
>
> > + if (readb_relaxed(timer->control) & MASK_TCS_TC) {
> > + writeb_relaxed(MASK_TCS_TC, timer->control);
> > +
> > + event_handler = READ_ONCE(timer->evt.event_handler);
> > + if (event_handler)
> > + event_handler(>evt);
> > + return IRQ_HANDLED;
> > + } else {
> > + return IRQ_NONE;
> > + }
> > +}
It's also less indented code and perhaps clearer to reverse the test
if (!readb_relaxed(timer->control) & MASK_TCS_TC)
return IRQ_NONE;
writeb_relaxed(MASK_TCS_TC, timer->control);
event_handler = READ_ONCE(timer->evt.event_handler);
if (event_handler)
event_handler(>evt);
return IRQ_HANDLED;
Re: [PATCH v1 4/4] fbtft: Replace 'depends on FB_TFT' by 'if FB_TFT ... endif'
On Tue, 2022-01-25 at 22:21 +0200, Andy Shevchenko wrote: > Replace 'depends on FB_TFT' by 'if FB_TFT ... endif' > for the sake of deduplication. [] > diff --git a/drivers/video/fbtft/Kconfig b/drivers/video/fbtft/Kconfig [] > @@ -10,87 +10,75 @@ menuconfig FB_TFT > select FB_DEFERRED_IO > select FB_BACKLIGHT > > +if FB_TFT > + [] > config FB_TFT_PCD8544 > tristate "FB driver for the PCD8544 LCD Controller" > - depends on FB_TFT > help > Generic Framebuffer support for PCD8544 > > @@ -108,62 +96,52 @@ config FB_TFT_S6D02A1 Looks like you missed a couple. --- drivers/video/fbtft/Kconfig | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/video/fbtft/Kconfig b/drivers/video/fbtft/Kconfig index 14ea3c6a60da0..4a02871f2cc71 100644 --- a/drivers/video/fbtft/Kconfig +++ b/drivers/video/fbtft/Kconfig @@ -84,13 +84,11 @@ config FB_TFT_PCD8544 config FB_TFT_RA8875 tristate "FB driver for the RA8875 LCD Controller" - depends on FB_TFT help Generic Framebuffer support for RA8875 config FB_TFT_S6D02A1 tristate "FB driver for the S6D02A1 LCD Controller" - depends on FB_TFT help Generic Framebuffer support for S6D02A1
Re: [PATCH 1/3] lib/string_helpers: Consolidate yesno() implementation
On Wed, 2022-01-19 at 16:00 -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 ;-) checkpatch doesn't say anything normally, it's a stupid script. It just mindlessly bleats a message when a line exceeds 100 chars... Just fyi: I think it's nicer on a single line too.
Re: [PATCH v2] drm/tilcdc: add const to of_device_id
On Thu, 2021-12-16 at 17:26 +0800, Xiang wangx wrote:
> struct of_device_id should normally be const.
[]
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
[]
> @@ -60,7 +60,7 @@ void tilcdc_module_cleanup(struct tilcdc_module *mod)
> list_del(>list);
> }
>
> -static struct of_device_id tilcdc_of_match[];
> +static const struct of_device_id tilcdc_of_match[];
This line could likely be removed instead.
$ git grep -w -n tilcdc_of_match
drivers/gpu/drm/tilcdc/tilcdc_drv.c:63:static struct of_device_id
tilcdc_of_match[];
drivers/gpu/drm/tilcdc/tilcdc_drv.c:590:static struct of_device_id
tilcdc_of_match[] = {
drivers/gpu/drm/tilcdc/tilcdc_drv.c:595:MODULE_DEVICE_TABLE(of,
tilcdc_of_match);
drivers/gpu/drm/tilcdc/tilcdc_drv.c:603:.of_match_table =
tilcdc_of_match,
> @@ -587,7 +587,7 @@ static int tilcdc_pdev_remove(struct platform_device
> *pdev)
> return 0;
> }
>
> -static struct of_device_id tilcdc_of_match[] = {
> +static const struct of_device_id tilcdc_of_match[] = {
> { .compatible = "ti,am33xx-tilcdc", },
> { .compatible = "ti,da850-tilcdc", },
> { },
Re: [PATCH v4 3/3] MAINTAINERS: Mark VMware mailing list entries as email aliases
On Tue, 2021-11-16 at 14:41 -0800, Srivatsa S. Bhat wrote: > From: Srivatsa S. Bhat (VMware) > > VMware mailing lists in the MAINTAINERS file are private lists meant > for VMware-internal review/notification for patches to the respective > subsystems. Anyone can post to these addresses, but there is no public > read access like open mailing lists, which makes them more like email > aliases instead (to reach out to reviewers). > > So update all the VMware mailing list references in the MAINTAINERS > file to mark them as such, using "R: email-al...@vmware.com". > > Signed-off-by: Srivatsa S. Bhat (VMware) Acked-by: Joe Perches > diff --git a/MAINTAINERS b/MAINTAINERS [] > @@ -6223,8 +6223,8 @@ T: git git://anongit.freedesktop.org/drm/drm-misc > F: drivers/gpu/drm/vboxvideo/ > > DRM DRIVER FOR VMWARE VIRTUAL GPU > -M: "VMware Graphics" > M: Zack Rusin > +R: VMware Graphics Reviewers etc...
Re: [PATCH] drm/amd/amdgpu: remove useless break after return
On Sun, 2021-11-14 at 23:14 -0800, Bernard Zhao wrote:
> This change is to remove useless break after return.
[]
> diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
> b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
[]
> @@ -2092,22 +2092,18 @@ static int dce_v8_0_pick_dig_encoder(struct
> drm_encoder *encoder)
> return 1;
> else
> return 0;
> - break;
> case ENCODER_OBJECT_ID_INTERNAL_UNIPHY1:
> if (dig->linkb)
> return 3;
> else
> return 2;
> - break;
> case ENCODER_OBJECT_ID_INTERNAL_UNIPHY2:
> if (dig->linkb)
> return 5;
> else
> return 4;
> - break;
> case ENCODER_OBJECT_ID_INTERNAL_UNIPHY3:
> return 6;
> - break;
> default:
> DRM_ERROR("invalid encoder_id: 0x%x\n",
> amdgpu_encoder->encoder_id);
> return 0;
Perhaps rewrite to make the sequential numbering more obvious.
---
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 19 +++
1 file changed, 3 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index b200b9e722d97..7307524b706b4 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -2088,26 +2088,13 @@ static int dce_v8_0_pick_dig_encoder(struct drm_encoder
*encoder)
switch (amdgpu_encoder->encoder_id) {
case ENCODER_OBJECT_ID_INTERNAL_UNIPHY:
- if (dig->linkb)
- return 1;
- else
- return 0;
- break;
+ return !dig->linkb ? 0 : 1;
case ENCODER_OBJECT_ID_INTERNAL_UNIPHY1:
- if (dig->linkb)
- return 3;
- else
- return 2;
- break;
+ return !dig->linkb ? 2 : 3;
case ENCODER_OBJECT_ID_INTERNAL_UNIPHY2:
- if (dig->linkb)
- return 5;
- else
- return 4;
- break;
+ return !dig->linkb ? 4 : 5;
case ENCODER_OBJECT_ID_INTERNAL_UNIPHY3:
return 6;
- break;
default:
DRM_ERROR("invalid encoder_id: 0x%x\n",
amdgpu_encoder->encoder_id);
return 0;
Re: [PATCH v3 3/3] MAINTAINERS: Mark VMware mailing list entries as email aliases
On Wed, 2021-11-10 at 17:39 -0800, Jakub Kicinski wrote: > On Wed, 10 Nov 2021 12:09:06 -0800 Srivatsa S. Bhat wrote: > > DRM DRIVER FOR VMWARE VIRTUAL GPU > > -M: "VMware Graphics" > > M: Zack Rusin > > +R: VMware Graphics Reviewers > > L: dri-devel@lists.freedesktop.org > > S: Supported > > T: git git://anongit.freedesktop.org/drm/drm-misc > > It'd be preferable for these corporate entries to be marked or > otherwise distinguishable so that we can ignore them when we try > to purge MAINTAINERS from developers who stopped participating. > > These addresses will never show up in a commit tag which is normally > sign of inactivity. Funny. The link below is from over 5 years ago. https://lore.kernel.org/lkml/1472081625.3746.217.ca...@perches.com/ Almost all of those entries are still in MAINTAINERS. I think the concept of purging is a non-issue.
Re: [PATCH 2/2] MAINTAINERS: Mark VMware mailing list entries as private
On Tue, 2021-11-09 at 00:58 +, Nadav Amit wrote: > > On Nov 8, 2021, at 4:37 PM, Joe Perches wrote: > > On Mon, 2021-11-08 at 16:22 -0800, Srivatsa S. Bhat wrote: > > > > So it's an exploder not an actual maintainer and it likely isn't > > publically archived with any normal list mechanism. > > > > So IMO "private" isn't appropriate. Neither is "L:" > > Perhaps just mark it as what it is as an "exploder". > > > > Or maybe these blocks should be similar to: > > > > M: Name of Lead Developer > > M: VMware maintainers -maintain...@vmlinux.com> Maybe adding entries like M: Named maintainer R: VMware reviewers -maintain...@vmware.com> would be best/simplest.
Re: [PATCH 2/2] MAINTAINERS: Mark VMware mailing list entries as private
On Mon, 2021-11-08 at 16:22 -0800, Srivatsa S. Bhat wrote: > +Greg, Thomas > > Hi Joe, > > On 11/8/21 3:37 PM, Joe Perches wrote: > > On Mon, 2021-11-08 at 12:30 -0800, Srivatsa S. Bhat wrote: > > > From: Srivatsa S. Bhat (VMware) > > > > > > VMware mailing lists in the MAINTAINERS file are private lists meant > > > for VMware-internal review/notification for patches to the respective > > > subsystems. So, in an earlier discussion [1][2], it was recommended to > > > mark them as such. Update all the remaining VMware mailing list > > > references to use that format -- "L: list@address (private)". > > [] > > > diff --git a/MAINTAINERS b/MAINTAINERS > > [] > > > @@ -6134,8 +6134,8 @@ T: git git://anongit.freedesktop.org/drm/drm-misc > > > F: drivers/gpu/drm/vboxvideo/ > > > > > > DRM DRIVER FOR VMWARE VIRTUAL GPU > > > -M: "VMware Graphics" > > > M: Zack Rusin > > > +L: linux-graphics-maintai...@vmware.com (private) > > > > This MAINTAINERS file is for _public_ use, marking something > > non-public isn't useful. > > > > private makes no sense and likely these L: entries shouldn't exist. > > Well, the public can send messages to this list, but membership is > restricted. Ah, new information. That's not quite what the commit message describes. > In many ways, I believe this is similar to x...@kernel.org, which is an > email alias that anyone can post to in order to reach the x86 > maintainer community for patch review. I see x...@kernel.org listed as > both L: and M: in the MAINTAINERS file, among different entries. > > Although the @vmware list ids refer to VMware-internal mailing lists > as opposed to email aliases, they serve a very similar purpose -- to > inform VMware folks about patches to the relevant subsystems. > > Is there a consensus on how such lists should be specified? Not so far as I know. > One > suggestion (from Greg in the email thread referenced above) was to > mark it as private, which is what this patch does. Maybe we can find a > better alternative? > > How about specifying such lists using M: (indicating that this address > can be used to reach maintainers), as long as that is not the only M: > entry for a given subsystem (i.e., it includes real people's email id > as well)? I think that would address Greg's primary objection too from > that other thread (related to personal responsibility as maintainers). So it's an exploder not an actual maintainer and it likely isn't publically archived with any normal list mechanism. So IMO "private" isn't appropriate. Neither is "L:" Perhaps just mark it as what it is as an "exploder". Or maybe these blocks should be similar to: M: Name of Lead Developer M: VMware maintainers -maintain...@vmlinux.com> Maybe something like a comment mechanism should be added to the MAINTAINERS file. Maybe # so this entry could be something like: M: VMware maintainers -maintain...@vmlinux.com> # VMware's ever changing internal maintainers list
Re: [PATCH 2/2] MAINTAINERS: Mark VMware mailing list entries as private
On Mon, 2021-11-08 at 16:16 -0800, Jakub Kicinski wrote: > On Mon, 08 Nov 2021 15:37:53 -0800 Joe Perches wrote: > > > @@ -6134,8 +6134,8 @@ T: git git://anongit.freedesktop.org/drm/drm-misc > > > F: drivers/gpu/drm/vboxvideo/ > > > > > > DRM DRIVER FOR VMWARE VIRTUAL GPU > > > -M: "VMware Graphics" > > > M: Zack Rusin > > > +L: linux-graphics-maintai...@vmware.com (private) > > > > This MAINTAINERS file is for _public_ use, marking something > > non-public isn't useful. > > But Greg has a point. Corporations like to send us code with a list > as the maintainer and MODULE_AUTHOR set to corp's name. We deal with > humans, not legal entities. MAINTAINERS is used not for corporations private use but to find out _who_ to send and cc patches and defect reports. A "private" email address used only for corporate internal review cannot receive patches. > I've been trying to get them to use "M: email" without the name, > but "L: list (private)" also works. > > Either way I feel like we need _some_ way to tell humans from corporate > "please CC this address" entries. This is not the way AFAIKT. > > private makes no sense and likely these L: entries shouldn't exist.
Re: [PATCH 2/2] MAINTAINERS: Mark VMware mailing list entries as private
On Mon, 2021-11-08 at 12:30 -0800, Srivatsa S. Bhat wrote: > From: Srivatsa S. Bhat (VMware) > > VMware mailing lists in the MAINTAINERS file are private lists meant > for VMware-internal review/notification for patches to the respective > subsystems. So, in an earlier discussion [1][2], it was recommended to > mark them as such. Update all the remaining VMware mailing list > references to use that format -- "L: list@address (private)". [] > diff --git a/MAINTAINERS b/MAINTAINERS [] > @@ -6134,8 +6134,8 @@ T: git git://anongit.freedesktop.org/drm/drm-misc > F: drivers/gpu/drm/vboxvideo/ > > DRM DRIVER FOR VMWARE VIRTUAL GPU > -M: "VMware Graphics" > M: Zack Rusin > +L: linux-graphics-maintai...@vmware.com (private) This MAINTAINERS file is for _public_ use, marking something non-public isn't useful. private makes no sense and likely these L: entries shouldn't exist.
Re: dt-bindings: treewide: Update @st.com email address to @foss.st.com
On Wed, 2021-10-27 at 15:56 +0200, Patrice CHOTARD wrote: > On 10/27/21 8:11 AM, Patrice CHOTARD wrote: > > On 10/20/21 1:39 PM, Marc Zyngier wrote: > > > On Wed, 20 Oct 2021 08:45:02 +0100, > > > Krzysztof Kozlowski wrote: > > > > On 20/10/2021 08:50, patrice.chot...@foss.st.com wrote: > > > > > From: Patrice Chotard > > > > > > > > > > Not all @st.com email address are concerned, only people who have > > > > > a specific @foss.st.com email will see their entry updated. > > > > > For some people, who left the company, remove their email. > > > > Also would be nice to see here explained *why* are you doing this. > > > > > > And why this can't be done with a single update to .mailmap, like > > > anyone else does. > > > > Thanks for the tips, yes, it will be simpler. > > I made a try by updating .mailmap with adding a new entry with my > @foss.st.com email : > > Pali Rohár > Paolo 'Blaisorblade' Giarrusso > +Patrice Chotard > Patrick Mochel > Paul Burton > > But when running ./scripts/get_maintainer.pl > Documentation/devicetree/bindings/arm/sti.yaml, by old email is still > displayed > > Rob Herring (maintainer:OPEN FIRMWARE AND FLATTENED > DEVICE TREE BINDINGS) > Patrice Chotard (in file) > devicet...@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE > BINDINGS) > linux-ker...@vger.kernel.org (open list) > > By default, the get_maintainer.pl script is using .mailmap file > ($email_use_mailmap = 1). > > It seems there is an issue with get_maintainer.pl and maintainer name/e-mail > found in yaml file ? I'm of two minds whether it's an "issue" actually. get_maintainer is not the only tool used to create email address lists. Some actually read files like MAINTAINERS or .dts or .yaml files directly to find maintainer addresses. So If your name and email address is listed in an source file where nominally active email addresses are entered then I believe .mailmap should not modify it. So I believe email addresses in each file should be updated in preference to using a mailmap entry for nominally active email addresses in these files. --- $ cat Documentation/devicetree/bindings/arm/sti.yaml # SPDX-License-Identifier: GPL-2.0 %YAML 1.2 --- $id: http://devicetree.org/schemas/arm/sti.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# title: ST STi Platforms Device Tree Bindings maintainers: - Patrice Chotard
Re: [PATCH 2/4] amdgpu_ucode: reduce number of pr_debug calls
On Wed, 2021-09-29 at 19:44 -0600, Jim Cromie wrote:
> There are blocks of DRM_DEBUG calls, consolidate their args into
> single calls. With dynamic-debug in use, each callsite consumes 56
> bytes of callsite data, and this patch removes about 65 calls, so
> it saves ~3.5kb.
>
> no functional changes.
No functional change, but an output logging content change.
> RFC: this creates multi-line log messages, does that break any syslog
> conventions ?
It does change the output as each individual DRM_DEBUG is a call to
__drm_dbg which is effectively:
printk(KERN_DEBUG "[" DRM_NAME ":%ps] %pV",
__builtin_return_address(0), );
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ucode.c
[]
> @@ -30,17 +30,26 @@
>
>
> static void amdgpu_ucode_print_common_hdr(const struct
> common_firmware_header *hdr)
> {
> - DRM_DEBUG("size_bytes: %u\n", le32_to_cpu(hdr->size_bytes));
> - DRM_DEBUG("header_size_bytes: %u\n",
> le32_to_cpu(hdr->header_size_bytes));
[]
> + DRM_DEBUG("size_bytes: %u\n"
> + "header_size_bytes: %u\n"
etc...
Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller
On Thu, 2021-08-19 at 15:51 +0100, Colin Ian King wrote: > it still makes sense for these kind of > janitorial changes as it makes sense to constify arrays when they are > read-only and making them static is sensible for const data. I'm not disagreeing. Marking unmodifiable arrays as const is generally useful for readers. Decent compilers though can _mostly_ determine whether or not an array is used as const and whether the array can be placed in a readonly section and is not required to be in a writable one. But the object sizes deltas you show with an allmodconfig are misleading. At a minimum I think you should show the output sizes as allmodconfig.
Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller
On Thu, 2021-08-19 at 14:54 +0100, Colin Ian King wrote: > On 19/08/2021 14:51, Joe Perches wrote: > > On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote: > > > From: Colin Ian King > > > > > > Don't populate the array ext_div on the stack but instead it > > > static const. Makes the object code smaller by 118 bytes: > > > > > > Before: > > > textdatabss dechex filename > > > 39449 17500128 57077 def5 ./drivers/gpu/drm/bridge/tc358767.o > > > > > > After: > > > textdatabss dechex filename > > > 39235 17596128 56959 de7f ./drivers/gpu/drm/bridge/tc358767.o > > > > Why is text smaller and data larger with this change? > > There are less instructions being used with the change since it's not > shoving the array data onto the stack at run time. Instead the array is > being stored in the data section and there is less object code required > to access the data. Ah. It's really because it's not a minimal compilation ala defconfig. I think you should really stop making these size comparisons with .config uses that are not based on a defconfig as a whole lot of other things are going on. Please notice that the object sizes are significantly smaller below: So with an x86-64 defconfig and this compilation unit enabled with CONFIG_OF enabled and CONFIG_DRM_TOSHIBA_TC358767=y, with gcc 10.3 and this change the object size actually increases a bit. $ size drivers/gpu/drm/bridge/tc358767.o* textdata bss dec hex filename 13554 268 1 1382335ff drivers/gpu/drm/bridge/tc358767.o.new 13548 268 1 1381735f9 drivers/gpu/drm/bridge/tc358767.o.old objdump -h shows these differences: .old: 0 .text 1e1f 0040 2**4 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE [...] 14 .rodata 05ae 46e0 2**5 CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA .new: 0 .text 1e05 0040 2**4 CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE [...] 11 .rodata 05ce 4600 2**5 CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA cheers, Joe
Re: [PATCH] drm/bridge/tc358767: make the array ext_div static const, makes object smaller
On Thu, 2021-08-19 at 14:38 +0100, Colin King wrote:
> From: Colin Ian King
>
> Don't populate the array ext_div on the stack but instead it
> static const. Makes the object code smaller by 118 bytes:
>
> Before:
> textdatabss dechex filename
> 39449 17500128 57077 def5 ./drivers/gpu/drm/bridge/tc358767.o
>
> After:
> textdatabss dechex filename
> 39235 17596128 56959 de7f ./drivers/gpu/drm/bridge/tc358767.o
Why is text smaller and data larger with this change?
>
> (gcc version 10.3.0)
>
> Signed-off-by: Colin Ian King
> ---
> drivers/gpu/drm/bridge/tc358767.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c
> b/drivers/gpu/drm/bridge/tc358767.c
> index 23a6f90b694b..599c23759400 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -468,7 +468,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk,
> u32 pixelclock)
> int div, best_div = 1;
> int mul, best_mul = 1;
> int delta, best_delta;
> - int ext_div[] = {1, 2, 3, 5, 7};
> + static const int ext_div[] = {1, 2, 3, 5, 7};
> int best_pixelclock = 0;
> int vco_hi = 0;
> u32 pxl_pllparam;
Re: [PATCH 1/8] drm/ingenic: Remove dead code
On Sun, 2021-08-08 at 19:58 +0200, Thomas Zimmermann wrote: > > Am 08.08.21 um 15:45 schrieb Paul Cercueil: > > The priv->ipu_plane would get a different value further down the code, > > without the first assigned value being read first; so the first > > assignation can be dropped. > > > > Signed-off-by: Paul Cercueil > > Acked-by: Thomas Zimmermann I think this is at best an incomplete description. How is it known that this priv->ipu_plane assignment isn't necessary for any path of any failure path after this assignment and before the new assignment? > > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c [] > > @@ -984,9 +984,6 @@ static int ingenic_drm_bind(struct device *dev, bool > > has_components) > > priv->dma_hwdescs->hwdesc_pal.cmd = JZ_LCD_CMD_ENABLE_PAL > > | (sizeof(priv->dma_hwdescs->palette) / 4); > > > > - if (soc_info->has_osd) > > - priv->ipu_plane = drm_plane_from_index(drm, 0); > > - > > primary = priv->soc_info->has_osd ? >f1 : >f0; > > > > drm_plane_helper_add(primary, _drm_plane_helper_funcs); > > >
Re: drm/amd/display: Simplify hdcp validate_bksv
On Mon, 2021-07-12 at 07:02 +0800, kernel test robot wrote: > Hi Joe, > > I love your patch! Yet something to improve: > > [auto build test ERROR on drm-intel/for-linux-next] > [also build test ERROR on drm-exynos/exynos-drm-next linus/master > next-20210709] > [cannot apply to kees/for-next/pstore tegra-drm/drm/tegra/for-next > drm/drm-next v5.13] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: > https://github.com/0day-ci/linux/commits/Joe-Perches/drm-amd-display-Simplify-hdcp-validate_bksv/20210712-034708 > base: git://anongit.freedesktop.org/drm-intel for-linux-next > config: i386-randconfig-a003-20210712 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-22) 9.3.0 > reproduce (this is a W=1 build): > # > https://github.com/0day-ci/linux/commit/66fae2c1becdcb71c95f2c6a6413de4dfe1deb51 > git remote add linux-review https://github.com/0day-ci/linux > git fetch --no-tags linux-review > Joe-Perches/drm-amd-display-Simplify-hdcp-validate_bksv/20210712-034708 > git checkout 66fae2c1becdcb71c95f2c6a6413de4dfe1deb51 > # save the attached .config to linux build tree > make W=1 ARCH=i386 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > > All errors (new ones prefixed by >>, old ones prefixed by <<): > > > > ERROR: modpost: "__popcountdi2" [drivers/gpu/drm/amd/amdgpu/amdgpu.ko] > > > undefined! curious. Anyone know why?
drm/amd/display: Simplify hdcp validate_bksv
commit 06888d571b51 ("drm/amd/display: Avoid HDCP over-read and corruption")
fixed an overread with an invalid buffer length but added an unnecessary
buffer and copy.
Simplify the code by using a single uint64_t and __builtin_popcountll to
count the number of bits set in the original bksv buffer instead of a loop.
This also avoid a possible unaligned access of the temporary bksv.
Signed-off-by: Joe Perches
---
It seems quite odd 20 bits set is a magic number here.
Should it be a specific be/le value instead?
drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c | 11 ++-
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
index de872e7958b06..78a4c6dd95d99 100644
--- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
+++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
@@ -28,17 +28,10 @@
static inline enum mod_hdcp_status validate_bksv(struct mod_hdcp *hdcp)
{
uint64_t n = 0;
- uint8_t count = 0;
- u8 bksv[sizeof(n)] = { };
- memcpy(bksv, hdcp->auth.msg.hdcp1.bksv,
sizeof(hdcp->auth.msg.hdcp1.bksv));
- n = *(uint64_t *)bksv;
+ memcpy(, hdcp->auth.msg.hdcp1.bksv,
sizeof(hdcp->auth.msg.hdcp1.bksv));
- while (n) {
- count++;
- n &= (n - 1);
- }
- return (count == 20) ? MOD_HDCP_STATUS_SUCCESS :
+ return (__builtin_popcountll(n) == 20) ? MOD_HDCP_STATUS_SUCCESS :
MOD_HDCP_STATUS_HDCP1_INVALID_BKSV;
}
Re: [PATCH] drm/amd/display: Fix identical code for different branches
On Sun, 2021-07-11 at 19:24 +0200, Len Baker wrote:
> The branches of the "if" statement are the same. So remove the
> unnecessary if and goto statements.
>
> Addresses-Coverity-ID: 1456916 ("Identical code for different branches")
> Fixes: 4c283fdac08ab ("drm/amd/display: Add HDCP module")
> Signed-off-by: Len Baker
I'm not a big fan of this type of change.
It's currently the same style used for six tests in this function
and changing this last one would just make it harder to see the
code blocks as consistent.
I doubt any reasonable compiler would produce different objects.
> diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
> b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
[]
> @@ -305,10 +305,8 @@ static enum mod_hdcp_status wait_for_ready(struct
> mod_hdcp *hdcp,
> hdcp, "bcaps_read"))
> goto out;
> }
> - if (!mod_hdcp_execute_and_set(check_ksv_ready,
> - >ready_check, ,
> - hdcp, "ready_check"))
> - goto out;
> + mod_hdcp_execute_and_set(check_ksv_ready, >ready_check, ,
> + hdcp, "ready_check");
> out:
> return status;
> }
> --
> 2.25.1
>
Re: [RFC PATCH v2 1/4] drm_print.h: rewrap __DRM_DEFINE_DBG_RATELIMITED macro
On Sat, 2021-07-10 at 23:49 -0600, Jim Cromie wrote:
> whitespace only, to diff-minimize a later commit.
> no functional changes
[]
> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
[]
> @@ -524,19 +524,24 @@ void __drm_err(const char *format, ...);
> #define DRM_DEBUG_DP(fmt, ...)
> \
> __drm_dbg(DRM_UT_DP, fmt, ## __VA_ARGS__)
>
>
> -#define __DRM_DEFINE_DBG_RATELIMITED(category, drm, fmt, ...)
> \
> -({
> \
> - static DEFINE_RATELIMIT_STATE(rs_, DEFAULT_RATELIMIT_INTERVAL,
> DEFAULT_RATELIMIT_BURST);\
> - const struct drm_device *drm_ = (drm);
> \
> -
> \
> - if (drm_debug_enabled(DRM_UT_ ## category) && __ratelimit(_))
> \
> - drm_dev_printk(drm_ ? drm_->dev : NULL, KERN_DEBUG, fmt, ##
> __VA_ARGS__); \
> +#define __DRM_DEFINE_DBG_RATELIMITED(category, drm, fmt, ...)
> \
> +({ \
> + static DEFINE_RATELIMIT_STATE(rs_, \
> + DEFAULT_RATELIMIT_INTERVAL, \
> + DEFAULT_RATELIMIT_BURST); \
> + const struct drm_device *drm_ = (drm); \
> + \
> + if (drm_debug_enabled(DRM_UT_ ## category) \
> + && __ratelimit(_)) \
Though I don't really see the need for the change, the typical style
has the logical continuation at the end of the test.
if (drm_debug_enabled(DRM_UT_ ## category) && \
__ratelimit(_)) \
Re: [PATCH V2] treewide: Add missing semicolons to __assign_str uses
On Sat, 2021-06-12 at 08:42 -0700, Joe Perches wrote: > The __assign_str macro has an unusual ending semicolon but the vast > majority of uses of the macro already have semicolon termination. ping?
[PATCH V2] treewide: Add missing semicolons to __assign_str uses
The __assign_str macro has an unusual ending semicolon but the vast
majority of uses of the macro already have semicolon termination.
$ git grep -P '\b__assign_str\b' | wc -l
551
$ git grep -P '\b__assign_str\b.*;' | wc -l
480
Add semicolons to the __assign_str() uses without semicolon termination
and all the other uses without semicolon termination via additional defines
that are equivalent to __assign_str() with the eventual goal of removing
the semicolon from the __assign_str() macro definition.
Link:
https://lore.kernel.org/lkml/1e068d21106bb6db05b735b4916bb420e6c9842a.ca...@perches.com/
Signed-off-by: Joe Perches
---
V2: Remove semicolon addition to #define VIF_ASSIGN as every use of
this macro already has a semicolon termination.
Compiled x84-64 allyesconfig
drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 14
drivers/gpu/drm/lima/lima_trace.h | 2 +-
drivers/infiniband/hw/hfi1/trace_misc.h| 4 +--
drivers/infiniband/hw/hfi1/trace_rc.h | 4 +--
drivers/infiniband/hw/hfi1/trace_tid.h | 6 ++--
drivers/infiniband/hw/hfi1/trace_tx.h | 8 ++---
drivers/infiniband/sw/rdmavt/trace_cq.h| 4 +--
drivers/infiniband/sw/rdmavt/trace_mr.h| 2 +-
drivers/infiniband/sw/rdmavt/trace_qp.h| 4 +--
drivers/infiniband/sw/rdmavt/trace_rc.h| 2 +-
drivers/infiniband/sw/rdmavt/trace_tx.h| 4 +--
drivers/misc/mei/mei-trace.h | 6 ++--
.../net/ethernet/marvell/octeontx2/af/rvu_trace.h | 12 +++
drivers/net/fjes/fjes_trace.h | 4 +--
drivers/usb/cdns3/cdnsp-trace.h| 2 +-
fs/nfs/nfs4trace.h | 6 ++--
fs/nfs/nfstrace.h | 4 +--
include/trace/events/btrfs.h | 2 +-
include/trace/events/dma_fence.h | 4 +--
include/trace/events/rpcgss.h | 4 +--
include/trace/events/sunrpc.h | 40 +++---
21 files changed, 69 insertions(+), 69 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
index 0527772fe1b80..d855cb53c7e09 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
@@ -176,10 +176,10 @@ TRACE_EVENT(amdgpu_cs_ioctl,
TP_fast_assign(
__entry->sched_job_id = job->base.id;
- __assign_str(timeline,
AMDGPU_JOB_GET_TIMELINE_NAME(job))
+ __assign_str(timeline,
AMDGPU_JOB_GET_TIMELINE_NAME(job));
__entry->context =
job->base.s_fence->finished.context;
__entry->seqno = job->base.s_fence->finished.seqno;
- __assign_str(ring,
to_amdgpu_ring(job->base.sched)->name)
+ __assign_str(ring,
to_amdgpu_ring(job->base.sched)->name);
__entry->num_ibs = job->num_ibs;
),
TP_printk("sched_job=%llu, timeline=%s, context=%u, seqno=%u,
ring_name=%s, num_ibs=%u",
@@ -201,10 +201,10 @@ TRACE_EVENT(amdgpu_sched_run_job,
TP_fast_assign(
__entry->sched_job_id = job->base.id;
- __assign_str(timeline,
AMDGPU_JOB_GET_TIMELINE_NAME(job))
+ __assign_str(timeline,
AMDGPU_JOB_GET_TIMELINE_NAME(job));
__entry->context =
job->base.s_fence->finished.context;
__entry->seqno = job->base.s_fence->finished.seqno;
- __assign_str(ring,
to_amdgpu_ring(job->base.sched)->name)
+ __assign_str(ring,
to_amdgpu_ring(job->base.sched)->name);
__entry->num_ibs = job->num_ibs;
),
TP_printk("sched_job=%llu, timeline=%s, context=%u, seqno=%u,
ring_name=%s, num_ibs=%u",
@@ -229,7 +229,7 @@ TRACE_EVENT(amdgpu_vm_grab_id,
TP_fast_assign(
__entry->pasid = vm->pasid;
- __assign_str(ring, ring->name)
+ __assign_str(ring, ring->name);
__entry->vmid = job->vmid;
__entry->vm_hub = ring->funcs->vmhub,
__entry->pd_addr = job->vm_pd_addr;
@@ -424,7 +424,7 @@ TRACE_EVENT(amdgpu_vm_flush,
),
TP_fast_assign(
- __assign_str(ring, ring->name)
+ __assign_str(ring, ring->name);
__entry->vmid = vmid;
Re: [PATCH][next] drm/i915/gem: Fix fall-through warning for Clang
On Mon, 2021-06-07 at 15:32 -0500, Gustavo A. R. Silva wrote:
> In preparation to enable -Wimplicit-fallthrough for Clang, fix a
> warning by explicitly adding a fallthrough; statement.
[]
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
> b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
[]
> @@ -62,6 +62,7 @@ static void try_to_writeback(struct drm_i915_gem_object
> *obj,
> switch (obj->mm.madv) {
> case I915_MADV_DONTNEED:
> i915_gem_object_truncate(obj);
> + fallthrough;
> case __I915_MADV_PURGED:
> return;
> }
I think fallthrough to return is not particularly nice to follow.
This is the current function:
static void try_to_writeback(struct drm_i915_gem_object *obj,
unsigned int flags)
{
switch (obj->mm.madv) {
case I915_MADV_DONTNEED:
i915_gem_object_truncate(obj);
case __I915_MADV_PURGED:
return;
}
if (flags & I915_SHRINK_WRITEBACK)
i915_gem_object_writeback(obj);
}
One of these might be more typical:
static void try_to_writeback(struct drm_i915_gem_object *obj,
unsigned int flags)
{
switch (obj->mm.madv) {
case I915_MADV_DONTNEED:
i915_gem_object_truncate(obj);
break;
case __I915_MADV_PURGED:
break;
default:
if (flags & I915_SHRINK_WRITEBACK)
i915_gem_object_writeback(obj);
break;
}
}
or maybe:
static void try_to_writeback(struct drm_i915_gem_object *obj,
unsigned int flags)
{
switch (obj->mm.madv) {
case I915_MADV_DONTNEED:
i915_gem_object_truncate(obj);
return;
case __I915_MADV_PURGED:
return;
}
if (flags & I915_SHRINK_WRITEBACK)
i915_gem_object_writeback(obj);
}
[PATCH] treewide: Add missing semicolons to __assign_str uses
The __assign_str macro has an unusual ending semicolon but the vast
majority of uses of the macro already have semicolon termination.
$ git grep -P '\b__assign_str\b' | wc -l
551
$ git grep -P '\b__assign_str\b.*;' | wc -l
480
Add semicolons to the __assign_str() uses without semicolon termination
and all the other uses without semicolon termination via additional defines
that are equivalent to __assign_str() with the eventual goal of removing
the semicolon from the __assign_str() macro definition.
Link:
https://lore.kernel.org/lkml/1e068d21106bb6db05b735b4916bb420e6c9842a.ca...@perches.com/
Signed-off-by: Joe Perches
---
Resending without all the direct cc's, only the mailing lists as
it seems not to have gone through via vger.
Compiled x84-64 allyesconfig
On Fri, 2021-06-04 at 12:21 -0400, Steven Rostedt wrote:
> I have no problem taking a clean up patch that adds semicolons to all
> use cases of "__assign_str()" and ever remove the one from where it is
> defined. As long as it doesn't break any builds, I'm fine with that.
Removing the semicolon from the macro definition is left for another patch.
drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 14
drivers/gpu/drm/lima/lima_trace.h | 2 +-
drivers/infiniband/hw/hfi1/trace_misc.h| 4 +--
drivers/infiniband/hw/hfi1/trace_rc.h | 4 +--
drivers/infiniband/hw/hfi1/trace_tid.h | 6 ++--
drivers/infiniband/hw/hfi1/trace_tx.h | 8 ++---
drivers/infiniband/sw/rdmavt/trace_cq.h| 4 +--
drivers/infiniband/sw/rdmavt/trace_mr.h| 2 +-
drivers/infiniband/sw/rdmavt/trace_qp.h| 4 +--
drivers/infiniband/sw/rdmavt/trace_rc.h| 2 +-
drivers/infiniband/sw/rdmavt/trace_tx.h| 4 +--
drivers/misc/mei/mei-trace.h | 6 ++--
.../net/ethernet/marvell/octeontx2/af/rvu_trace.h | 12 +++
drivers/net/fjes/fjes_trace.h | 4 +--
drivers/usb/cdns3/cdnsp-trace.h| 2 +-
fs/nfs/nfs4trace.h | 6 ++--
fs/nfs/nfstrace.h | 4 +--
include/trace/events/btrfs.h | 2 +-
include/trace/events/dma_fence.h | 4 +--
include/trace/events/rpcgss.h | 4 +--
include/trace/events/sunrpc.h | 40 +++---
net/mac80211/trace.h | 2 +-
22 files changed, 70 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
index 0527772fe1b80..d855cb53c7e09 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
@@ -176,10 +176,10 @@ TRACE_EVENT(amdgpu_cs_ioctl,
TP_fast_assign(
__entry->sched_job_id = job->base.id;
- __assign_str(timeline,
AMDGPU_JOB_GET_TIMELINE_NAME(job))
+ __assign_str(timeline,
AMDGPU_JOB_GET_TIMELINE_NAME(job));
__entry->context =
job->base.s_fence->finished.context;
__entry->seqno = job->base.s_fence->finished.seqno;
- __assign_str(ring,
to_amdgpu_ring(job->base.sched)->name)
+ __assign_str(ring,
to_amdgpu_ring(job->base.sched)->name);
__entry->num_ibs = job->num_ibs;
),
TP_printk("sched_job=%llu, timeline=%s, context=%u, seqno=%u,
ring_name=%s, num_ibs=%u",
@@ -201,10 +201,10 @@ TRACE_EVENT(amdgpu_sched_run_job,
TP_fast_assign(
__entry->sched_job_id = job->base.id;
- __assign_str(timeline,
AMDGPU_JOB_GET_TIMELINE_NAME(job))
+ __assign_str(timeline,
AMDGPU_JOB_GET_TIMELINE_NAME(job));
__entry->context =
job->base.s_fence->finished.context;
__entry->seqno = job->base.s_fence->finished.seqno;
- __assign_str(ring,
to_amdgpu_ring(job->base.sched)->name)
+ __assign_str(ring,
to_amdgpu_ring(job->base.sched)->name);
__entry->num_ibs = job->num_ibs;
),
TP_printk("sched_job=%llu, timeline=%s, context=%u, seqno=%u,
ring_name=%s, num_ibs=%u",
@@ -229,7 +229,7 @@ TRACE_EVENT(amdgpu_vm_grab_id,
TP_fast_assign(
__entry->pasid = vm->pasid;
- __assign_str(ring, ring->name)
+ __assign_str(ring, ring->name);
__entry->vmid = job->vmid;
__entry
[PATCH] treewide: Add missing semicolons to __assign_str uses
The __assign_str macro has an unusual ending semicolon but the vast
majority of uses of the macro already have semicolon termination.
$ git grep -P '\b__assign_str\b' | wc -l
551
$ git grep -P '\b__assign_str\b.*;' | wc -l
480
Add semicolons to the __assign_str() uses without semicolon termination
and all the other uses without semicolon termination via additional defines
that are equivalent to __assign_str() with the eventual goal of removing
the semicolon from the __assign_str() macro definition.
Link:
https://lore.kernel.org/lkml/1e068d21106bb6db05b735b4916bb420e6c9842a.ca...@perches.com/
Signed-off-by: Joe Perches
---
Compiled x84-64 allyesconfig
On Fri, 2021-06-04 at 12:21 -0400, Steven Rostedt wrote:
> I have no problem taking a clean up patch that adds semicolons to all
> use cases of "__assign_str()" and ever remove the one from where it is
> defined. As long as it doesn't break any builds, I'm fine with that.
Removing the semicolon from the macro definition is left for another patch.
drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 14
drivers/gpu/drm/lima/lima_trace.h | 2 +-
drivers/infiniband/hw/hfi1/trace_misc.h| 4 +--
drivers/infiniband/hw/hfi1/trace_rc.h | 4 +--
drivers/infiniband/hw/hfi1/trace_tid.h | 6 ++--
drivers/infiniband/hw/hfi1/trace_tx.h | 8 ++---
drivers/infiniband/sw/rdmavt/trace_cq.h| 4 +--
drivers/infiniband/sw/rdmavt/trace_mr.h| 2 +-
drivers/infiniband/sw/rdmavt/trace_qp.h| 4 +--
drivers/infiniband/sw/rdmavt/trace_rc.h| 2 +-
drivers/infiniband/sw/rdmavt/trace_tx.h| 4 +--
drivers/misc/mei/mei-trace.h | 6 ++--
.../net/ethernet/marvell/octeontx2/af/rvu_trace.h | 12 +++
drivers/net/fjes/fjes_trace.h | 4 +--
drivers/usb/cdns3/cdnsp-trace.h| 2 +-
fs/nfs/nfs4trace.h | 6 ++--
fs/nfs/nfstrace.h | 4 +--
include/trace/events/btrfs.h | 2 +-
include/trace/events/dma_fence.h | 4 +--
include/trace/events/rpcgss.h | 4 +--
include/trace/events/sunrpc.h | 40 +++---
net/mac80211/trace.h | 2 +-
22 files changed, 70 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
index 0527772fe1b80..d855cb53c7e09 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
@@ -176,10 +176,10 @@ TRACE_EVENT(amdgpu_cs_ioctl,
TP_fast_assign(
__entry->sched_job_id = job->base.id;
- __assign_str(timeline,
AMDGPU_JOB_GET_TIMELINE_NAME(job))
+ __assign_str(timeline,
AMDGPU_JOB_GET_TIMELINE_NAME(job));
__entry->context =
job->base.s_fence->finished.context;
__entry->seqno = job->base.s_fence->finished.seqno;
- __assign_str(ring,
to_amdgpu_ring(job->base.sched)->name)
+ __assign_str(ring,
to_amdgpu_ring(job->base.sched)->name);
__entry->num_ibs = job->num_ibs;
),
TP_printk("sched_job=%llu, timeline=%s, context=%u, seqno=%u,
ring_name=%s, num_ibs=%u",
@@ -201,10 +201,10 @@ TRACE_EVENT(amdgpu_sched_run_job,
TP_fast_assign(
__entry->sched_job_id = job->base.id;
- __assign_str(timeline,
AMDGPU_JOB_GET_TIMELINE_NAME(job))
+ __assign_str(timeline,
AMDGPU_JOB_GET_TIMELINE_NAME(job));
__entry->context =
job->base.s_fence->finished.context;
__entry->seqno = job->base.s_fence->finished.seqno;
- __assign_str(ring,
to_amdgpu_ring(job->base.sched)->name)
+ __assign_str(ring,
to_amdgpu_ring(job->base.sched)->name);
__entry->num_ibs = job->num_ibs;
),
TP_printk("sched_job=%llu, timeline=%s, context=%u, seqno=%u,
ring_name=%s, num_ibs=%u",
@@ -229,7 +229,7 @@ TRACE_EVENT(amdgpu_vm_grab_id,
TP_fast_assign(
__entry->pasid = vm->pasid;
- __assign_str(ring, ring->name)
+ __assign_str(ring, ring->name);
__entry->vmid = job->vmid;
__entry->vm_hub = ring->funcs->vmhub,
__entry->pd_addr = job->vm_pd_addr;
@@ -424,7 +
[trivial PATCH] drm/amd/display: Fix typo of format termination newline
/n should be \n
Signed-off-by: Joe Perches
---
drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c | 2 +-
drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c | 2 +-
drivers/gpu/drm/amd/display/dc/dcn302/dcn302_resource.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
index 45f96221a094..b38fee783277 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn30/dcn30_resource.c
@@ -1724,7 +1724,7 @@ static bool init_soc_bounding_box(struct dc *dc,
DC_LOGGER_INIT(dc->ctx->logger);
if (!is_soc_bounding_box_valid(dc)) {
- DC_LOG_ERROR("%s: not valid soc bounding box/n", __func__);
+ DC_LOG_ERROR("%s: not valid soc bounding box\n", __func__);
return false;
}
diff --git a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c
b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c
index 5b54b7fc5105..3bf66c994dd5 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn301/dcn301_resource.c
@@ -1497,7 +1497,7 @@ static bool init_soc_bounding_box(struct dc *dc,
DC_LOGGER_INIT(dc->ctx->logger);
if (!is_soc_bounding_box_valid(dc)) {
- DC_LOG_ERROR("%s: not valid soc bounding box/n", __func__);
+ DC_LOG_ERROR("%s: not valid soc bounding box\n", __func__);
return false;
}
diff --git a/drivers/gpu/drm/amd/display/dc/dcn302/dcn302_resource.c
b/drivers/gpu/drm/amd/display/dc/dcn302/dcn302_resource.c
index fc2dea243d1b..84c61128423e 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn302/dcn302_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn302/dcn302_resource.c
@@ -1093,7 +1093,7 @@ static bool init_soc_bounding_box(struct dc *dc, struct
resource_pool *pool)
DC_LOGGER_INIT(dc->ctx->logger);
if (!is_soc_bounding_box_valid(dc)) {
- DC_LOG_ERROR("%s: not valid soc bounding box/n", __func__);
+ DC_LOG_ERROR("%s: not valid soc bounding box\n", __func__);
return false;
}
Re: [PATCH 1/1] video: hyperv_fb: Add ratelimit on error message
On Tue, 2021-04-20 at 08:44 -0700, Michael Kelley wrote:
> Due to a full ring buffer, the driver may be unable to send updates to
> the Hyper-V host. But outputing the error message can make the problem
> worse because console output is also typically written to the frame
> buffer. As a result, in some circumstances the error message is output
> continuously.
>
> Break the cycle by rate limiting the error message. Also output
> the error code for additional diagnosability.
>
> Signed-off-by: Michael Kelley
None of the callers of this function ever check the return status.
Why is important/useful to emit this message at all?
> ---
> drivers/video/fbdev/hyperv_fb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index 4dc9077..a7e6eea 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -308,7 +308,7 @@ static inline int synthvid_send(struct hv_device *hdev,
> VM_PKT_DATA_INBAND, 0);
>
>
> if (ret)
> - pr_err("Unable to send packet via vmbus\n");
> + pr_err_ratelimited("Unable to send packet via vmbus; error
> %d\n", ret);
>
>
> return ret;
> }
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 23:36 +0100, Rasmus Villemoes wrote: > On 24/03/2021 23.18, Joe Perches wrote: > > There's no silly game here. %pe would either print a string or a value. > > A hashed value, that is, never the raw value. There is value in printing the raw value. As discussed, it can simplify the code. The worry about exposing a ptr value is IMO overstated. It's trivial to inspect the uses and _all_ %p uses need inspection and validation at acceptance anyway. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 22:27 +0100, Rasmus Villemoes wrote:
> On 24/03/2021 20.24, Joe Perches wrote:
> > On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
> > > On 24/03/2021 18.20, Joe Perches wrote:
> > >
> > > >
> > > > Maybe it's better to output non PTR_ERR %pe uses as decimal so this
> > > > sort of code would work.
> > >
> > > No, because that would leak the pointer value when somebody has
> > > accidentally passed a real kernel pointer to %pe.
> >
> > I think it's not really an issue.
> >
> > _All_ code that uses %p extensions need inspection anyway.
>
> There are now a bunch of sanity checks in place that catch e.g. an
> ERR_PTR passed to an extension that would derefence the pointer;
> enforcing that only ERR_PTRs are passed to %pe (or falling back to %p)
> is another of those safeguards.
>
> > It's already possible to intentionally 'leak' the ptr value
> > by using %pe, -ptr so I think that's not really an issue.
> >
>
> Huh, what? I assume -ptr is shorthand for (void*)-(unsigned long)ptr.
> How would that leak the value if ptr is an ordinary kernel pointer?
> That's not an ERR_PTR unless (unsigned long)ptr is < 4095 or so.
You are confusing ERR_PTR with IS_ERR
ERR_PTR is just
include/linux/err.h:static inline void * __must_check ERR_PTR(long error)
include/linux/err.h-{
include/linux/err.h-return (void *) error;
include/linux/err.h-}f
> If you want to print the pointer value just do %px. No need for silly
> games.
There's no silly game here. %pe would either print a string or a value.
It already does that in 2 cases.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
> On 24/03/2021 18.20, Joe Perches wrote:
> > On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
> > > On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
> > > > On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
> > > []
> > > > > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c
> > > > > > b/drivers/gpu/drm/imx/imx-ldb.c
> > > > > []
> > > > > > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> > > > > > drm_encoder *encoder)
> > > > > > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > > > > > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child,
> > > > > > encoder);
> > > > > >
> > > > > > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > > > > > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > > > > + __func__, ERR_PTR(mux));
> > > > >
> > > > > This does not compile without warnings.
> > > > >
> > > > > drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> > > > > drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects
> > > > > argument of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> > > > > 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > > > | ^~
> > > > >
> > > > > If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> > > > > is converting an int a void * to decode the error type and
> > > > > emit it as a string.
> > > >
> > > > Sorry about that.
> > > >
> > > > I decided against using ERR_PTR() in order to also check for
> > > > positive array overflow, but the version I tested was different from
> > > > the version I sent.
> > > >
> > > > v3 coming.
> > >
> > > Thanks. No worries.
> > >
> > > Up to you, vsprintf would emit the positive mux as a funky hashed
> > > hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
> > > perhaps %d without the ERR_PTR use makes the most sense.
> > >
>
> >
> > Maybe it's better to output non PTR_ERR %pe uses as decimal so this
> > sort of code would work.
>
> No, because that would leak the pointer value when somebody has
> accidentally passed a real kernel pointer to %pe.
I think it's not really an issue.
_All_ code that uses %p extensions need inspection anyway.
It's already possible to intentionally 'leak' the ptr value
by using %pe, -ptr so I think that's not really an issue.
>
> If the code wants a cute -EFOO string explaining what's wrong, what
> about "%pe", ERR_PTR(mux < 0 : mux : -ERANGE)? Or two separate error
> messages
>
> if (mux < 0)
> ...
> else if (mux >= ARRAY_SIZE())
> ...
Multiple tests, more unnecessary code, multiple format strings, etc...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
> On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
> > On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
> []
> > > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c
> > > > b/drivers/gpu/drm/imx/imx-ldb.c
> > > []
> > > > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> > > > drm_encoder *encoder)
> > > > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > > > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child,
> > > > encoder);
> > > >
> > > > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > > > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > > + __func__, ERR_PTR(mux));
> > >
> > > This does not compile without warnings.
> > >
> > > drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> > > drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects
> > > argument of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> > > 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > | ^~
> > >
> > > If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> > > is converting an int a void * to decode the error type and
> > > emit it as a string.
> >
> > Sorry about that.
> >
> > I decided against using ERR_PTR() in order to also check for
> > positive array overflow, but the version I tested was different from
> > the version I sent.
> >
> > v3 coming.
>
> Thanks. No worries.
>
> Up to you, vsprintf would emit the positive mux as a funky hashed
> hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
> perhaps %d without the ERR_PTR use makes the most sense.
>
>
Maybe it's better to output non PTR_ERR %pe uses as decimal so this
sort of code would work.
---
lib/vsprintf.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3600db686fa4..debdd1c62038 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -619,19 +619,23 @@ static char *string_nocheck(char *buf, char *end, const
char *s,
return widen_string(buf, len, end, spec);
}
-static char *err_ptr(char *buf, char *end, void *ptr,
-struct printf_spec spec)
+static noinline_for_stack
+char *err_ptr(char *buf, char *end, void *ptr, struct printf_spec spec)
{
int err = PTR_ERR(ptr);
- const char *sym = errname(err);
- if (sym)
- return string_nocheck(buf, end, sym, spec);
+ if (IS_ERR(ptr)) {
+ const char *sym = errname(err);
+
+ if (sym)
+ return string_nocheck(buf, end, sym, spec);
+ }
/*
-* Somebody passed ERR_PTR(-1234) or some other non-existing
-* Efoo - or perhaps CONFIG_SYMBOLIC_ERRNAME=n. Fall back to
-* printing it as its decimal representation.
+* Somebody passed ERR_PTR(-1234) or some other non-existing -E
+* or perhaps CONFIG_SYMBOLIC_ERRNAME=n
+* or perhaps a positive number like an array index
+* Fall back to printing it as its decimal representation.
*/
spec.flags |= SIGN;
spec.base = 10;
@@ -2407,9 +2411,7 @@ char *pointer(const char *fmt, char *buf, char *end, void
*ptr,
case 'x':
return pointer_string(buf, end, ptr, spec);
case 'e':
- /* %pe with a non-ERR_PTR gets treated as plain %p */
- if (!IS_ERR(ptr))
- break;
+ /* %pe with a non-ERR_PTR(ptr) gets treated as %ld */
return err_ptr(buf, end, ptr, spec);
case 'u':
case 'k':
---
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
> On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
[]
> > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> > []
> > > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> > > drm_encoder *encoder)
> > > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
> > >
> > > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > + __func__, ERR_PTR(mux));
> >
> > This does not compile without warnings.
> >
> > drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> > drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects argument
> > of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> > 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > | ^~
> >
> > If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> > is converting an int a void * to decode the error type and
> > emit it as a string.
>
> Sorry about that.
>
> I decided against using ERR_PTR() in order to also check for
> positive array overflow, but the version I tested was different from
> the version I sent.
>
> v3 coming.
Thanks. No worries.
Up to you, vsprintf would emit the positive mux as a funky hashed
hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
perhaps %d without the ERR_PTR use makes the most sense.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] amdgpu: fix gcc -Wrestrict warning
On Tue, 2021-03-23 at 14:04 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> gcc warns about an sprintf() that uses the same buffer as source
> and destination, which is undefined behavior in C99:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c: In function
> 'amdgpu_securedisplay_debugfs_write':
> drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:141:6: error: 'sprintf'
> argument 3 overlaps destination object 'i2c_output' [-Werror=restrict]
> 141 | sprintf(i2c_output, "%s 0x%X", i2c_output,
> | ^~
> 142 |
> securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]);
> |
> ~
> drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c:97:7: note: destination
> object referenced by 'restrict'-qualified argument 1 was declared here
> 97 | char i2c_output[256];
> | ^~
>
> Rewrite it to remember the current offset into the buffer instead.
>
> Signed-off-by: Arnd Bergmann
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> index 834440ab9ff7..69d7f6bff5d4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
> @@ -136,9 +136,10 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct
> file *f, const char __u
> ret = psp_securedisplay_invoke(psp,
> TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);
> if (!ret) {
> if (securedisplay_cmd->status ==
> TA_SECUREDISPLAY_STATUS__SUCCESS) {
> + int pos = 0;
> memset(i2c_output, 0, sizeof(i2c_output));
> for (i = 0; i <
> TA_SECUREDISPLAY_I2C_BUFFER_SIZE; i++)
> - sprintf(i2c_output, "%s 0x%X",
> i2c_output,
> + pos += sprintf(i2c_output + pos, "
> 0x%X",
>
> securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]);
> dev_info(adev->dev, "SECUREDISPLAY: I2C buffer
> out put is :%s\n", i2c_output);
Perhaps use a hex output like:
---
drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
index 9cf856c94f94..25bb34c72d20 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c
@@ -97,13 +97,12 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct
file *f, const char __u
uint32_t op;
int i;
char str[64];
- char i2c_output[256];
int ret;
if (*pos || size > sizeof(str) - 1)
return -EINVAL;
- memset(str, 0, sizeof(str));
+ memset(str, 0, sizeof(str));
ret = copy_from_user(str, buf, size);
if (ret)
return -EFAULT;
@@ -139,11 +138,9 @@ static ssize_t amdgpu_securedisplay_debugfs_write(struct
file *f, const char __u
ret = psp_securedisplay_invoke(psp,
TA_SECUREDISPLAY_COMMAND__SEND_ROI_CRC);
if (!ret) {
if (securedisplay_cmd->status ==
TA_SECUREDISPLAY_STATUS__SUCCESS) {
- memset(i2c_output, 0, sizeof(i2c_output));
- for (i = 0; i <
TA_SECUREDISPLAY_I2C_BUFFER_SIZE; i++)
- sprintf(i2c_output, "%s 0x%X",
i2c_output,
-
securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf[i]);
- dev_info(adev->dev, "SECUREDISPLAY: I2C buffer
out put is :%s\n", i2c_output);
+ dev_info(adev->dev, "SECUREDISPLAY: I2C buffer
output is: %*ph\n",
+(int)TA_SECUREDISPLAY_I2C_BUFFER_SIZE,
+
securedisplay_cmd->securedisplay_out_message.send_roi_crc.i2c_buf);
} else {
psp_securedisplay_parse_resp_status(psp,
securedisplay_cmd->status);
}
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
On Wed, 2021-03-24 at 13:17 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> When CONFIG_OF is disabled, building with 'make W=1' produces warnings
> about out of bounds array access:
>
> drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_set_clock.constprop':
> drivers/gpu/drm/imx/imx-ldb.c:186:8: error: array subscript -22 is below
> array bounds of 'struct clk *[4]' [-Werror=array-bounds]
>
> Add an error check before the index is used, which helps with the
> warning, as well as any possible other error condition that may be
> triggered at runtime.
>
> The warning could be fixed by adding a Kconfig depedency on CONFIG_OF,
> but Liu Ying points out that the driver may hit the out-of-bounds
> problem at runtime anyway.
>
> Signed-off-by: Arnd Bergmann
> ---
> v2: fix subject line
> expand patch description
> print mux number
> check upper bound as well
[]
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
[]
> @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct drm_encoder
> *encoder)
> int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
>
> + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> + __func__, ERR_PTR(mux));
This does not compile without warnings.
drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects argument of
type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
| ^~
If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
is converting an int a void * to decode the error type and
emit it as a string.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] video: mmp: Few typo fixes
On Mon, 2021-03-22 at 12:36 -0700, Randy Dunlap wrote:
> On 3/22/21 6:02 AM, Bhaskar Chowdhury wrote:
> >
> > s/configed/configured/
> > s/registed/registered/
> > s/defintions/definitions/
> >
> > Signed-off-by: Bhaskar Chowdhury
>
> Acked-by: Randy Dunlap
[]
> > diff --git a/include/video/mmp_disp.h b/include/video/mmp_disp.h
> > index 77252cb46361..ea8b4331b7a1 100644
> > --- a/include/video/mmp_disp.h
> > +++ b/include/video/mmp_disp.h
> > @@ -172,7 +172,7 @@ struct mmp_panel {
> > /* use node to register to list */
> > struct list_head node;
> > const char *name;
> > - /* path name used to connect to proper path configed */
> > + /* path name used to connect to proper path configured */
The spelling is now correct, but the word order doesn't make much sense.
> > @@ -291,7 +291,7 @@ static inline int mmp_overlay_set_addr(struct
> > mmp_overlay *overlay,
> > * it defined a common interface that plat driver need to implement
> > */
> > struct mmp_path_info {
> > - /* driver data, set when registed*/
> > + /* driver data, set when registered*/
should have a space before */
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/imx: fix out of bounds array access warning
On Tue, 2021-03-23 at 14:05 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> When CONFIG_OF is disabled, building with 'make W=1' produces warnings
> about out of bounds array access:
>
> drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_set_clock.constprop':
> drivers/gpu/drm/imx/imx-ldb.c:186:8: error: array subscript -22 is below
> array bounds of 'struct clk *[4]' [-Werror=array-bounds]
>
> Add an error check before the index is used, which helps with the
> warning, as well as any possible other error condition that may be
> triggered at runtime.
[]
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
[]
> @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct drm_encoder
> *encoder)
> int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
>
> + if (mux < 0) {
> + dev_warn(ldb->dev,
> + "%s: invalid mux\n", __func__);
trivia:
Any real reason to make this 2 lines? It fits nicely in 80 chars. Maybe:
dev_warn(ldb->dev, "%s: invalid mux: %d\n", __func__, mux);
or maybe:
dev_warn(ldb->dev, "%s: invalid mux: %pe\n",
__func__, ERR_PTR(mux));
> @@ -255,6 +261,12 @@ imx_ldb_encoder_atomic_mode_set(struct drm_encoder
> *encoder,
[]
> + if (mux < 0) {
> + dev_warn(ldb->dev,
> + "%s: invalid mux\n", __func__);
etc...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Few typo fixes
On Thu, 2021-03-18 at 16:07 +0530, Bhaskar Chowdhury wrote: > s/instatiated/instantiated/ > s/unreference/unreferenced/ []> diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c [] > @@ -644,7 +644,7 @@ EXPORT_SYMBOL(drm_property_blob_get); > * @id: id of the blob property > * > * If successful, this takes an additional reference to the blob property. > - * callers need to make sure to eventually unreference the returned property > + * callers need to make sure to eventually unreferenced the returned property I think this is worse. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: Convert S_ permission uses to octal
Convert S_ permissions to the more readable octal.
Link:
https://lore.kernel.org/lkml/ca+55afw5v23t-zvdzp-mmd_eyxf8wbafwwb59934fv7g21u...@mail.gmail.com/
Done using:
$ git ls-files -- drivers/gpu/drm/*.[ch] |
xargs ./scripts/checkpatch.pl -f --fix-inplace --types=SYMBOLIC_PERMS
No difference in generated .o files allyesconfig x86-64
The files below were not compiled for x86-64:
drivers/gpu/drm/msm/adreno/a5xx_debugfs.c
drivers/gpu/drm/msm/msm_debugfs.c
drivers/gpu/drm/msm/msm_perf.c
drivers/gpu/drm/msm/msm_rd.c
drivers/gpu/drm/sti/sti_drv.c
checkpatch does report several places where permissions perhaps could
be downgraded. None of these locations are modified by this patch.
WARNING:EXPORTED_WORLD_WRITABLE: Exporting world writable files is usually an
error. Consider more restrictive permissions.
#165: FILE: drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1146:
+ debugfs_create_file("ras_ctrl", 0666, con->dir,
ERROR:EXPORTED_WORLD_WRITABLE: Exporting writable files is usually an error.
Consider more restrictive permissions.
#165: FILE: drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1146:
+ debugfs_create_file("ras_ctrl", 0666, con->dir,
adev, _ras_debugfs_ctrl_ops);
WARNING:EXPORTED_WORLD_WRITABLE: Exporting world writable files is usually an
error. Consider more restrictive permissions.
#168: FILE: drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1148:
+ debugfs_create_file("ras_eeprom_reset", 0666, con->dir,
ERROR:EXPORTED_WORLD_WRITABLE: Exporting writable files is usually an error.
Consider more restrictive permissions.
#168: FILE: drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1148:
+ debugfs_create_file("ras_eeprom_reset", 0666, con->dir,
adev, _ras_debugfs_eeprom_ops);
WARNING:EXPORTED_WORLD_WRITABLE: Exporting world writable files is usually an
error. Consider more restrictive permissions.
#177: FILE: drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1159:
+ debugfs_create_bool("auto_reboot", 0666, con->dir,
ERROR:EXPORTED_WORLD_WRITABLE: Exporting writable files is usually an error.
Consider more restrictive permissions.
#177: FILE: drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1159:
+ debugfs_create_bool("auto_reboot", 0666, con->dir,
>reboot);
WARNING:EXPORTED_WORLD_WRITABLE: Exporting world writable files is usually an
error. Consider more restrictive permissions.
#688: FILE: drivers/gpu/drm/msm/adreno/a5xx_debugfs.c:157:
+ debugfs_create_file("reset", 0222, minor->debugfs_root, dev,
ERROR:EXPORTED_WORLD_WRITABLE: Exporting writable files is usually an error.
Consider more restrictive permissions.
#688: FILE: drivers/gpu/drm/msm/adreno/a5xx_debugfs.c:157:
+ debugfs_create_file("reset", 0222, minor->debugfs_root, dev,
_fops);
Signed-off-by: Joe Perches
---
drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c| 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 8 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_fw_attestation.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c| 4 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c| 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_rap.c| 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c| 14 ++--
drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_securedisplay.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 10 +--
drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c | 6 +-
drivers/gpu/drm/amd/amdgpu/df_v3_6.c | 2 +-
drivers/gpu/drm/amd/pm/amdgpu_pm.c | 90 +++---
drivers/gpu/drm/amd/pm/inc/amdgpu_pm.h | 4 +-
drivers/gpu/drm/arm/malidp_drv.c | 2 +-
drivers/gpu/drm/drm_debugfs.c | 8 +-
drivers/gpu/drm/drm_debugfs_crc.c | 4 +-
drivers/gpu/drm/drm_mipi_dbi.c | 4 +-
drivers/gpu/drm/drm_sysfs.c| 2 +-
.../gpu/drm/i915/display/intel_display_debugfs.c | 10 +--
drivers/gpu/drm/i915/gvt/firmware.c| 2 +-
drivers/gpu/drm/i915/i915_debugfs.c| 4 +-
drivers/gpu/drm/i915/i915_perf.c | 2 +-
drivers/gpu/drm/i915/i915_sysfs.c | 22 +++---
drivers/gpu/drm/msm/adreno/a5xx_debugfs.c | 2 +-
drivers/gpu/drm/msm/msm_debugfs.c | 2 +-
drivers/gpu/drm/msm/msm_perf.c | 2 +-
drivers/gpu/drm/msm/msm_rd.c | 2 +-
drivers/gpu/drm/nouveau/nouveau_debugfs.c | 2 +-
drivers/gpu/drm/radeon/radeon_pm.c | 30
drivers/gpu/drm/radeon/radeon_ttm.c| 4 +-
drivers/gpu/drm/sti/sti_drv.c | 2 +-
drivers/
Re: [PATCH] drm/msm/dp: Add a missing semi-colon
On Mon, 2021-02-08 at 10:57 -0800, Stephen Boyd wrote: > Quoting Joe Perches (2021-02-06 21:06:54) > > Wow, that's really unfortunate that dp_panel_update_tu_timings > > is also void. > > > > Perhaps this as YA checkpatch warning: > > > > --- > > Acked-by: Stephen Boyd Are you acking the proposed checkpatch patch? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/msm/dp: Add a missing semi-colon
On Sat, 2021-02-06 at 20:18 -0800, Stephen Boyd wrote:
> A missing semicolon here causes my external display to stop working.
> Indeed, missing the semicolon on the return statement leads to
> dp_panel_update_tu_timings() not existing because the compiler thinks
> it's part of the return statement of a void function, so it must not be
> important.
>
> $ ./scripts/bloat-o-meter before.o after.o
> add/remove: 1/1 grow/shrink: 0/1 up/down: 7400/-7540 (-140)
> Function old new delta
> dp_panel_update_tu_timings -7400 +7400
> _dp_ctrl_calc_tu.constprop 18024 17900-124
> dp_panel_update_tu_timings.constprop7416 - -7416
> Total: Before=54440, After=54300, chg -0.26%
>
> Add a semicolon so this function works like it used to.
[]
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
[]
> @@ -631,7 +631,7 @@ static void _dp_ctrl_calc_tu(struct dp_tu_calc_input *in,
>
>
> tu = kzalloc(sizeof(*tu), GFP_KERNEL);
> if (!tu)
> - return
> + return;
>
>
> dp_panel_update_tu_timings(in, tu);
Wow, that's really unfortunate that dp_panel_update_tu_timings
is also void.
Perhaps this as YA checkpatch warning:
---
scripts/checkpatch.pl | 6 ++
1 file changed, 6 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9a549b009d2f..6df13e5a1557 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3674,6 +3674,12 @@ sub process {
}
}
+# check only a c90 keyword on the line (except else)
+ if ($sline =~ /^\+\s*($c90_Keywords)\s*$/ && $1 ne 'else') {
+ WARN("BARE_KEYWORD",
+"'$1' as the only word on a line is not good
style\n" . $herecurr);
+ }
+
# check multi-line statement indentation matches previous line
if ($perl_version_ok &&
$prevline =~ /^\+([
\t]*)((?:$c90_Keywords(?:\s+if)\s*)|(?:$Declare\s*)?(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*|(?:\*\s*)*$Lval\s*=\s*$Ident\s*)\(.*(\&\&|\|\||,)\s*$/)
{
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/29] dma-buf: Avoid comma separated statements
On Wed, 2021-02-03 at 14:26 +0100, Christian König wrote:
> Am 30.01.21 um 19:47 schrieb Joe Perches:
> > On Mon, 2020-08-24 at 21:56 -0700, Joe Perches wrote:
> > > Use semicolons and braces.
> > Ping?
> > > Signed-off-by: Joe Perches
> Reviewed-by: Christian König
>
> Do you have commit rights to drm-misc-next?
No.
> > > ---
> > > drivers/dma-buf/st-dma-fence.c | 7 +--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/dma-buf/st-dma-fence.c
> > > b/drivers/dma-buf/st-dma-fence.c
> > > index e593064341c8..c8a12d7ad71a 100644
> > > --- a/drivers/dma-buf/st-dma-fence.c
> > > +++ b/drivers/dma-buf/st-dma-fence.c
> > > @@ -471,8 +471,11 @@ static int thread_signal_callback(void *arg)
> > > dma_fence_signal(f1);
> > >
> > > smp_store_mb(cb.seen, false);
> > > - if (!f2 || dma_fence_add_callback(f2, , simple_callback))
> > > - miss++, cb.seen = true;
> > > + if (!f2 ||
> > > + dma_fence_add_callback(f2, , simple_callback)) {
> > > + miss++;
> > > + cb.seen = true;
> > > + }
> > >
> > > if (!t->before)
> > > dma_fence_signal(f1);
> >
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 10/29] drm/i915: Avoid comma separated statements
On Mon, 2020-08-24 at 21:56 -0700, Joe Perches wrote:
> Use semicolons and braces.
Ping?
> Signed-off-by: Joe Perches
> ---
> drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 8 +---
> drivers/gpu/drm/i915/gt/intel_gt_requests.c| 6 --
> drivers/gpu/drm/i915/gt/selftest_workarounds.c | 6 --
> drivers/gpu/drm/i915/intel_runtime_pm.c| 6 --
> 4 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> index 699125928272..114c13285ff1 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> @@ -323,10 +323,12 @@ static int __gen8_ppgtt_alloc(struct i915_address_space
> * const vm,
> }
>
>
> spin_lock(>lock);
> - if (likely(!pd->entry[idx]))
> + if (likely(!pd->entry[idx])) {
> set_pd_entry(pd, idx, pt);
> - else
> - alloc = pt, pt = pd->entry[idx];
> + } else {
> + alloc = pt;
> + pt = pd->entry[idx];
> + }
> }
>
>
> if (lvl) {
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> index 66fcbf9d0fdd..54408d0b5e6e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> @@ -139,8 +139,10 @@ long intel_gt_retire_requests_timeout(struct intel_gt
> *gt, long timeout)
> LIST_HEAD(free);
>
>
> interruptible = true;
> - if (unlikely(timeout < 0))
> - timeout = -timeout, interruptible = false;
> + if (unlikely(timeout < 0)) {
> + timeout = -timeout;
> + interruptible = false;
> + }
>
>
> flush_submission(gt, timeout); /* kick the ksoftirqd tasklets */
> spin_lock(>lock);
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index febc9e6692ba..3e4cbeed20bd 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -521,8 +521,10 @@ static int check_dirty_whitelist(struct intel_context
> *ce)
>
>
> srm = MI_STORE_REGISTER_MEM;
> lrm = MI_LOAD_REGISTER_MEM;
> - if (INTEL_GEN(engine->i915) >= 8)
> - lrm++, srm++;
> + if (INTEL_GEN(engine->i915) >= 8) {
> + lrm++;
> + srm++;
> + }
>
>
> pr_debug("%s: Writing garbage to %x\n",
> engine->name, reg);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 153ca9e65382..f498f1c80755 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -201,8 +201,10 @@ __print_intel_runtime_pm_wakeref(struct drm_printer *p,
> unsigned long rep;
>
>
> rep = 1;
> - while (i + 1 < dbg->count && dbg->owners[i + 1] == stack)
> - rep++, i++;
> + while (i + 1 < dbg->count && dbg->owners[i + 1] == stack) {
> + rep++;
> + i++;
> + }
> __print_depot_stack(stack, buf, PAGE_SIZE, 2);
> drm_printf(p, "Wakeref x%lu taken at:\n%s", rep, buf);
> }
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 09/29] drm/gma500: Avoid comma separated statements
On Mon, 2020-08-24 at 21:56 -0700, Joe Perches wrote:
> Use semicolons and braces.
Ping?
> Signed-off-by: Joe Perches
> ---
> drivers/gpu/drm/gma500/mdfld_intel_display.c | 44 +---
> 1 file changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/mdfld_intel_display.c
> b/drivers/gpu/drm/gma500/mdfld_intel_display.c
> index aae2d358364c..bfa330df9443 100644
> --- a/drivers/gpu/drm/gma500/mdfld_intel_display.c
> +++ b/drivers/gpu/drm/gma500/mdfld_intel_display.c
> @@ -824,33 +824,45 @@ static int mdfld_crtc_mode_set(struct drm_crtc *crtc,
> if ((ksel == KSEL_CRYSTAL_19) || (ksel == KSEL_BYPASS_19)) {
> refclk = 19200;
>
>
> - if (is_mipi || is_mipi2)
> - clk_n = 1, clk_p2 = 8;
> - else if (is_hdmi)
> - clk_n = 1, clk_p2 = 10;
> + if (is_mipi || is_mipi2) {
> + clk_n = 1;
> + clk_p2 = 8;
> + } else if (is_hdmi) {
> + clk_n = 1;
> + clk_p2 = 10;
> + }
> } else if (ksel == KSEL_BYPASS_25) {
> refclk = 25000;
>
>
> - if (is_mipi || is_mipi2)
> - clk_n = 1, clk_p2 = 8;
> - else if (is_hdmi)
> - clk_n = 1, clk_p2 = 10;
> + if (is_mipi || is_mipi2) {
> + clk_n = 1;
> + clk_p2 = 8;
> + } else if (is_hdmi) {
> + clk_n = 1;
> + clk_p2 = 10;
> + }
> } else if ((ksel == KSEL_BYPASS_83_100) &&
> dev_priv->core_freq == 166) {
> refclk = 83000;
>
>
> - if (is_mipi || is_mipi2)
> - clk_n = 4, clk_p2 = 8;
> - else if (is_hdmi)
> - clk_n = 4, clk_p2 = 10;
> + if (is_mipi || is_mipi2) {
> + clk_n = 4;
> + clk_p2 = 8;
> + } else if (is_hdmi) {
> + clk_n = 4;
> + clk_p2 = 10;
> + }
> } else if ((ksel == KSEL_BYPASS_83_100) &&
> (dev_priv->core_freq == 100 ||
> dev_priv->core_freq == 200)) {
> refclk = 10;
> - if (is_mipi || is_mipi2)
> - clk_n = 4, clk_p2 = 8;
> - else if (is_hdmi)
> - clk_n = 4, clk_p2 = 10;
> + if (is_mipi || is_mipi2) {
> + clk_n = 4;
> + clk_p2 = 8;
> + } else if (is_hdmi) {
> + clk_n = 4;
> + clk_p2 = 10;
> + }
> }
>
>
> if (is_mipi)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 08/29] dma-buf: Avoid comma separated statements
On Mon, 2020-08-24 at 21:56 -0700, Joe Perches wrote:
> Use semicolons and braces.
Ping?
> Signed-off-by: Joe Perches
> ---
> drivers/dma-buf/st-dma-fence.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c
> index e593064341c8..c8a12d7ad71a 100644
> --- a/drivers/dma-buf/st-dma-fence.c
> +++ b/drivers/dma-buf/st-dma-fence.c
> @@ -471,8 +471,11 @@ static int thread_signal_callback(void *arg)
> dma_fence_signal(f1);
>
>
> smp_store_mb(cb.seen, false);
> - if (!f2 || dma_fence_add_callback(f2, , simple_callback))
> - miss++, cb.seen = true;
> + if (!f2 ||
> + dma_fence_add_callback(f2, , simple_callback)) {
> + miss++;
> + cb.seen = true;
> + }
>
>
> if (!t->before)
> dma_fence_signal(f1);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v10] staging: fbtft: add tearing signal detect
> Comments are the exception to the "no spaces at the start of a line"
> rule. I was expecting that the kbuild-bot would send a Smatch warning
> for inconsistent indenting, but comments are not counted there either.
>
> I'm sort of surprised that we don't have checkpatch rule about the
> missing space characters. It should be: "/* Tearing Effect Line On */".
Maybe this but the "preceded by a tab" test is pretty noisy.
---
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4f8494527139..72347e82d384 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3720,6 +3720,22 @@ sub process {
s/(\(\s*$Type\s*\))[ \t]+/$1/;
}
}
+
+# Comment styles
+# Initial comment only lines that have a leading space
+ if ($rawline =~ m{^\+([ \t]+)(?:/\*|//)} && $1 =~ / /) {
+ WARN("COMMENT_STYLE",
+"Initial comment lines should be indented only
with tabs\n" . $herecurr);
+# comments not aligned on tabs
+ } elsif ($rawline !~ m{^\+(?:/\*|//)} &&
+$rawline =~ m{^\+.*[^\t](?:/\*|//)}) {
+ CHK("COMMENT_STYLE",
+ "Comments should generally be preceded by a tab\n"
. $herecurr);
+ }
+
+# comment initiators should generally be followed by a space if using words
+ if ($rawline =~ m{^\+.*(?:/\*|//)\w}) {
+ WARN("COMMENT_STYLE",
+"Comment text should use a space after the comment
initiator\n" . $herecurr);
+ }
# Block comment styles
# Networking with an initial /*
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v10] staging: fbtft: add tearing signal detect
On Wed, 2021-01-27 at 17:49 +0300, Dan Carpenter wrote: > On Wed, Jan 27, 2021 at 03:25:20PM +0100, Greg KH wrote: > > Andy and Joe, there's something wrong here that is missing the fact that > > a line is being indented with spaces and not tabs in the patch > > at > > https://lore.kernel.org/r/1611754972-151016-1-git-send-email-zhangxuez...@gmail.com > > > > Any ideas what broke? > > /*Tearing Effect Line On*/ > > Comments are the exception to the "no spaces at the start of a line" > rule. I was expecting that the kbuild-bot would send a Smatch warning > for inconsistent indenting, but comments are not counted there either. > > I'm sort of surprised that we don't have checkpatch rule about the > missing space characters. It should be: "/* Tearing Effect Line On */". You could always write your own rule... checkpatch doesn't care if a comment looks like // or /*foobarfoobarfoobar*/ ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v1 02/28] video: fbcon: Use pr_debug() in fbcon
On Fri, 2020-11-27 at 20:57 +0100, Sam Ravnborg wrote:
> Replacing DPRINTK() statements with pr_debug fixes
> W=1 warnings.
> And moves to a more standard logging setup at the same time.
[]
> diff --git a/drivers/video/fbdev/core/fbcon.c
> b/drivers/video/fbdev/core/fbcon.c
[]
> @@ -1015,9 +1007,9 @@ static const char *fbcon_startup(void)
> rows /= vc->vc_font.height;
> vc_resize(vc, cols, rows);
>
>
> - DPRINTK("mode: %s\n", info->fix.id);
> - DPRINTK("visual: %d\n", info->fix.visual);
> - DPRINTK("res:%dx%d-%d\n", info->var.xres,
> + pr_debug("mode: %s\n", info->fix.id);
> + pr_debug("visual: %d\n", info->fix.visual);
> + pr_debug("res:%dx%d-%d\n", info->var.xres,
> info->var.yres,
> info->var.bits_per_pixel);
It'd be nicer to reindent the subsequent lines too.
> @@ -3299,7 +3291,7 @@ static void fbcon_exit(void)
>
>
> if (info->queue.func)
> pending = cancel_work_sync(>queue);
> - DPRINTK("fbcon: %s pending work\n", (pending ? "canceled" :
> + pr_debug("fbcon: %s pending work\n", (pending ? "canceled" :
> "no"));
perhaps:
pr_debug("fbcon: %s pending work\n", pending ? "canceled" :
"no");
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Tue, 2020-11-24 at 11:58 +1100, Finn Thain wrote: > it's not for me to prove that such patches don't affect code > generation. That's for the patch author and (unfortunately) for reviewers. Ideally, that proof would be provided by the compilation system itself and not patch authors nor reviewers nor maintainers. Unfortunately gcc does not guarantee repeatability or deterministic output. To my knowledge, neither does clang. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, 2020-11-23 at 07:58 -0800, James Bottomley wrote: > We're also complaining about the inability to recruit maintainers: > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > And burn out: > > http://antirez.com/news/129 https://www.wired.com/story/open-source-coders-few-tired/ > What I'm actually trying to articulate is a way of measuring value of > the patch vs cost ... it has nothing really to do with who foots the > actual bill. It's unclear how to measure value in consistency. But one way that costs can be reduced is by automation and _not_ involving maintainers when the patch itself is provably correct. > One thesis I'm actually starting to formulate is that this continual > devaluing of maintainers is why we have so much difficulty keeping and > recruiting them. The linux kernel has something like 1500 different maintainers listed in the MAINTAINERS file. That's not a trivial number. $ git grep '^M:' MAINTAINERS | sort | uniq -c | wc -l 1543 $ git grep '^M:' MAINTAINERS| cut -f1 -d'<' | sort | uniq -c | wc -l 1446 I think the question you are asking is about trust and how it effects development. And back to that wired story, the actual number of what you might be considering to be maintainers is likely less than 10% of the listed numbers above. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] MAINTAINERS tag for cleanup robot
On Mon, 2020-11-23 at 09:33 +1100, Finn Thain wrote:
> On Sun, 22 Nov 2020, Joe Perches wrote:
> > But provably correct conversions IMO _should_ be done and IMO churn
> > considerations should generally have less importance.
[]
> Moreover, the patch review workload for skilled humans is being generated
> by the automation, which is completely backwards: the machine is supposed
> to be helping.
Which is why the provably correct matters.
coccinelle transforms can be, but are not necessarily, provably correct.
The _show transforms done via the sysfs_emit_dev.cocci script are correct
as in commit aa838896d87a ("drivers core: Use sysfs_emit and sysfs_emit_at
for show(device *...) functions")
Worthwhile? A different question, but I think yes as it reduces the
overall question space of the existing other sprintf overrun possibilities.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote: > On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote: > > On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote: > > > Please tell me our reward for all this effort isn't a single > > > missing error print. > > > > There were quite literally dozens of logical defects found > > by the fallthrough additions. Very few were logging only. > > So can you give us the best examples (or indeed all of them if someone > is keeping score)? hopefully this isn't a US election situation ... Gustavo? Are you running for congress now? https://lwn.net/Articles/794944/ ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote: > Please tell me > our reward for all this effort isn't a single missing error print. There were quite literally dozens of logical defects found by the fallthrough additions. Very few were logging only. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] MAINTAINERS tag for cleanup robot
On Sun, 2020-11-22 at 08:49 -0800, James Bottomley wrote: > We can enforce sysfs_emit going forwards > using tools like checkpatch It's not really possible for checkpatch to find or warn about sysfs uses of sprintf. checkpatch is really just a trivial line-by-line parser and it has no concept of code intent. It just can't warn on every use of the sprintf family. There are just too many perfectly valid uses. > but there's no benefit and a lot of harm to > be done by trying to churn the entire tree Single uses of sprintf for sysfs is not really any problem. But likely there are still several possible overrun sprintf/snprintf paths in sysfs. Some of them are very obscure and unlikely to be found by a robot as the logic for sysfs buf uses can be fairly twisty. But provably correct conversions IMO _should_ be done and IMO churn considerations should generally have less importance. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] MAINTAINERS tag for cleanup robot
On Sun, 2020-11-22 at 08:33 -0800, Tom Rix wrote: > On 11/21/20 9:10 AM, Joe Perches wrote: > > On Sat, 2020-11-21 at 08:50 -0800, t...@redhat.com wrote: > > > A difficult part of automating commits is composing the subsystem > > > preamble in the commit log. For the ongoing effort of a fixer producing > > > one or two fixes a release the use of 'treewide:' does not seem > > > appropriate. > > > > > > It would be better if the normal prefix was used. Unfortunately normal is > > > not consistent across the tree. > > > > > > So I am looking for comments for adding a new tag to the MAINTAINERS file > > > > > > D: Commit subsystem prefix > > > > > > ex/ for FPGA DFL DRIVERS > > > > > > D: fpga: dfl: > > I'm all for it. Good luck with the effort. It's not completely trivial. > > > > From a decade ago: > > > > https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/ > > > > (and that thread started with extra semicolon patches too) > > Reading the history, how about this. > > get_maintainer.pl outputs a single prefix, if multiple files have the > same prefix it works, if they don't its an error. > > Another script 'commit_one_file.sh' does the call to get_mainainter.pl > to get the prefix and be called by run-clang-tools.py to get the fixer > specific message. It's not whether the script used is get_maintainer or any other script, the question is really if the MAINTAINERS file is the appropriate place to store per-subsystem patch specific prefixes. It is. Then the question should be how are the forms described and what is the inheritance priority. My preference would be to have a default of inherit the parent base and add basename(subsystem dirname). Commit history seems to have standardized on using colons as the separator between the commit prefix and the subject. A good mechanism to explore how various subsystems have uses prefixes in the past might be something like: $ git log --no-merges --pretty='%s' - | \ perl -n -e 'print substr($_, 0, rindex($_, ":") + 1) . "\n";' | \ sort | uniq -c | sort -rn Using 1 for commit_count and drivers/scsi for subsystem_path, the top 40 entries are below: About 1% don't have a colon, and there is no real consistency even within individual drivers below scsi. For instance, qla2xxx: 1 814 scsi: qla2xxx: 2 691 scsi: lpfc: 3 389 scsi: hisi_sas: 4 354 scsi: ufs: 5 339 scsi: 6 291 qla2xxx: 7 256 scsi: megaraid_sas: 8 249 scsi: mpt3sas: 9 200 hpsa: 10 190 scsi: aacraid: 11 174 lpfc: 12 153 scsi: qedf: 13 144 scsi: smartpqi: 14 139 scsi: cxlflash: 15 122 scsi: core: 16 110 [SCSI] qla2xxx: 17 108 ncr5380: 18 98 scsi: hpsa: 19 97 20 89 treewide: 21 88 mpt3sas: 22 86 scsi: libfc: 23 85 scsi: qedi: 24 84 scsi: be2iscsi: 25 81 [SCSI] qla4xxx: 26 81 hisi_sas: 27 81 block: 28 75 megaraid_sas: 29 71 scsi: sd: 30 69 [SCSI] hpsa: 31 68 cxlflash: 32 65 scsi: libsas: 33 65 scsi: fnic: 34 61 scsi: scsi_debug: 35 60 scsi: arcmsr: 36 57 be2iscsi: 37 53 atp870u: 38 51 scsi: bfa: 39 50 scsi: storvsc: 40 48 sd: ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] MAINTAINERS tag for cleanup robot
On Sat, 2020-11-21 at 09:18 -0800, James Bottomley wrote: > On Sat, 2020-11-21 at 08:50 -0800, t...@redhat.com wrote: > > A difficult part of automating commits is composing the subsystem > > preamble in the commit log. For the ongoing effort of a fixer > > producing one or two fixes a release the use of 'treewide:' does > > not seem appropriate. > > > > It would be better if the normal prefix was used. Unfortunately > > normal is not consistent across the tree. > > > > D: Commit subsystem prefix > > > > ex/ for FPGA DFL DRIVERS > > > > D: fpga: dfl: > > I've got to bet this is going to cause more issues than it solves. > SCSI uses scsi: : for drivers but not every driver has a > MAINTAINERS entry. We use either scsi: or scsi: core: for mid layer > things, but we're not consistent. Block uses blk-: for all > of it's stuff but almost no s have a MAINTAINERS entry. So > the next thing you're going to cause is an explosion of suggested > MAINTAINERs entries. As well as some changes require simultaneous changes across multiple subsystems. > Has anyone actually complained about treewide:? It depends on what you mean by treewide: If a treewide: patch is applied by some "higher level" maintainer, then generally, no. If the treewide patch is also cc'd to many individual maintainers, then yes, many many times. Mostly because patches cause what is in their view churn or that changes are not specific to their subsystem grounds. The treewide patch is sometimes dropped, sometimes broken up and generally not completely applied. What would be useful in many cases like this is for a pre and post application of the treewide patch to be compiled and the object code verified for lack of any logic change. Unfortunately, gcc does not guarantee deterministic compilation so this isn't feasible with at least gcc. Does clang guarantee this? I'm not sure it's possible: https://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] MAINTAINERS tag for cleanup robot
On Sat, 2020-11-21 at 08:50 -0800, t...@redhat.com wrote:
> A difficult part of automating commits is composing the subsystem
> preamble in the commit log. For the ongoing effort of a fixer producing
> one or two fixes a release the use of 'treewide:' does not seem appropriate.
>
> It would be better if the normal prefix was used. Unfortunately normal is
> not consistent across the tree.
>
> So I am looking for comments for adding a new tag to the MAINTAINERS file
>
> D: Commit subsystem prefix
>
> ex/ for FPGA DFL DRIVERS
>
> D: fpga: dfl:
I'm all for it. Good luck with the effort. It's not completely trivial.
>From a decade ago:
https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/
(and that thread started with extra semicolon patches too)
> Continuing with cleaning up clang's -Wextra-semi-stmt
> diff --git a/Makefile b/Makefile
[]
> @@ -1567,20 +1567,21 @@ help:
> echo ''
> @echo 'Static analysers:'
> @echo ' checkstack - Generate a list of stack hogs'
> @echo ' versioncheck- Sanity check on version.h usage'
> @echo ' includecheck- Check for duplicate included header files'
> @echo ' export_report - List the usages of all exported symbols'
> @echo ' headerdep - Detect inclusion cycles in headers'
> @echo ' coccicheck - Check with Coccinelle'
> @echo ' clang-analyzer - Check with clang static analyzer'
> @echo ' clang-tidy - Check with clang-tidy'
> + @echo ' clang-tidy-fix - Check and fix with clang-tidy'
A pity the ordering of the code below isn't the same as the above.
> -PHONY += clang-tidy clang-analyzer
> +PHONY += clang-tidy-fix clang-tidy clang-analyzer
[]
> -clang-tidy clang-analyzer: $(extmod-prefix)compile_commands.json
> +clang-tidy-fix clang-tidy clang-analyzer:
> $(extmod-prefix)compile_commands.json
> $(call cmd,clang_tools)
> else
> -clang-tidy clang-analyzer:
> +clang-tidy-fix clang-tidy clang-analyzer:
[]
> diff --git a/scripts/clang-tools/run-clang-tools.py
> b/scripts/clang-tools/run-clang-tools.py
[]
> @@ -22,43 +22,57 @@ def parse_arguments():
[]
> parser.add_argument("type",
> -choices=["clang-tidy", "clang-analyzer"],
> +choices=["clang-tidy-fix", "clang-tidy",
> "clang-analyzer"],
etc...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Fri, 2020-11-20 at 12:21 -0600, Gustavo A. R. Silva wrote: > Hi all, > > This series aims to fix almost all remaining fall-through warnings in > order to enable -Wimplicit-fallthrough for Clang. > > In preparation to enable -Wimplicit-fallthrough for Clang, explicitly > add multiple break/goto/return/fallthrough statements instead of just > letting the code fall through to the next case. > > Notice that in order to enable -Wimplicit-fallthrough for Clang, this > change[1] is meant to be reverted at some point. So, this patch helps > to move in that direction. This was a bit hard to parse for a second or three. Thanks Gustavo. How was this change done? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/40] drm/amd/include/vega10_ip_offset: Mark _BASE structs as __maybe_unused
On Fri, 2020-11-13 at 13:48 +, Lee Jones wrote:
> This patch fixes nearly 400 warnings!
>
> These structures are too widely used in too many varying
> configurations to be split-up into different headers or moved into
> source files.
>
> Instead, we'll mark them as __maybe_unused which tells the compiler
> that we're aware they're being included into source files which do not
> make use of them - but we've looked into it, and it's okay.
https://gcc.gnu.org/onlinedocs/gcc-4.7.2/gcc/Type-Attributes.html#Type-Attributes
Wouldn't it be simpler to mark the struct definitions as maybe_unused
instead of the declarations?
And perhaps remove all the unnecessary zeroed declarations?
Something like this example?
---
drivers/gpu/drm/amd/include/arct_ip_offset.h | 353 +++
1 file changed, 145 insertions(+), 208 deletions(-)
diff --git a/drivers/gpu/drm/amd/include/arct_ip_offset.h
b/drivers/gpu/drm/amd/include/arct_ip_offset.h
index a7791a9e1f90..9f2d6b832dd9 100644
--- a/drivers/gpu/drm/amd/include/arct_ip_offset.h
+++ b/drivers/gpu/drm/amd/include/arct_ip_offset.h
@@ -33,215 +33,152 @@ struct IP_BASE_INSTANCE
struct IP_BASE
{
struct IP_BASE_INSTANCE instance[MAX_INSTANCE];
-};
-
-
-static const struct IP_BASE ATHUB_BASE={ { { { 0x0C20,
0x00012460, 0x00408C00, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } } } };
-static const struct IP_BASE CLK_BASE={ { { { 0x000120C0,
0x00016C00, 0x00401800, 0, 0, 0 } },
-{ { 0x000120E0, 0x00016E00,
0x00401C00, 0, 0, 0 } },
-{ { 0x00012100, 0x00017000,
0x00402000, 0, 0, 0 } },
-{ { 0x00012120, 0x00017200,
0x00402400, 0, 0, 0 } },
-{ { 0x000136C0, 0x0001B000,
0x0042D800, 0, 0, 0 } },
-{ { 0x00013720, 0x0001B200,
0x0042E400, 0, 0, 0 } },
-{ { 0x000125E0, 0x00017E00,
0x0040BC00, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } } } };
-static const struct IP_BASE DF_BASE={ { { { 0x7000,
0x000125C0, 0x0040B800, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } } } };
-static const struct IP_BASE FUSE_BASE={ { { { 0x000120A0,
0x00017400, 0x00401400, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } } } };
-static const struct IP_BASE GC_BASE={ { { { 0x2000,
0xA000, 0x00012160, 0x00402C00, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } } } };
-static const struct IP_BASE HDP_BASE={ { { { 0x0F20,
0x00012520, 0x0040A400, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } },
-{ { 0, 0, 0, 0, 0, 0 } } } };
-static const struct IP_BASE MMHUB_BASE={ { { { 0x00012440,
0x0001A000, 0x00408800, 0,
Re: [PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
On Fri, 2020-11-13 at 12:54 +0200, Sakari Ailus wrote:
> Hi Joe,
>
> On Tue, Nov 03, 2020 at 08:49:36AM -0800, Joe Perches wrote:
> > On Tue, 2020-11-03 at 16:56 +0200, Sakari Ailus wrote:
> > > On Tue, Nov 03, 2020 at 04:47:47PM +0200, Andy Shevchenko wrote:
> > > > On Tue, Nov 03, 2020 at 03:34:00PM +0200, Sakari Ailus wrote:
> > > > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and
> > > > > DRM
> > > > > pixel formats denoted by fourccs. The fourcc encoding is the same for
> > > > > both
> > > > > so the same implementation can be used.
> > > >
> > > > ...
> > > >
> > > > > +static noinline_for_stack
> > > > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > > > + struct printf_spec spec, const char *fmt)
> > > > > +{
> > > > > + char output[sizeof("(xx)(xx)(xx)(xx) little-endian
> > > > > (0x01234567)")];
> > > >
> > > > I would add a comment that there is another possibility, i.e.
> > > > big-endian, but
> > > > it occupies less space.
> >
> > I think it's unnecessary as it's obvious and similarly done in other
> > _string type functions.
> >
> > > > > + p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
> > > > > +sizeof(u32));
> > > >
> > > > I would go with one line here.
> > >
> > > It's wrapped since the result would be over 80 otherwise.
> >
> > Perhaps simpler as
> >
> > p = special_hex_number(p, p + 10, *fourcc, sizeof(u32));
>
> Yes. But having bugs elsewhere would have a magnified effect.
How's that? Where would "elsewhere" be?
> I wouldn't be afraid of a newline here.
I'd prefer obvious code instead of indirected p vs output
and having to lookup whatever output is again.
special_hex_number is already known to fit in the buffer.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Subject: [RFC] clang tooling cleanups
On Tue, 2020-10-27 at 09:42 -0700, t...@redhat.com wrote:
> This rfc will describe
> An upcoming treewide cleanup.
> How clang tooling was used to programatically do the clean up.
> Solicit opinions on how to generally use clang tooling.
>
> The clang warning -Wextra-semi-stmt produces about 10k warnings.
> Reviewing these, a subset of semicolon after a switch looks safe to
> fix all the time. An example problem
>
> void foo(int a) {
> switch(a) {
> case 1:
> ...
> }; <--- extra semicolon
> }
>
> Treewide, there are about 100 problems in 50 files for x86_64 allyesconfig.
> These fixes will be the upcoming cleanup.
coccinelle already does some of these.
For instance: scripts/coccinelle/misc/semicolon.cocci
Perhaps some tool coordination can be done here as
coccinelle/checkpatch/clang/Lindent call all be used
to do some facet or another of these cleanup issues.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
On Tue, 2020-11-03 at 16:56 +0200, Sakari Ailus wrote:
> On Tue, Nov 03, 2020 at 04:47:47PM +0200, Andy Shevchenko wrote:
> > On Tue, Nov 03, 2020 at 03:34:00PM +0200, Sakari Ailus wrote:
> > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
> > > pixel formats denoted by fourccs. The fourcc encoding is the same for both
> > > so the same implementation can be used.
> >
> > ...
> >
> > > +static noinline_for_stack
> > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > + struct printf_spec spec, const char *fmt)
> > > +{
> > > + char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")];
> >
> > I would add a comment that there is another possibility, i.e. big-endian,
> > but
> > it occupies less space.
I think it's unnecessary as it's obvious and similarly done in other
_string type functions.
> > > + p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
> > > +sizeof(u32));
> >
> > I would go with one line here.
>
> It's wrapped since the result would be over 80 otherwise.
Perhaps simpler as
p = special_hex_number(p, p + 10, *fourcc, sizeof(u32));
> > The (theoretical) problem is here that the case when buffer size is not
> > enough
> > to print a value will be like '(0xabc)' but should be rather '(0xabcd' like
> > snprintf() does in general.
Isn't the stack buffer known to be large enough?
> > > + *p++ = ')';
> > > + *p = '\0';
> > > +
> > > + return string(buf, end, output, spec);
Isn't the actual output buffer used here truncating output?
If the general problem is someone using a limited length pointer
output like %10p4cc, then all the output is getting truncated no?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/8] slab: provide and use krealloc_array()
On Mon, 2020-11-02 at 16:20 +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski > > Andy brought to my attention the fact that users allocating an array of > equally sized elements should check if the size multiplication doesn't > overflow. This is why we have helpers like kmalloc_array(). > > However we don't have krealloc_array() equivalent and there are many > users who do their own multiplication when calling krealloc() for arrays. > > This series provides krealloc_array() and uses it in a couple places. My concern about this is a possible assumption that __GFP_ZERO will work, and as far as I know, it will not. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Outreachy kernel] [PATCH] drm/amdgpu: use DEFINE_DEBUGFS_ATTRIBUTE with debugfs_create_file_unsafe()
On Fri, 2020-10-30 at 09:03 +0100, Greg KH wrote: > On Fri, Oct 30, 2020 at 01:27:16PM +0530, Deepak R Varma wrote: > > On Fri, Oct 30, 2020 at 08:11:20AM +0100, Greg KH wrote: > > > On Fri, Oct 30, 2020 at 08:52:45AM +0530, Deepak R Varma wrote: > > > > Using DEFINE_DEBUGFS_ATTRIBUTE macro with debugfs_create_file_unsafe() > > > > function in place of the debugfs_create_file() function will make the > > > > file operation struct "reset" aware of the file's lifetime. Additional > > > > details here: https://lists.archive.carbon60.com/linux/kernel/2369498 > > > > > > > > Issue reported by Coccinelle script: > > > > scripts/coccinelle/api/debugfs/debugfs_simple_attr.cocci [] > There is a reason we didn't just do a global search/replace for this in > the kernel when the new functions were added, so I don't know why > checkpatch is now saying it must be done. I think it's not a checkpatch warning here. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/8] vhost: vringh: use krealloc_array()
On Tue, 2020-10-27 at 17:58 +0100, Bartosz Golaszewski wrote:
> On Tue, Oct 27, 2020 at 5:50 PM Joe Perches wrote:
> >
> > On Tue, 2020-10-27 at 11:28 -0400, Michael S. Tsirkin wrote:
> > > On Tue, Oct 27, 2020 at 01:17:20PM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski
> > > >
> > > > Use the helper that checks for overflows internally instead of manually
> > > > calculating the size of the new array.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski
> > >
> > > No problem with the patch, it does introduce some symmetry in the code.
> >
> > Perhaps more symmetry by using kmemdup
> > ---
> > drivers/vhost/vringh.c | 23 ++-
> > 1 file changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > index 8bd8b403f087..99222a3651cd 100644
> > --- a/drivers/vhost/vringh.c
> > +++ b/drivers/vhost/vringh.c
> > @@ -191,26 +191,23 @@ static int move_to_indirect(const struct vringh *vrh,
> > static int resize_iovec(struct vringh_kiov *iov, gfp_t gfp)
> > {
> > struct kvec *new;
> > - unsigned int flag, new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED)
> > * 2;
> > + size_t new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2;
> > + size_t size;
> >
> > if (new_num < 8)
> > new_num = 8;
> >
> > - flag = (iov->max_num & VRINGH_IOV_ALLOCATED);
> > - if (flag)
> > - new = krealloc(iov->iov, new_num * sizeof(struct iovec),
> > gfp);
> > - else {
> > - new = kmalloc_array(new_num, sizeof(struct iovec), gfp);
> > - if (new) {
> > - memcpy(new, iov->iov,
> > - iov->max_num * sizeof(struct iovec));
> > - flag = VRINGH_IOV_ALLOCATED;
> > - }
> > - }
> > + if (unlikely(check_mul_overflow(new_num, sizeof(struct iovec),
> > )))
> > + return -ENOMEM;
> > +
>
> The whole point of using helpers such as kmalloc_array() is not doing
> these checks manually.
Tradeoffs for in readability for overflow and not mistyping or doing
the multiplication of iov->max_num * sizeof(struct iovec) twice.
Just fyi:
the realloc doesn't do a multiplication overflow test as written so the
suggestion is slightly more resistant to defect.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/8] vhost: vringh: use krealloc_array()
On Tue, 2020-10-27 at 11:28 -0400, Michael S. Tsirkin wrote:
> On Tue, Oct 27, 2020 at 01:17:20PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski
> >
> > Use the helper that checks for overflows internally instead of manually
> > calculating the size of the new array.
> >
> > Signed-off-by: Bartosz Golaszewski
>
> No problem with the patch, it does introduce some symmetry in the code.
Perhaps more symmetry by using kmemdup
---
drivers/vhost/vringh.c | 23 ++-
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
index 8bd8b403f087..99222a3651cd 100644
--- a/drivers/vhost/vringh.c
+++ b/drivers/vhost/vringh.c
@@ -191,26 +191,23 @@ static int move_to_indirect(const struct vringh *vrh,
static int resize_iovec(struct vringh_kiov *iov, gfp_t gfp)
{
struct kvec *new;
- unsigned int flag, new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2;
+ size_t new_num = (iov->max_num & ~VRINGH_IOV_ALLOCATED) * 2;
+ size_t size;
if (new_num < 8)
new_num = 8;
- flag = (iov->max_num & VRINGH_IOV_ALLOCATED);
- if (flag)
- new = krealloc(iov->iov, new_num * sizeof(struct iovec), gfp);
- else {
- new = kmalloc_array(new_num, sizeof(struct iovec), gfp);
- if (new) {
- memcpy(new, iov->iov,
- iov->max_num * sizeof(struct iovec));
- flag = VRINGH_IOV_ALLOCATED;
- }
- }
+ if (unlikely(check_mul_overflow(new_num, sizeof(struct iovec), )))
+ return -ENOMEM;
+
+ if (iov->max_num & VRINGH_IOV_ALLOCATED)
+ new = krealloc(iov->iov, size, gfp);
+ else
+ new = kmemdup(iov->iov, size, gfp);
if (!new)
return -ENOMEM;
iov->iov = new;
- iov->max_num = (new_num | flag);
+ iov->max_num = new_num | VRINGH_IOV_ALLOCATED;
return 0;
}
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] treewide: cleanup unreachable breaks
On Mon, 2020-10-19 at 12:42 -0700, Nick Desaulniers wrote: > On Sat, Oct 17, 2020 at 10:43 PM Greg KH wrote: > > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > > From: Tom Rix > > > > > > This is a upcoming change to clean up a new warning treewide. > > > I am wondering if the change could be one mega patch (see below) or > > > normal patch per file about 100 patches or somewhere half way by > > > collecting > > > early acks. > > > > Please break it up into one-patch-per-subsystem, like normal, and get it > > merged that way. > > > > Sending us a patch, without even a diffstat to review, isn't going to > > get you very far... > > Tom, > If you're able to automate this cleanup, I suggest checking in a > script that can be run on a directory. Then for each subsystem you > can say in your commit "I ran scripts/fix_whatever.py on this subdir." > Then others can help you drive the tree wide cleanup. Then we can > enable -Wunreachable-code-break either by default, or W=2 right now > might be a good idea. > > Ah, George (gbiv@, cc'ed), did an analysis recently of > `-Wunreachable-code-loop-increment`, `-Wunreachable-code-break`, and > `-Wunreachable-code-return` for Android userspace. From the review: > ``` > Spoilers: of these, it seems useful to turn on > -Wunreachable-code-loop-increment and -Wunreachable-code-return by > default for Android > ... > While these conventions about always having break arguably became > obsolete when we enabled -Wfallthrough, my sample turned up zero > potential bugs caught by this warning, and we'd need to put a lot of > effort into getting a clean tree. So this warning doesn't seem to be > worth it. > ``` > Looks like there's an order of magnitude of `-Wunreachable-code-break` > than the other two. > > We probably should add all 3 to W=2 builds (wrapped in cc-option). > I've filed https://github.com/ClangBuiltLinux/linux/issues/1180 to > follow up on. I suggest using W=1 as people that are doing cleanups generally use that and not W=123 or any other style. Every other use of W= is still quite noisy and these code warnings are relatively trivially to fix up. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Ocfs2-devel] [RFC] treewide: cleanup unreachable breaks
On Sun, 2020-10-18 at 19:59 +0100, Matthew Wilcox wrote: > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > clang has a number of useful, new warnings see > > https://urldefense.com/v3/__https://clang.llvm.org/docs/DiagnosticsReference.html__;!!GqivPVa7Brio!Krxz78O3RKcB9JBMVo_F98FupVhj_jxX60ddN6tKGEbv_cnooXc1nnBmchm-e_O9ieGnyQ$ > > > > Please get your IT department to remove that stupidity. If you can't, > please send email from a non-Red Hat email address. I didn't get it this way, neither did lore. It's on your end. https://lore.kernel.org/lkml/20201017160928.12698-1-t...@redhat.com/ > I don't understand why this is a useful warning to fix. Precision in coding style intent and code minimization would be the biggest factors IMO. > What actual problem is caused by the code below? Obviously none. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Cocci] [RFC] treewide: cleanup unreachable breaks
On Sat, 2020-10-17 at 20:21 +0200, Julia Lawall wrote: > On Sat, 17 Oct 2020, Joe Perches wrote: > > On Sat, 2020-10-17 at 09:09 -0700, t...@redhat.com wrote: > > > From: Tom Rix > > > > > > This is a upcoming change to clean up a new warning treewide. > > > I am wondering if the change could be one mega patch (see below) or > > > normal patch per file about 100 patches or somewhere half way by > > > collecting > > > early acks. > > > > > > clang has a number of useful, new warnings see > > > https://clang.llvm.org/docs/DiagnosticsReference.html > > > > > > This change cleans up -Wunreachable-code-break > > > https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break > > > for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64. > > > > Early acks/individual patches by subsystem would be good. > > Better still would be an automated cocci script. > > Coccinelle is not especially good at this, because it is based on control > flow, and a return or goto diverts the control flow away from the break. > A hack to solve the problem is to put an if around the return or goto, but > that gives the break a meaningless file name and line number. I collected > the following list, but it only has 439 results, so fewer than clang. But > maybe there are some files that are not considered by clang in the x86 > allyesconfig configuration. > > Probably checkpatch is the best solution here, since it is not > configuration sensitive and doesn't care about control flow. Likely the clang compiler is the best option here. It might be useful to add -Wunreachable-code-break to W=1 or just always enable it if it isn't already enabled. diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index 95e4cdb94fe9..3819787579d5 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -32,6 +32,7 @@ KBUILD_CFLAGS += $(call cc-option, -Wunused-but-set-variable) KBUILD_CFLAGS += $(call cc-option, -Wunused-const-variable) KBUILD_CFLAGS += $(call cc-option, -Wpacked-not-aligned) KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation) +KBUILD_CFLAGS += $(call cc-option, -Wunreachable-code-break) # The following turn off the warnings enabled by -Wextra KBUILD_CFLAGS += -Wno-missing-field-initializers KBUILD_CFLAGS += -Wno-sign-compare (and thank you Tom for pushing this forward) checkpatch can't find instances like: case FOO: if (foo) return 1; else return 2; break; As it doesn't track flow and relies on the number of tabs to be the same for any goto/return and break; checkpatch will warn on: case FOO: ... goto bar; break; ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC] treewide: cleanup unreachable breaks
On Sat, 2020-10-17 at 09:09 -0700, t...@redhat.com wrote: > From: Tom Rix > > This is a upcoming change to clean up a new warning treewide. > I am wondering if the change could be one mega patch (see below) or > normal patch per file about 100 patches or somewhere half way by collecting > early acks. > > clang has a number of useful, new warnings see > https://clang.llvm.org/docs/DiagnosticsReference.html > > This change cleans up -Wunreachable-code-break > https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break > for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64. Early acks/individual patches by subsystem would be good. Better still would be an automated cocci script. The existing checkpatch test for UNNECESSARY_BREAK has a few too many false positives. >From a script run on next on July 28th: arch/arm/mach-s3c24xx/mach-rx1950.c:266: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/arm/nwfpe/fpa11_cprt.c:38: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/arm/nwfpe/fpa11_cprt.c:41: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:684: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:687: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:690: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:693: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:697: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:700: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:276: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:279: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:282: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:287: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:290: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/rb532/setup.c:76: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/rb532/setup.c:79: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:231: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:234: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:237: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:240: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/net/bpf_jit_comp.c:455: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/platforms/cell/spufs/switch.c:2047: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/platforms/cell/spufs/switch.c:2077: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/sh/boards/mach-landisk/gio.c:111: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/mce/core.c:1734: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/mce/core.c:1738: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/microcode/amd.c:218: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:107: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:132: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:147: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:158: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/ata/libata-scsi.c:3973: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/base/power/main.c:366: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/block/xen-blkback/blkback.c:1272: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/ipmi/ipmi_devintf.c:493: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/lp.c:625: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/mwave/mwavedd.c:406: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return
Re: [PATCH] drm/fourcc: Add AXBXGXRX106106106106 format
On Wed, 2020-10-07 at 10:27 +0100, Matteo Franchin wrote:
> Add ABGR format with 10-bit components packed in 64-bit per pixel.
> This format can be used to handle
> VK_FORMAT_R10X6G10X6B10X6A10X6_UNORM_4PACK16 on little-endian
> architectures.
trivial note:
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
[]
> @@ -202,6 +202,7 @@ const struct drm_format_info *__drm_format_info(u32
> format)
> { .format = DRM_FORMAT_XBGR16161616F, .depth = 0,
> .num_planes = 1, .cpp = { 8, 0, 0 }, .hsub = 1, .vsub = 1 },
> { .format = DRM_FORMAT_ARGB16161616F, .depth = 0,
> .num_planes = 1, .cpp = { 8, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true
> },
> { .format = DRM_FORMAT_ABGR16161616F, .depth = 0,
> .num_planes = 1, .cpp = { 8, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true
> },
> + { .format = DRM_FORMAT_AXBXGXRX106106106106,.depth = 0,
> .num_planes = 1, .cpp = { 8, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true
> },
My is to separate this into 2 lines so every
column including .depth on still visually aligns.
+ { .format = DRM_FORMAT_AXBXGXRX106106106106,
+ .depth = 0,
.num_planes = 1, .cpp = { 8, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH] DRM: amd: powerplay: don't undef pr_warn() {causes ARC build errors}
On Mon, 2020-10-05 at 21:50 -0700, Randy Dunlap wrote: > From: Randy Dunlap > > arch/arc/ implements BUG_ON() with BUG(). ARC has its own BUG() > function and that function uses pr_warn() as part of its implementation. > > Several (8) files in amd/powerplay/ #undef various pr_xyz() functions so > that they won't be used by these drivers, since dev_() functions are > preferred here and the #undefs make the pr_() functions unavailable. [] > --- lnx-59-rc7.orig/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > +++ lnx-59-rc7/drivers/gpu/drm/amd/powerplay/navi10_ppt.c > @@ -52,7 +52,7 @@ > * They are more MGPU friendly. > */ > #undef pr_err > -#undef pr_warn > +//#undef pr_warn > #undef pr_info > #undef pr_debug > > --- lnx-59-rc7.orig/drivers/gpu/drm/amd/powerplay/sienna_cichlid_ppt.c > +++ lnx-59-rc7/drivers/gpu/drm/amd/powerplay/sienna_cichlid_ppt.c > @@ -54,7 +54,7 @@ > * They are more MGPU friendly. > */ > #undef pr_err > -#undef pr_warn > +//#undef pr_warn > #undef pr_info > #undef pr_debug These are bad ideas as all of these pr_ entries may become functions in a near-term future. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/stm: dsi: Use dev_ based logging
On Fri, 2020-09-25 at 12:22 +0200, Yannick Fertre wrote:
> Standardize on the dev_ based logging and drop the include of drm_print.h.
> Remove useless dsi_color_from_mipi function.
[]
> diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
[]
> - DRM_DEBUG_DRIVER("pll_in %ukHz pll_out %ukHz lane_mbps %uMHz\n",
> - pll_in_khz, pll_out_khz, *lane_mbps);
> + dev_dbg(dsi->dev, "pll_in %ukHz pll_out %ukHz lane_mbps %uMHz\n",
> pll_in_khz, pll_out_khz,
> + *lane_mbps);
The line wrapping change here is pretty pointless and IMO
makes the code harder to read.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On Thu, 2020-09-10 at 15:21 +0100, Robin Murphy wrote:
> On 2020-09-09 21:06, Joe Perches wrote:
> > fallthrough to a separate case/default label break; isn't very readable.
> >
> > Convert pseudo-keyword fallthrough; statements to a simple break; when
> > the next label is case or default and the only statement in the next
> > label block is break;
> >
> > Found using:
> >
> > $ grep-2.5.4 -rP --include=*.[ch] -n
> > "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
> >
> > Miscellanea:
> >
> > o Move or coalesce a couple label blocks above a default: block.
> >
> > Signed-off-by: Joe Perches
> > ---
> >
> > Compiled allyesconfig x86-64 only.
> > A few files for other arches were not compiled.
> >
>
> [...]
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index c192544e874b..743db1abec40 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -3777,7 +3777,7 @@ static int arm_smmu_device_hw_probe(struct
> > arm_smmu_device *smmu)
> > switch (FIELD_GET(IDR0_TTF, reg)) {
> > case IDR0_TTF_AARCH32_64:
> > smmu->ias = 40;
> > - fallthrough;
> > + break;
> > case IDR0_TTF_AARCH64:
> > break;
> > default:
>
> I have to say I don't really agree with the readability argument for
> this one - a fallthrough is semantically correct here, since the first
> case is a superset of the second. It just happens that anything we would
> do for the common subset is implicitly assumed (there are other
> potential cases we simply haven't added support for at the moment), thus
> the second case is currently empty.
> This change actively obfuscates that distinction.
Then perhaps comments should be added to usefully
describe the mechanisms.
case IDR0_TTF_AARCH32_64:
smmu->ias = 40;
fallthrough;/* and still do the 64 bit processing */
case IDR0_TTF_AARCH64:
/* Nothing specific yet */
break;
> Robin.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
On Wed, 2020-09-09 at 19:36 -0300, Jason Gunthorpe wrote:
> On Wed, Sep 09, 2020 at 01:06:39PM -0700, Joe Perches wrote:
> > fallthrough to a separate case/default label break; isn't very readable.
> >
> > Convert pseudo-keyword fallthrough; statements to a simple break; when
> > the next label is case or default and the only statement in the next
> > label block is break;
> >
> > Found using:
> >
> > $ grep-2.5.4 -rP --include=*.[ch] -n
> > "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
> >
> > Miscellanea:
> >
> > o Move or coalesce a couple label blocks above a default: block.
> >
> > Signed-off-by: Joe Perches
> > ---
> >
> > Compiled allyesconfig x86-64 only.
> > A few files for other arches were not compiled.
>
> IB part looks OK, I prefer it like this
>
> You could do the same for continue as well, I saw a few of those..
I saw some continue uses as well but wasn't sure
and didn't look to see if the switch/case with
continue was in a for/while loop.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[trivial PATCH] treewide: Convert switch/case fallthrough; to break;
fallthrough to a separate case/default label break; isn't very readable.
Convert pseudo-keyword fallthrough; statements to a simple break; when
the next label is case or default and the only statement in the next
label block is break;
Found using:
$ grep-2.5.4 -rP --include=*.[ch] -n
"fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
Miscellanea:
o Move or coalesce a couple label blocks above a default: block.
Signed-off-by: Joe Perches
---
Compiled allyesconfig x86-64 only.
A few files for other arches were not compiled.
arch/arm/mach-mmp/pm-pxa910.c | 2 +-
arch/arm64/kvm/handle_exit.c | 2 +-
arch/mips/kernel/cpu-probe.c | 2 +-
arch/mips/math-emu/cp1emu.c | 2 +-
arch/s390/pci/pci.c | 2 +-
crypto/tcrypt.c | 4 ++--
drivers/ata/sata_mv.c | 2 +-
drivers/atm/lanai.c | 2 +-
drivers/gpu/drm/i915/display/intel_sprite.c | 2 +-
drivers/gpu/drm/nouveau/nvkm/engine/disp/hdmi.c | 2 +-
drivers/hid/wacom_wac.c | 2 +-
drivers/i2c/busses/i2c-i801.c | 2 +-
drivers/infiniband/ulp/rtrs/rtrs-clt.c| 14 +++---
drivers/infiniband/ulp/rtrs/rtrs-srv.c| 6 +++---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +-
drivers/irqchip/irq-vic.c | 4 ++--
drivers/md/dm.c | 2 +-
drivers/media/dvb-frontends/drxd_hard.c | 2 +-
drivers/media/i2c/ov5640.c| 2 +-
drivers/media/i2c/ov6650.c| 5 ++---
drivers/media/i2c/smiapp/smiapp-core.c| 2 +-
drivers/media/i2c/tvp5150.c | 2 +-
drivers/media/pci/ddbridge/ddbridge-core.c| 2 +-
drivers/media/usb/cpia2/cpia2_core.c | 2 +-
drivers/mfd/iqs62x.c | 3 +--
drivers/mmc/host/atmel-mci.c | 2 +-
drivers/mtd/nand/raw/nandsim.c| 2 +-
drivers/net/ethernet/intel/e1000e/phy.c | 2 +-
drivers/net/ethernet/intel/fm10k/fm10k_pf.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_adminq.c | 2 +-
drivers/net/ethernet/intel/i40e/i40e_txrx.c | 2 +-
drivers/net/ethernet/intel/iavf/iavf_txrx.c | 2 +-
drivers/net/ethernet/intel/igb/e1000_phy.c| 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c| 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c| 2 +-
drivers/net/ethernet/intel/ixgbevf/vf.c | 2 +-
drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c | 2 +-
drivers/net/ethernet/qlogic/qed/qed_mcp.c | 2 +-
drivers/net/ethernet/sfc/falcon/farch.c | 2 +-
drivers/net/ethernet/sfc/farch.c | 2 +-
drivers/net/phy/adin.c| 3 +--
drivers/net/usb/pegasus.c | 4 ++--
drivers/net/usb/usbnet.c | 2 +-
drivers/net/wireless/ath/ath5k/eeprom.c | 2 +-
drivers/net/wireless/mediatek/mt7601u/dma.c | 8
drivers/nvme/host/core.c | 12 ++--
drivers/pcmcia/db1xxx_ss.c| 4 ++--
drivers/power/supply/abx500_chargalg.c| 2 +-
drivers/power/supply/charger-manager.c| 2 +-
drivers/rtc/rtc-pcf85063.c| 2 +-
drivers/s390/scsi/zfcp_fsf.c | 2 +-
drivers/scsi/aic7xxx/aic79xx_core.c | 4 ++--
drivers/scsi/aic94xx/aic94xx_tmf.c| 2 +-
drivers/scsi/lpfc/lpfc_sli.c | 2 +-
drivers/scsi/smartpqi/smartpqi_init.c | 2 +-
drivers/scsi/sr.c | 2 +-
drivers/tty/serial/sunsu.c| 2 +-
drivers/tty/serial/sunzilog.c | 2 +-
drivers/tty/vt/vt_ioctl.c | 2 +-
drivers/usb/dwc3/core.c | 2 +-
drivers/usb/gadget/legacy/inode.c | 2 +-
drivers/usb/gadget/udc/pxa25x_udc.c | 4 ++--
drivers/usb/host/ohci-hcd.c | 2 +-
drivers/usb/isp1760/isp1760-hcd.c | 2 +-
drivers/usb/musb/
Re: [PATCH 08/29] dma-buf: Avoid comma separated statements
On Fri, 2020-09-04 at 12:42 +0100, Kieran Bingham wrote: > I'm pleased to see this is treewide ;-) > Definitely prefer this. These are only the comma uses in if/do/while blocks that would also need braces as separate statements. There a multiple dozens more just with single statements terminated by commas and not semicolons. See: https://lore.kernel.org/patchwork/patch/1291033/ ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
