Re: truncation in drivers/video/fbdev/neofb.c
On Thu, Aug 31, 2023 at 3:16 PM Helge Deller wrote:
>
> * Nick Desaulniers :
> > On Thu, Aug 31, 2023 at 2:23 PM Helge Deller wrote:
> > >
> > > * Helge Deller :
> > > > On 8/29/23 18:45, Nick Desaulniers wrote:
> > > > > A recent change in clang made it better about spotting snprintf that
> > > > > will result in truncation. Nathan reported the following instances:
> > > > >
> > > > > drivers/video/fbdev/neofb.c:1959:3: warning: 'snprintf' will always be
> > > > > truncated; specified size is 16, but format string expands to at least
> > > > > 17 [-Wfortify-source]
> > >
> > > FYI, I've added the patch below to the fbdev for-next git tree.
> > > [...]
> >
> > This indeed makes the warning go away, but that's more so due to the
> > use of strscpy now rather than snprintf. That alone is a good change
> > but we still have definite truncation. See below:
> > [...]
>
> Nick, thanks for your review and findings!
> Now every string should be max. 15 chars (which fits with the trailing
> NUL into the char[16] array).
>
> Helge
>
>
> Subject: [PATCH] fbdev: neofb: Shorten Neomagic product name in info struct
>
> Avoid those compiler warnings:
> neofb.c:1959:3: warning: 'snprintf' will always be truncated;
>specified size is 16, but format string expands to at least 17
> [-Wfortify-source]
>
> Signed-off-by: Helge Deller
> Reported-by: Nathan Chancellor
> Reported-by: Nick Desaulniers
> Link:
> https://lore.kernel.org/all/cakwvodn0xovwjq6ufm_rojtkb0f1i1hw-j_xygfkdnfdhwa...@mail.gmail.com/
> Link: https://github.com/ClangBuiltLinux/linux/issues/1923
ah yeah LGTM
Reviewed-by: Nick Desaulniers
>
> diff --git a/drivers/video/fbdev/neofb.c b/drivers/video/fbdev/neofb.c
> index d2f622b4c372..b58b11015c0c 100644
> --- a/drivers/video/fbdev/neofb.c
> +++ b/drivers/video/fbdev/neofb.c
> @@ -1948,49 +1948,40 @@ static struct fb_info *neo_alloc_fb_info(struct
> pci_dev *dev,
>
> switch (info->fix.accel) {
> case FB_ACCEL_NEOMAGIC_NM2070:
> - snprintf(info->fix.id, sizeof(info->fix.id),
> - "MagicGraph 128");
> + strscpy(info->fix.id, "MagicGraph128", sizeof(info->fix.id));
> break;
> case FB_ACCEL_NEOMAGIC_NM2090:
> - snprintf(info->fix.id, sizeof(info->fix.id),
> - "MagicGraph 128V");
> + strscpy(info->fix.id, "MagicGraph128V", sizeof(info->fix.id));
> break;
> case FB_ACCEL_NEOMAGIC_NM2093:
> - snprintf(info->fix.id, sizeof(info->fix.id),
> - "MagicGraph 128ZV");
> + strscpy(info->fix.id, "MagicGraph128ZV",
> sizeof(info->fix.id));
> break;
> case FB_ACCEL_NEOMAGIC_NM2097:
> - snprintf(info->fix.id, sizeof(info->fix.id),
> - "MagicGraph 128ZV+");
> + strscpy(info->fix.id, "Mag.Graph128ZV+",
> sizeof(info->fix.id));
> break;
> case FB_ACCEL_NEOMAGIC_NM2160:
> - snprintf(info->fix.id, sizeof(info->fix.id),
> - "MagicGraph 128XD");
> + strscpy(info->fix.id, "MagicGraph128XD",
> sizeof(info->fix.id));
> break;
> case FB_ACCEL_NEOMAGIC_NM2200:
> - snprintf(info->fix.id, sizeof(info->fix.id),
> - "MagicGraph 256AV");
> + strscpy(info->fix.id, "MagicGraph256AV",
> sizeof(info->fix.id));
> info->flags |= FBINFO_HWACCEL_IMAGEBLIT |
>FBINFO_HWACCEL_COPYAREA |
>FBINFO_HWACCEL_FILLRECT;
> break;
> case FB_ACCEL_NEOMAGIC_NM2230:
> - snprintf(info->fix.id, sizeof(info->fix.id),
> - "MagicGraph 256AV+");
> + strscpy(info->fix.id, "Mag.Graph256AV+",
> sizeof(info->fix.id));
> info->flags |= FBINFO_HWACCEL_IMAGEBLIT |
>FBINFO_HWACCEL_COPYAREA |
>FBINFO_HWACCEL_FILLRECT;
> break;
> case FB_ACCEL_NEOMAGIC_NM2360:
> - snprintf(info->fix.id, sizeof(info->fix.id),
> - &quo
Re: truncation in drivers/video/fbdev/neofb.c
On Thu, Aug 31, 2023 at 2:23 PM Helge Deller wrote:
>
> * Helge Deller :
> > On 8/29/23 18:45, Nick Desaulniers wrote:
> > > A recent change in clang made it better about spotting snprintf that
> > > will result in truncation. Nathan reported the following instances:
> > >
> > > drivers/video/fbdev/neofb.c:1959:3: warning: 'snprintf' will always be
> > > truncated; specified size is 16, but format string expands to at least
> > > 17 [-Wfortify-source]
>
> FYI, I've added the patch below to the fbdev for-next git tree.
>
> Helge
>
> From: Helge Deller
> Subject: [PATCH] fbdev: neofb: Shorten Neomagic product name in info struct
>
> Avoid those compiler warnings:
> neofb.c:1959:3: warning: 'snprintf' will always be truncated;
>specified size is 16, but format string expands to at least 17
> [-Wfortify-source]
>
> Signed-off-by: Helge Deller
> Reported-by: Nathan Chancellor
> Reported-by: Nick Desaulniers
> Link:
> https://lore.kernel.org/all/cakwvodn0xovwjq6ufm_rojtkb0f1i1hw-j_xygfkdnfdhwa...@mail.gmail.com/
> Link: https://github.com/ClangBuiltLinux/linux/issues/1923
This indeed makes the warning go away, but that's more so due to the
use of strscpy now rather than snprintf. That alone is a good change
but we still have definite truncation. See below:
>
> diff --git a/drivers/video/fbdev/neofb.c b/drivers/video/fbdev/neofb.c
> index d2f622b4c372..b905fe93b525 100644
> --- a/drivers/video/fbdev/neofb.c
> +++ b/drivers/video/fbdev/neofb.c
> @@ -1948,49 +1948,40 @@ static struct fb_info *neo_alloc_fb_info(struct
> pci_dev *dev,
>
> switch (info->fix.accel) {
> case FB_ACCEL_NEOMAGIC_NM2070:
> - snprintf(info->fix.id, sizeof(info->fix.id),
> - "MagicGraph 128");
> + strscpy(info->fix.id, "MagicGraph128", sizeof(info->fix.id));
> break;
> case FB_ACCEL_NEOMAGIC_NM2090:
> - snprintf(info->fix.id, sizeof(info->fix.id),
> - "MagicGraph 128V");
> + strscpy(info->fix.id, "MagicGraph128V", sizeof(info->fix.id));
> break;
> case FB_ACCEL_NEOMAGIC_NM2093:
> - snprintf(info->fix.id, sizeof(info->fix.id),
> - "MagicGraph 128ZV");
> + strscpy(info->fix.id, "MagicGraph128ZV",
> sizeof(info->fix.id));
> break;
> case FB_ACCEL_NEOMAGIC_NM2097:
> - snprintf(info->fix.id, sizeof(info->fix.id),
> - "MagicGraph 128ZV+");
> + strscpy(info->fix.id, "Mag.Graph128ZV+",
> sizeof(info->fix.id));
I see the Mag.Graph change here; I'm curious why MagicGraph256XL+
didn't need that, too?
MagicGraph128ZV+
MagicGraph256XL+
those look like the same number of characters; are they? Does this one
need the `.` change?
```
diff --git a/drivers/video/fbdev/neofb.c b/drivers/video/fbdev/neofb.c
index b905fe93b525..6b7836f7f809 100644
--- a/drivers/video/fbdev/neofb.c
+++ b/drivers/video/fbdev/neofb.c
@@ -1957,7 +1957,7 @@ static struct fb_info *neo_alloc_fb_info(struct
pci_dev *dev,
strscpy(info->fix.id, "MagicGraph128ZV", sizeof(info->fix.id));
break;
case FB_ACCEL_NEOMAGIC_NM2097:
- strscpy(info->fix.id, "Mag.Graph128ZV+", sizeof(info->fix.id));
+ strscpy(info->fix.id, "MagicGraph128ZV+", sizeof(info->fix.id));
break;
case FB_ACCEL_NEOMAGIC_NM2160:
strscpy(info->fix.id, "MagicGraph128XD", sizeof(info->fix.id));
```
Builds without error for me. Though I guess the compiler doesn't know
what strscpy is semantically.
Ah
MagicGraph128ZV+
MagicGraph256XL+
are both 17 characters. (16 literal characters + trailing NUL)
So MagicGraph256XL+ (and MagicGraph256AV+) below will fail to copy the
NUL from the source, then overwrite the `+` with NUL IIUC.
There's no ambiguity for MagicGraph256XL+, but now MagicGraph256AV+
would look like MagicGraph256AV which is another possible variant.
Then perhaps MagicGraph256XL+ MagicGraph256AV+ and below might need to change.
Sorry, initially it looked like a simple fix and I was ready to sign
off on it; I hope I'm mistaken. Thank you for the patch, but do
consider sending it to the ML for review before committing.
> break;
> case FB_ACCEL_NEOMAGIC_NM2160:
> - snprintf(info->fix.id, sizeof(info->fix.id),
> - "MagicGraph 128XD");
> + strscpy(info-
truncation in drivers/video/fbdev/neofb.c
Helge,
A recent change in clang made it better about spotting snprintf that
will result in truncation. Nathan reported the following instances:
drivers/video/fbdev/neofb.c:1959:3: warning: 'snprintf' will always be
truncated; specified size is 16, but format string expands to at least
17 [-Wfortify-source]
drivers/video/fbdev/neofb.c:1963:3: warning: 'snprintf' will always be
truncated; specified size is 16, but format string expands to at least
18 [-Wfortify-source]
drivers/video/fbdev/neofb.c:1967:3: warning: 'snprintf' will always be
truncated; specified size is 16, but format string expands to at least
17 [-Wfortify-source]
drivers/video/fbdev/neofb.c:1971:3: warning: 'snprintf' will always be
truncated; specified size is 16, but format string expands to at least
17 [-Wfortify-source]
drivers/video/fbdev/neofb.c:1978:3: warning: 'snprintf' will always be
truncated; specified size is 16, but format string expands to at least
18 [-Wfortify-source]
drivers/video/fbdev/neofb.c:1985:3: warning: 'snprintf' will always be
truncated; specified size is 16, but format string expands to at least
17 [-Wfortify-source]
drivers/video/fbdev/neofb.c:1992:3: warning: 'snprintf' will always be
truncated; specified size is 16, but format string expands to at least
18 [-Wfortify-source]
https://github.com/ClangBuiltLinux/linux/issues/1923
Clang is right here. `info->fix.id` is declared as `char id[16];` so
indeed string literals like "MagicGraph 256AV+" indeed lead to
truncation. But this is declared in include/uapi/linux/fb.h; I assume
those headers cant be changed? Can the strings be shortened then? Is
it perhaps time to delete this driver?
I see AKPM mentioned alluded to this in
commit 0e90454 ("neofb: avoid overwriting fb_info fields")
(Also, snprintf probably isn't necessary for string literals that
don't contain format strings)
--
Thanks,
~Nick Desaulniers
Re: [PATCH 1/2] drm/exec: use unique instead of local label
On Wed, Aug 2, 2023 at 1:44 AM Boris Brezillon
wrote:
>
> On Tue, 1 Aug 2023 13:35:13 -0700
> Nick Desaulniers wrote:
>
> > On Mon, Jul 31, 2023 at 5:36 AM Christian König
> > wrote:
> > >
> > > GCC forbids to jump to labels in loop conditions and a new clang
> > > check stumbled over this.
> > >
> > > So instead using a local label inside the loop condition use an
> > > unique label outside of it.
> > >
> > > Fixes: commit 09593216bff1 ("drm: execution context for GEM buffers v7")
> > > Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/1890
> > > Link:
> > > https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067
> > > Reported-by: Nathan Chancellor
> > > Reported-by: Naresh Kamboju
> > > CC: Boris Brezillon
> > > Signed-off-by: Christian König
> >
> > Works for me; thanks for the patch!
> > Reviewed-by: Nick Desaulniers
> >
> > I suspect it's possible to change the indirect goto into a direct goto
> > with some further refactoring (macros can take block statements; if
> > drm_exec_until_all_locked accepted a block statement arg then you
> > could introduce a new scope, and a new local label to that scope, then
> > just use direct goto),
>
> Maybe I'm wrong, but this sounds like the version I proposed here [1].
Nearly; here's what I was imagining:
```
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 977e1804718d..3ea8beb159f0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -904,7 +904,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
e->user_invalidated = userpage_invalidated;
}
- drm_exec_until_all_locked(>exec) {
+ drm_exec_until_all_locked(>exec, {
r = amdgpu_vm_lock_pd(>vm, >exec, 1 + p->gang_size);
drm_exec_retry_on_contention(>exec);
if (unlikely(r))
@@ -928,7 +928,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
if (unlikely(r))
goto out_free_user_pages;
}
- }
+ })
amdgpu_bo_list_for_each_userptr_entry(e, p->bo_list) {
struct mm_struct *usermm;
diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
index 73205afec162..8e32a9b704e7 100644
--- a/include/drm/drm_exec.h
+++ b/include/drm/drm_exec.h
@@ -74,14 +74,13 @@ struct drm_exec {
* Since labels can't be defined local to the loops body we use a jump pointer
* to make sure that the retry is only used from within the loops body.
*/
-#define drm_exec_until_all_locked(exec)\
- for (void *__drm_exec_retry_ptr; ({ \
- __label__ __drm_exec_retry; \
-__drm_exec_retry: \
- __drm_exec_retry_ptr = &&__drm_exec_retry; \
- (void)__drm_exec_retry_ptr; \
- drm_exec_cleanup(exec); \
- });)
+#define drm_exec_until_all_locked(exec, block) \
+ { \
+ __label__ __drm_exec_retry; \
+__drm_exec_retry: \
+ while (drm_exec_cleanup(exec)) \
+ block \
+}
/**
* drm_exec_retry_on_contention - restart the loop to grap all locks
@@ -93,7 +92,7 @@ __drm_exec_retry:
\
#define drm_exec_retry_on_contention(exec) \
do {\
if (unlikely(drm_exec_is_contended(exec))) \
- goto *__drm_exec_retry_ptr; \
+ goto __drm_exec_retry; \
} while (0)
/**
```
(only updated one macro expansion site in
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c, didn't add proper trailing
tabs to macro but you get the gist).
But I think both compilers can optimize out the unnecessary
indirection when it's obvious, so I don't think it matters much, other
than the tastes of whoever has to maintain this.
>
> > but this will probably apply cleaner. (oh, is
> > 09593216bff1 only in next at the moment? The AuthorDate threw me.)
> >
> > There are some curious cases where __attribute__((cleanup())) doesn't
> > mesh well with indirect gotos.
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=37722
> >
> > May not ever be a problem here...
>
> [1]https://patchwork.freedesktop.org/patch/543077/
--
Thanks,
~Nick Desaulniers
Re: [PATCH 1/2] drm/exec: use unique instead of local label
On Mon, Jul 31, 2023 at 5:36 AM Christian König
wrote:
>
> GCC forbids to jump to labels in loop conditions and a new clang
> check stumbled over this.
>
> So instead using a local label inside the loop condition use an
> unique label outside of it.
>
> Fixes: commit 09593216bff1 ("drm: execution context for GEM buffers v7")
> Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
> Link: https://github.com/ClangBuiltLinux/linux/issues/1890
> Link:
> https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067
> Reported-by: Nathan Chancellor
> Reported-by: Naresh Kamboju
> CC: Boris Brezillon
> Signed-off-by: Christian König
Works for me; thanks for the patch!
Reviewed-by: Nick Desaulniers
I suspect it's possible to change the indirect goto into a direct goto
with some further refactoring (macros can take block statements; if
drm_exec_until_all_locked accepted a block statement arg then you
could introduce a new scope, and a new local label to that scope, then
just use direct goto), but this will probably apply cleaner. (oh, is
09593216bff1 only in next at the moment? The AuthorDate threw me.)
There are some curious cases where __attribute__((cleanup())) doesn't
mesh well with indirect gotos.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=37722
May not ever be a problem here...
> ---
> include/drm/drm_exec.h | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index 73205afec162..e0462361adf9 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -3,6 +3,7 @@
> #ifndef __DRM_EXEC_H__
> #define __DRM_EXEC_H__
>
> +#include
If you wanted to be more specific (if this addition is due to
__PASTE), then `compiler_types.h` is more precise.
> #include
>
> #define DRM_EXEC_INTERRUPTIBLE_WAITBIT(0)
> @@ -74,13 +75,12 @@ struct drm_exec {
> * Since labels can't be defined local to the loops body we use a jump
> pointer
> * to make sure that the retry is only used from within the loops body.
> */
> -#define drm_exec_until_all_locked(exec)\
> - for (void *__drm_exec_retry_ptr; ({ \
> - __label__ __drm_exec_retry; \
> -__drm_exec_retry: \
> - __drm_exec_retry_ptr = &&__drm_exec_retry; \
> - (void)__drm_exec_retry_ptr; \
> - drm_exec_cleanup(exec); \
> +#define drm_exec_until_all_locked(exec)
> \
> +__PASTE(__drm_exec_, __LINE__):
> \
> + for (void *__drm_exec_retry_ptr; ({ \
> + __drm_exec_retry_ptr = &&__PASTE(__drm_exec_, __LINE__);\
> + (void)__drm_exec_retry_ptr; \
> + drm_exec_cleanup(exec); \
> });)
>
> /**
> --
> 2.34.1
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm: fix indirect goto into statement expression UB
On Thu, Jul 27, 2023 at 3:47 PM wrote:
>
> A new diagnostic in clang-17 now produces the following build error:
>
> drivers/gpu/drm/tests/drm_exec_test.c:41:3: error: cannot jump from this
> indirect goto statement to one of its possible targets
>41 | drm_exec_retry_on_contention();
> | ^
> include/drm/drm_exec.h:96:4: note: expanded from macro
> 'drm_exec_retry_on_contention'
>96 | goto *__drm_exec_retry_ptr;
> | ^
> drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: possible target of
> indirect goto statement
>39 | drm_exec_until_all_locked() {
> | ^
> include/drm/drm_exec.h:79:33: note: expanded from macro
> 'drm_exec_until_all_locked'
>79 | __label__ __drm_exec_retry;
> drivers/gpu/drm/tests/drm_exec_test.c:39:2: note: jump enters a
> statement expression
>
> The GCC manually currently states that:
> >> Jumping into a statement expression with a computed goto (see Labels
> >> as Values) has undefined behavior.
>
> So the diagnostic appears correct, even if codegen happened to produce
> working code.
>
> Looking closer at this code, while the original combination of statement
> expression, local label, and computed/indirect goto GNU C expressions
> were clever, a simple while loop and continue block might have sufficed.
>
> This approach might not work as expected if drm_exec_until_all_locked
> "loops" can be nested, but that doesn't appear to be an existing use
> case in the codebase.
>
> Fixes: commit 09593216bff1 ("drm: execution context for GEM buffers v7")
> Link: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
> Link: https://github.com/ClangBuiltLinux/linux/issues/1890
> Link:
> https://github.com/llvm/llvm-project/commit/20219106060208f0c2f5d096eb3aed7b712f5067
> Reported-by: Nathan Chancellor
> Reported-by: Naresh Kamboju
> Signed-off-by: Nick Desaulniers
> ---
> include/drm/drm_exec.h | 13 ++---
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h
> index 73205afec162..393ac022ed3a 100644
> --- a/include/drm/drm_exec.h
> +++ b/include/drm/drm_exec.h
> @@ -70,18 +70,9 @@ struct drm_exec {
> * Core functionality of the drm_exec object. Loops until all GEM objects are
> * locked and no more contention exists. At the beginning of the loop it is
> * guaranteed that no GEM object is locked.
> - *
> - * Since labels can't be defined local to the loops body we use a jump
> pointer
> - * to make sure that the retry is only used from within the loops body.
> */
> #define drm_exec_until_all_locked(exec)\
> - for (void *__drm_exec_retry_ptr; ({ \
> - __label__ __drm_exec_retry; \
> -__drm_exec_retry: \
> - __drm_exec_retry_ptr = &&__drm_exec_retry; \
> - (void)__drm_exec_retry_ptr; \
> - drm_exec_cleanup(exec); \
> - });)
> + while(drm_exec_cleanup(exec))
>
> /**
> * drm_exec_retry_on_contention - restart the loop to grap all locks
> @@ -93,7 +84,7 @@ __drm_exec_retry:
> \
> #define drm_exec_retry_on_contention(exec) \
> do {\
> if (unlikely(drm_exec_is_contended(exec))) \
> - goto *__drm_exec_retry_ptr; \
> + continue; \
d'oh that's going to continue the do {} while(0) ... let me send a v2...
> } while (0)
>
> /**
>
> ---
> base-commit: 451cc82bd11eb6a374f4dbcfc1cf007eafea91ab
> change-id: 20230727-amdgpu-93c0e5302951
>
> Best regards,
> --
> Nick Desaulniers
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH 1/2] drm/v3d: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()
On Tue, Jul 18, 2023 at 2:44 PM Nathan Chancellor wrote:
>
> A proposed update to clang's -Wconstant-logical-operand to warn when the
> left hand side is a constant shows the following instance in
> nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ,
> such as CONFIG_HZ=300:
>
> In file included from drivers/gpu/drm/v3d/v3d_debugfs.c:12:
> drivers/gpu/drm/v3d/v3d_drv.h:343:24: warning: use of logical '&&' with
> constant operand [-Wconstant-logical-operand]
> 343 | if (NSEC_PER_SEC % HZ &&
> | ~ ^
> drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: use '&' for a bitwise operation
> 343 | if (NSEC_PER_SEC % HZ &&
> | ^~
> | &
> drivers/gpu/drm/v3d/v3d_drv.h:343:24: note: remove constant to silence this
> warning
> 1 warning generated.
>
> Turn this into an explicit comparison against zero to make the
> expression a boolean to make it clear this should be a logical check,
> not a bitwise one.
>
> Link: https://reviews.llvm.org/D142609
> Signed-off-by: Nathan Chancellor
Thanks for the patch!
Reviewed-by: Nick Desaulniers
> ---
> drivers/gpu/drm/v3d/v3d_drv.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
> index b74b1351bfc8..7f664a4b2a75 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.h
> +++ b/drivers/gpu/drm/v3d/v3d_drv.h
> @@ -340,7 +340,7 @@ struct v3d_submit_ext {
> static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
> {
> /* nsecs_to_jiffies64() does not guard against overflow */
> - if (NSEC_PER_SEC % HZ &&
> + if ((NSEC_PER_SEC % HZ) != 0 &&
> div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
> return MAX_JIFFY_OFFSET;
>
>
> --
> 2.41.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/i915: Fix clang -Wimplicit-fallthrough in intel_async_flip_check_hw()
On Thu, May 25, 2023 at 1:30 PM Matthieu Baerts
wrote:
>
> Hi Nick,
>
> On 24/05/2023 20:56, Nick Desaulniers wrote:
> > On Wed, May 24, 2023 at 11:41 AM Nathan Chancellor
> > wrote:
> >>
> >> On Wed, May 24, 2023 at 11:32:32AM -0700, Nick Desaulniers wrote:
> >>> On Wed, May 24, 2023 at 8:38 AM Nathan Chancellor
> >>> wrote:
> >>>>
> >>>> Clang warns:
> >>>>
> >>>> drivers/gpu/drm/i915/display/intel_display.c:6012:3: error:
> >>>> unannotated fall-through between switch labels
> >>>> [-Werror,-Wimplicit-fallthrough]
> >>>> case I915_FORMAT_MOD_X_TILED:
> >>>> ^
> >>>> drivers/gpu/drm/i915/display/intel_display.c:6012:3: note: insert
> >>>> 'break;' to avoid fall-through
> >>>> case I915_FORMAT_MOD_X_TILED:
> >>>> ^
> >>>> break;
> >>>> 1 error generated.
> >>>>
> >>>> Clang is a little more pedantic than GCC, which does not warn when
> >>>> falling through to a case that is just break or return. Clang's version
> >>>> is more in line with the kernel's own stance in deprecated.rst, which
> >>>> states that all switch/case blocks must end in either break,
> >>>> fallthrough, continue, goto, or return. Add the missing break to silence
> >>>> the warning.
> >>>>
> >>>> Fixes: 937859485aef ("drm/i915: Support Async Flip on Linear buffers")
> >>>> Reported-by: kernel test robot
> >>>> Closes: https://lore.kernel.org/202305241902.uvhtmoxa-...@intel.com/
> >>>> Reported-by: Naresh Kamboju
> >>>> Closes:
> >>>> https://lore.kernel.org/CA+G9fYv68V3ewK0Qj-syQj7qX-hQr0H1MFL=qfnudoe_j2z...@mail.gmail.com/
> >>>> Signed-off-by: Nathan Chancellor
> >>>
> >>> Thanks for the patch! I've never seen the closes tag before, that's
> >>> new to me. Can you tell me more about it?
> >>
> >> It is new to me (at least in the context of the kernel) as well. I only
> >> used it over Link: because checkpatch.pl told me to:
> >>
> >> WARNING: Reported-by: should be immediately followed by Closes: with a URL
> >> to the report
> >> #26:
> >> Reported-by: kernel test robot
> >> Reported-by: Naresh Kamboju
> >>
> >> WARNING: Reported-by: should be immediately followed by Closes: with a URL
> >> to the report
> >> #27:
> >> Reported-by: Naresh Kamboju
> >> Signed-off-by: Nathan Chancellor
> >>
> >> It was Link: for a bit but commit 44c31888098a ("checkpatch: allow
> >> Closes tags with links") changed it to Closes:. Looks odd to me but
> >> whatever the linter says I suppose.
> >>
> >> Thanks for the review!
> >>
> >> Cheers,
> >> Nathan
> >>
> >>> A few more tags
> >>>
> >>> Reported-by: Tom Rix
> >>> Link:
> >>> https://lore.kernel.org/all/20230523125116.1669057-1-t...@redhat.com/
> >>> Reviewed-by: Nick Desaulniers
> >
> > Ah then I guess my link tag should have been
> >
> > Closes:
> > https://lore.kernel.org/all/20230523125116.1669057-1-t...@redhat.com/
> >
> > I hope the author of
> > commit 44c31888098a ("checkpatch: allow Closes tags with links")
> > has coordinated with the maintainer of b4, so that b4 recognizes Closes
> > tags.
> > b4 v0.12.2 does not pick up Closes tags.
>
> I'm sorry for the troubles caused by this series, that was not the
> intension.
>
> When looking at modifying b4 to support the Closes tag, I realised the
> Link tag from your previous message [1] was not taken as well. Was it
> just me?
oh? good find!
>
> If no, I just sent patches for b4, see [2]. I hope it will help!
appreciated! Thank you.
>
> Cheers,
> Matt
>
> [1]
> https://lore.kernel.org/all/CAKwvOd=jzjouunmd3rvc--goa0exphcf6chxua6w1kxjg2a...@mail.gmail.com/
> [2]
> https://lore.kernel.org/tools/20230525-closes-tags-v1-0-ed41b1773...@tessares.net/T/
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/i915: Fix clang -Wimplicit-fallthrough in intel_async_flip_check_hw()
On Wed, May 24, 2023 at 11:41 AM Nathan Chancellor wrote:
>
> On Wed, May 24, 2023 at 11:32:32AM -0700, Nick Desaulniers wrote:
> > On Wed, May 24, 2023 at 8:38 AM Nathan Chancellor wrote:
> > >
> > > Clang warns:
> > >
> > > drivers/gpu/drm/i915/display/intel_display.c:6012:3: error: unannotated
> > > fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
> > > case I915_FORMAT_MOD_X_TILED:
> > > ^
> > > drivers/gpu/drm/i915/display/intel_display.c:6012:3: note: insert
> > > 'break;' to avoid fall-through
> > > case I915_FORMAT_MOD_X_TILED:
> > > ^
> > > break;
> > > 1 error generated.
> > >
> > > Clang is a little more pedantic than GCC, which does not warn when
> > > falling through to a case that is just break or return. Clang's version
> > > is more in line with the kernel's own stance in deprecated.rst, which
> > > states that all switch/case blocks must end in either break,
> > > fallthrough, continue, goto, or return. Add the missing break to silence
> > > the warning.
> > >
> > > Fixes: 937859485aef ("drm/i915: Support Async Flip on Linear buffers")
> > > Reported-by: kernel test robot
> > > Closes: https://lore.kernel.org/202305241902.uvhtmoxa-...@intel.com/
> > > Reported-by: Naresh Kamboju
> > > Closes:
> > > https://lore.kernel.org/CA+G9fYv68V3ewK0Qj-syQj7qX-hQr0H1MFL=qfnudoe_j2z...@mail.gmail.com/
> > > Signed-off-by: Nathan Chancellor
> >
> > Thanks for the patch! I've never seen the closes tag before, that's
> > new to me. Can you tell me more about it?
>
> It is new to me (at least in the context of the kernel) as well. I only
> used it over Link: because checkpatch.pl told me to:
>
> WARNING: Reported-by: should be immediately followed by Closes: with a URL to
> the report
> #26:
> Reported-by: kernel test robot
> Reported-by: Naresh Kamboju
>
> WARNING: Reported-by: should be immediately followed by Closes: with a URL to
> the report
> #27:
> Reported-by: Naresh Kamboju
> Signed-off-by: Nathan Chancellor
>
> It was Link: for a bit but commit 44c31888098a ("checkpatch: allow
> Closes tags with links") changed it to Closes:. Looks odd to me but
> whatever the linter says I suppose.
>
> Thanks for the review!
>
> Cheers,
> Nathan
>
> > A few more tags
> >
> > Reported-by: Tom Rix
> > Link: https://lore.kernel.org/all/20230523125116.1669057-1-t...@redhat.com/
> > Reviewed-by: Nick Desaulniers
Ah then I guess my link tag should have been
Closes: https://lore.kernel.org/all/20230523125116.1669057-1-t...@redhat.com/
I hope the author of
commit 44c31888098a ("checkpatch: allow Closes tags with links")
has coordinated with the maintainer of b4, so that b4 recognizes Closes tags.
b4 v0.12.2 does not pick up Closes tags.
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/i915: Fix clang -Wimplicit-fallthrough in intel_async_flip_check_hw()
On Wed, May 24, 2023 at 8:38 AM Nathan Chancellor wrote:
>
> Clang warns:
>
> drivers/gpu/drm/i915/display/intel_display.c:6012:3: error: unannotated
> fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
> case I915_FORMAT_MOD_X_TILED:
> ^
> drivers/gpu/drm/i915/display/intel_display.c:6012:3: note: insert 'break;'
> to avoid fall-through
> case I915_FORMAT_MOD_X_TILED:
> ^
> break;
> 1 error generated.
>
> Clang is a little more pedantic than GCC, which does not warn when
> falling through to a case that is just break or return. Clang's version
> is more in line with the kernel's own stance in deprecated.rst, which
> states that all switch/case blocks must end in either break,
> fallthrough, continue, goto, or return. Add the missing break to silence
> the warning.
>
> Fixes: 937859485aef ("drm/i915: Support Async Flip on Linear buffers")
> Reported-by: kernel test robot
> Closes: https://lore.kernel.org/202305241902.uvhtmoxa-...@intel.com/
> Reported-by: Naresh Kamboju
> Closes:
> https://lore.kernel.org/CA+G9fYv68V3ewK0Qj-syQj7qX-hQr0H1MFL=qfnudoe_j2z...@mail.gmail.com/
> Signed-off-by: Nathan Chancellor
Thanks for the patch! I've never seen the closes tag before, that's
new to me. Can you tell me more about it?
A few more tags
Reported-by: Tom Rix
Link: https://lore.kernel.org/all/20230523125116.1669057-1-t...@redhat.com/
Reviewed-by: Nick Desaulniers
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 0490c6412ab5..6d49e0ab3e85 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -6008,6 +6008,7 @@ static int intel_async_flip_check_hw(struct
> intel_atomic_state *state, struct in
> plane->base.base.id,
> plane->base.name);
> return -EINVAL;
> }
> + break;
>
> case I915_FORMAT_MOD_X_TILED:
> case I915_FORMAT_MOD_Y_TILED:
>
> ---
> base-commit: 9a2cb1b31c040e2f1b313e2f7921f0f5e6b66d82
> change-id:
> 20230524-intel_async_flip_check_hw-implicit-fallthrough-c4c40b03802f
>
> Best regards,
> --
> Nathan Chancellor
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/panel: samsung-s6d7aa0: use pointer for drm_mode in panel desc struct
On Tue, May 23, 2023 at 2:29 PM Nick Desaulniers
wrote:
>
> On Tue, May 23, 2023 at 2:20 PM Artur Weber wrote:
> >
> > Fixes compilation errors on older GCC versions (before 8.x) and Clang
> > after changes introduced in commit 6810bb390282 ("drm/panel: Add
> > Samsung S6D7AA0 panel controller driver"). Tested with GCC 13.1.1,
> > GCC 6.4.0 and Clang 16.0.3.
>
> Hi Artur,
> Thanks for the patch! Mind sending a v2 with the diagnostic added to
> the commit message? This gives reviewers much more context as to what
> issue you are fixing.
Perhaps it was these errors:
https://lore.kernel.org/llvm/646c6def.a70a0220.58c1a.9...@mx.google.com/T/#u
?
If so, please add
Reported-by: kernelci.org bot
Link: https://lore.kernel.org/llvm/646c6def.a70a0220.58c1a.9...@mx.google.com
To the v2 commit message as well.
>
> >
> > Signed-off-by: Artur Weber
> > ---
> > drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c | 10 +-
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
> > b/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
> > index f532aa018428..102e1fc7ee38 100644
> > --- a/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
> > +++ b/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
> > @@ -37,7 +37,7 @@ struct s6d7aa0_panel_desc {
> > unsigned int panel_type;
> > int (*init_func)(struct s6d7aa0 *ctx);
> > int (*off_func)(struct s6d7aa0 *ctx);
> > - const struct drm_display_mode drm_mode;
> > + const struct drm_display_mode *drm_mode;
> > unsigned long mode_flags;
> > u32 bus_flags;
> > bool has_backlight;
> > @@ -309,7 +309,7 @@ static const struct s6d7aa0_panel_desc
> > s6d7aa0_lsl080al02_desc = {
> > .panel_type = S6D7AA0_PANEL_LSL080AL02,
> > .init_func = s6d7aa0_lsl080al02_init,
> > .off_func = s6d7aa0_lsl080al02_off,
> > - .drm_mode = s6d7aa0_lsl080al02_mode,
> > + .drm_mode = _lsl080al02_mode,
> > .mode_flags = MIPI_DSI_MODE_VSYNC_FLUSH |
> > MIPI_DSI_MODE_VIDEO_NO_HFP,
> > .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> >
> > @@ -412,7 +412,7 @@ static const struct s6d7aa0_panel_desc
> > s6d7aa0_lsl080al03_desc = {
> > .panel_type = S6D7AA0_PANEL_LSL080AL03,
> > .init_func = s6d7aa0_lsl080al03_init,
> > .off_func = s6d7aa0_lsl080al03_off,
> > - .drm_mode = s6d7aa0_lsl080al03_mode,
> > + .drm_mode = _lsl080al03_mode,
> > .mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET,
> > .bus_flags = 0,
> >
> > @@ -440,7 +440,7 @@ static const struct s6d7aa0_panel_desc
> > s6d7aa0_ltl101at01_desc = {
> > .panel_type = S6D7AA0_PANEL_LTL101AT01,
> > .init_func = s6d7aa0_lsl080al03_init, /* Similar init to LSL080AL03
> > */
> > .off_func = s6d7aa0_lsl080al03_off,
> > - .drm_mode = s6d7aa0_ltl101at01_mode,
> > + .drm_mode = _ltl101at01_mode,
> > .mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET,
> > .bus_flags = 0,
> >
> > @@ -458,7 +458,7 @@ static int s6d7aa0_get_modes(struct drm_panel *panel,
> > if (!ctx)
> > return -EINVAL;
> >
> > - mode = drm_mode_duplicate(connector->dev, >desc->drm_mode);
> > + mode = drm_mode_duplicate(connector->dev, ctx->desc->drm_mode);
> > if (!mode)
> > return -ENOMEM;
> >
> >
> > base-commit: 37cee4876a45a5c3da79a83d34ed4f3c68548aef
> > --
> > 2.40.1
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/panel: samsung-s6d7aa0: use pointer for drm_mode in panel desc struct
On Tue, May 23, 2023 at 2:20 PM Artur Weber wrote:
>
> Fixes compilation errors on older GCC versions (before 8.x) and Clang
> after changes introduced in commit 6810bb390282 ("drm/panel: Add
> Samsung S6D7AA0 panel controller driver"). Tested with GCC 13.1.1,
> GCC 6.4.0 and Clang 16.0.3.
Hi Artur,
Thanks for the patch! Mind sending a v2 with the diagnostic added to
the commit message? This gives reviewers much more context as to what
issue you are fixing.
>
> Signed-off-by: Artur Weber
> ---
> drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
> b/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
> index f532aa018428..102e1fc7ee38 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
> @@ -37,7 +37,7 @@ struct s6d7aa0_panel_desc {
> unsigned int panel_type;
> int (*init_func)(struct s6d7aa0 *ctx);
> int (*off_func)(struct s6d7aa0 *ctx);
> - const struct drm_display_mode drm_mode;
> + const struct drm_display_mode *drm_mode;
> unsigned long mode_flags;
> u32 bus_flags;
> bool has_backlight;
> @@ -309,7 +309,7 @@ static const struct s6d7aa0_panel_desc
> s6d7aa0_lsl080al02_desc = {
> .panel_type = S6D7AA0_PANEL_LSL080AL02,
> .init_func = s6d7aa0_lsl080al02_init,
> .off_func = s6d7aa0_lsl080al02_off,
> - .drm_mode = s6d7aa0_lsl080al02_mode,
> + .drm_mode = _lsl080al02_mode,
> .mode_flags = MIPI_DSI_MODE_VSYNC_FLUSH | MIPI_DSI_MODE_VIDEO_NO_HFP,
> .bus_flags = DRM_BUS_FLAG_DE_HIGH,
>
> @@ -412,7 +412,7 @@ static const struct s6d7aa0_panel_desc
> s6d7aa0_lsl080al03_desc = {
> .panel_type = S6D7AA0_PANEL_LSL080AL03,
> .init_func = s6d7aa0_lsl080al03_init,
> .off_func = s6d7aa0_lsl080al03_off,
> - .drm_mode = s6d7aa0_lsl080al03_mode,
> + .drm_mode = _lsl080al03_mode,
> .mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET,
> .bus_flags = 0,
>
> @@ -440,7 +440,7 @@ static const struct s6d7aa0_panel_desc
> s6d7aa0_ltl101at01_desc = {
> .panel_type = S6D7AA0_PANEL_LTL101AT01,
> .init_func = s6d7aa0_lsl080al03_init, /* Similar init to LSL080AL03 */
> .off_func = s6d7aa0_lsl080al03_off,
> - .drm_mode = s6d7aa0_ltl101at01_mode,
> + .drm_mode = _ltl101at01_mode,
> .mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET,
> .bus_flags = 0,
>
> @@ -458,7 +458,7 @@ static int s6d7aa0_get_modes(struct drm_panel *panel,
> if (!ctx)
> return -EINVAL;
>
> - mode = drm_mode_duplicate(connector->dev, >desc->drm_mode);
> + mode = drm_mode_duplicate(connector->dev, ctx->desc->drm_mode);
> if (!mode)
> return -ENOMEM;
>
>
> base-commit: 37cee4876a45a5c3da79a83d34ed4f3c68548aef
> --
> 2.40.1
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH v2 1/2] phy: mediatek: hdmi: mt8195: fix uninitialized variable usage in pll_calc
On Fri, Apr 21, 2023 at 3:13 PM Nathan Chancellor wrote:
>
> On Fri, Apr 14, 2023 at 06:07:46PM +0200, Guillaume Ranquet wrote:
> > The ret variable in mtk_hdmi_pll_calc() was used unitialized as reported
> > by the kernel test robot.
> >
> > Fix the issue by removing the variable altogether and testing out the
> > return value of mtk_hdmi_pll_set_hw()
> >
> > Fixes: 45810d486bb44 ("phy: mediatek: add support for phy-mtk-hdmi-mt8195")
> > Reported-by: kernel test robot
> > Signed-off-by: Guillaume Ranquet
>
> Reviewed-by: Nathan Chancellor
>
> Can somebody pick this up? It fixes a rather obvious warning, which is
> breaking clang builds (as evidenced by three versions of the same fix).
$ ./scripts/get_maintainer.pl -f
drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | grep maintainer
Chunfeng Yun (maintainer:ARM/Mediatek USB3
PHY DRIVER)
Matthias Brugger (maintainer:ARM/Mediatek SoC support)
Chunfeng, Matthias, can one of you pick this up, please?
Or Vinod who merged 45810d486bb44 FWICT?
>
> > ---
> > drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c | 8 ++--
> > 1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> > b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> > index abfc077fb0a8..054b73cb31ee 100644
> > --- a/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> > +++ b/drivers/phy/mediatek/phy-mtk-hdmi-mt8195.c
> > @@ -213,7 +213,7 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy
> > *hdmi_phy, struct clk_hw *hw,
> > u64 tmds_clk, pixel_clk, da_hdmitx21_ref_ck, ns_hdmipll_ck, pcw;
> > u8 txpredivs[4] = { 2, 4, 6, 12 };
> > u32 fbkdiv_low;
> > - int i, ret;
> > + int i;
> >
> > pixel_clk = rate;
> > tmds_clk = pixel_clk;
> > @@ -292,13 +292,9 @@ static int mtk_hdmi_pll_calc(struct mtk_hdmi_phy
> > *hdmi_phy, struct clk_hw *hw,
> > if (!(digital_div <= 32 && digital_div >= 1))
> > return -EINVAL;
> >
> > - mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low,
> > + return mtk_hdmi_pll_set_hw(hw, PLL_PREDIV, fbkdiv_high, fbkdiv_low,
> > PLL_FBKDIV_HS3, posdiv1, posdiv2, txprediv,
> > txposdiv, digital_div);
> > - if (ret)
> > - return -EINVAL;
> > -
> > - return 0;
> > }
> >
> > static int mtk_hdmi_pll_drv_setting(struct clk_hw *hw)
> >
> > --
> > 2.40.0
> >
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] accel/qaic: initialize ret variable to 0
On Tue, Apr 18, 2023 at 1:46 PM Jeffrey Hugo wrote:
>
> On 4/18/2023 1:20 PM, Tom Rix wrote:
> > clang static analysis reports
> > drivers/accel/qaic/qaic_data.c:610:2: warning: Undefined or garbage
> >value returned to caller [core.uninitialized.UndefReturn]
> > return ret;
> > ^~
> >
> > The ret variable is only set some of the time but is always returned.
> > So initialize ret to 0.
>
> This does not appear to be entirely accurate to me.
>
> Do you know what analysis Clang is doing? Is it limited to the function
> itself?
>
> remap_pfn_range, which initializes ret, will always run at-least once.
What happens if the loop body is never executed, say if `bo->sgt->sgl` is NULL?
>
> Feels more accurate to say that Clang cannot detect that ret will be
> initialized, and we want to quiet the warning from the false positive.
>
> > Fixes: ff13be830333 ("accel/qaic: Add datapath")
> > Signed-off-by: Tom Rix
> > ---
> > drivers/accel/qaic/qaic_data.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/accel/qaic/qaic_data.c b/drivers/accel/qaic/qaic_data.c
> > index c0a574cd1b35..b46a16fb3080 100644
> > --- a/drivers/accel/qaic/qaic_data.c
> > +++ b/drivers/accel/qaic/qaic_data.c
> > @@ -591,7 +591,7 @@ static int qaic_gem_object_mmap(struct drm_gem_object
> > *obj, struct vm_area_struc
> > struct qaic_bo *bo = to_qaic_bo(obj);
> > unsigned long offset = 0;
> > struct scatterlist *sg;
> > - int ret;
> > + int ret = 0;
> >
> > if (obj->import_attach)
> > return -EINVAL;
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/qxl: remove variable count
On Sat, Apr 8, 2023 at 9:50 AM Tom Rix wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/qxl/qxl_cmd.c:424:6: error: variable
> 'count' set but not used [-Werror,-Wunused-but-set-variable]
> int count = 0;
> ^
> This variable is not used so remove it.
Thanks for the patch!
Fixes: 64122c1f6ad ("drm: add new QXL driver. (v1.4)")
Reviewed-by: Nick Desaulniers
>
> Signed-off-by: Tom Rix
> ---
> drivers/gpu/drm/qxl/qxl_cmd.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_cmd.c b/drivers/gpu/drm/qxl/qxl_cmd.c
> index 281edab518cd..d6ea01f3797b 100644
> --- a/drivers/gpu/drm/qxl/qxl_cmd.c
> +++ b/drivers/gpu/drm/qxl/qxl_cmd.c
> @@ -421,7 +421,6 @@ int qxl_surface_id_alloc(struct qxl_device *qdev,
> {
> uint32_t handle;
> int idr_ret;
> - int count = 0;
> again:
> idr_preload(GFP_ATOMIC);
> spin_lock(>surf_id_idr_lock);
> @@ -433,7 +432,6 @@ int qxl_surface_id_alloc(struct qxl_device *qdev,
> handle = idr_ret;
>
> if (handle >= qdev->rom->n_surfaces) {
> - count++;
> spin_lock(>surf_id_idr_lock);
> idr_remove(>surf_id_idr, handle);
> spin_unlock(>surf_id_idr_lock);
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/amd/display: remove unused matching_stream_ptrs variable
On Fri, Apr 7, 2023 at 10:52 AM Nick Desaulniers
wrote:
>
> Jimmy, can you review?
>
> The change LGTM; but I'm not sure if there was something else intended here.
Nevermind, Jimmy's email address bounced.
Reviewed-by: Nick Desaulniers
>
> On Sat, Mar 25, 2023 at 6:45 AM Tom Rix wrote:
> >
> > clang with W=1 reports
> > drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_enc_cfg.c:625:6:
> > error:
> > variable 'matching_stream_ptrs' set but not used
> > [-Werror,-Wunused-but-set-variable]
> > int matching_stream_ptrs = 0;
> > ^
> > This variable is not used so remove it.
> >
> > Signed-off-by: Tom Rix
> > ---
> > drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c | 5 +
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> > b/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> > index 41198c729d90..30c0644d4418 100644
> > --- a/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> > @@ -622,7 +622,6 @@ bool link_enc_cfg_validate(struct dc *dc, struct
> > dc_state *state)
> > int i, j;
> > uint8_t valid_count = 0;
> > uint8_t dig_stream_count = 0;
> > - int matching_stream_ptrs = 0;
> > int eng_ids_per_ep_id[MAX_PIPES] = {0};
> > int ep_ids_per_eng_id[MAX_PIPES] = {0};
> > int valid_bitmap = 0;
> > @@ -645,9 +644,7 @@ bool link_enc_cfg_validate(struct dc *dc, struct
> > dc_state *state)
> > struct link_enc_assignment assignment =
> > state->res_ctx.link_enc_cfg_ctx.link_enc_assignments[i];
> >
> > if (assignment.valid) {
> > - if (assignment.stream == state->streams[i])
> > - matching_stream_ptrs++;
> > - else
> > + if (assignment.stream != state->streams[i])
> > valid_stream_ptrs = false;
> > }
> > }
> > --
> > 2.27.0
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/nouveau/acr: remove unused loc variable
On Fri, Mar 31, 2023 at 1:42 PM Tom Rix wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c:221:7: error: variable
> 'loc' set but not used [-Werror,-Wunused-but-set-variable]
> u32 loc, sig, cnt, *meta;
> ^
> This variable is not used so remove it.
>
> Signed-off-by: Tom Rix
Ben, any idea if this was supposed to be used somewhere? If not:
Fixes: 4b569ded09fd ("drm/nouveau/acr/ga102: initial support")
Reviewed-by: Nick Desaulniers
> ---
> drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c
> index f36a359d4531..bd104a030243 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/acr/lsfw.c
> @@ -218,7 +218,7 @@ nvkm_acr_lsfw_load_sig_image_desc_v2(struct nvkm_subdev
> *subdev,
> const struct firmware *hsbl;
> const struct nvfw_ls_hsbl_bin_hdr *hdr;
> const struct nvfw_ls_hsbl_hdr *hshdr;
> - u32 loc, sig, cnt, *meta;
> + u32 sig, cnt, *meta;
>
> ret = nvkm_firmware_load_name(subdev, path, "hs_bl_sig", ver,
> );
> if (ret)
> @@ -227,7 +227,6 @@ nvkm_acr_lsfw_load_sig_image_desc_v2(struct nvkm_subdev
> *subdev,
> hdr = nvfw_ls_hsbl_bin_hdr(subdev, hsbl->data);
> hshdr = nvfw_ls_hsbl_hdr(subdev, hsbl->data +
> hdr->header_offset);
> meta = (u32 *)(hsbl->data + hshdr->meta_data_offset);
> - loc = *(u32 *)(hsbl->data + hshdr->patch_loc);
> sig = *(u32 *)(hsbl->data + hshdr->patch_sig);
> cnt = *(u32 *)(hsbl->data + hshdr->num_sig);
>
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/qxl: remove unused num_relocs variable
On Fri, Mar 31, 2023 at 10:24 AM Tom Rix wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/qxl/qxl_ioctl.c:149:14: error: variable
> 'num_relocs' set but not used [-Werror,-Wunused-but-set-variable]
> int i, ret, num_relocs;
> ^
> This variable is not used so remove it.
>
> Signed-off-by: Tom Rix
Thanks for the patch!
Fixes: 74d9a6335dce ("drm/qxl: Simplify cleaning qxl processing command")
Reviewed-by: Nick Desaulniers
That commit should also have removed the label out_free_bos IMO since
having two labels for the same statement is a code smell. Tom, do you
mind sending a v2 with that folded in?
> ---
> drivers/gpu/drm/qxl/qxl_ioctl.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
> index 30f58b21372a..3422206d59d4 100644
> --- a/drivers/gpu/drm/qxl/qxl_ioctl.c
> +++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
> @@ -146,7 +146,7 @@ static int qxl_process_single_command(struct qxl_device
> *qdev,
> struct qxl_release *release;
> struct qxl_bo *cmd_bo;
> void *fb_cmd;
> - int i, ret, num_relocs;
> + int i, ret;
> int unwritten;
>
> switch (cmd->type) {
> @@ -201,7 +201,6 @@ static int qxl_process_single_command(struct qxl_device
> *qdev,
> }
>
> /* fill out reloc info structs */
> - num_relocs = 0;
> for (i = 0; i < cmd->relocs_num; ++i) {
> struct drm_qxl_reloc reloc;
> struct drm_qxl_reloc __user *u = u64_to_user_ptr(cmd->relocs);
> @@ -231,7 +230,6 @@ static int qxl_process_single_command(struct qxl_device
> *qdev,
> reloc_info[i].dst_bo = cmd_bo;
> reloc_info[i].dst_offset = reloc.dst_offset +
> release->release_offset;
> }
> - num_relocs++;
>
> /* reserve and validate the reloc dst bo */
> if (reloc.reloc_type == QXL_RELOC_TYPE_BO ||
> reloc.src_handle) {
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/amd/pm: remove unused num_of_active_display variable
On Fri, Mar 31, 2023 at 9:40 AM Tom Rix wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/amd/amdgpu/../pm/swsmu/amdgpu_smu.c:1700:6: error: variable
> 'num_of_active_display' set but not used [-Werror,-Wunused-but-set-variable]
> int num_of_active_display = 0;
> ^
> This variable is not used so remove it.
>
> Signed-off-by: Tom Rix
Thanks for the patch!
Fixes: commit 75145aab7a0d ("drm/amdgpu/swsmu: clean up a bunch of
stale interfaces")
Reviewed-by: Nick Desaulniers
> ---
> drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 7 ---
> 1 file changed, 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> index b5d64749990e..f93f7a9ed631 100644
> --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> @@ -1696,8 +1696,6 @@ static int smu_display_configuration_change(void
> *handle,
> const struct
> amd_pp_display_configuration *display_config)
> {
> struct smu_context *smu = handle;
> - int index = 0;
> - int num_of_active_display = 0;
>
> if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
> return -EOPNOTSUPP;
> @@ -1708,11 +1706,6 @@ static int smu_display_configuration_change(void
> *handle,
> smu_set_min_dcef_deep_sleep(smu,
>
> display_config->min_dcef_deep_sleep_set_clk / 100);
>
> - for (index = 0; index <
> display_config->num_path_including_non_display; index++) {
> - if (display_config->displays[index].controller_id != 0)
> - num_of_active_display++;
> - }
> -
> return 0;
> }
>
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/nouveau/svm: remove unused ret variable
On Wed, Mar 29, 2023 at 4:14 PM Tom Rix wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/nouveau/nouveau_svm.c:929:6: error: variable
> 'ret' set but not used [-Werror,-Wunused-but-set-variable]
> int ret;
> ^
> This variable is not used so remove it.
>
> Signed-off-by: Tom Rix
> ---
> drivers/gpu/drm/nouveau/nouveau_svm.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c
> b/drivers/gpu/drm/nouveau/nouveau_svm.c
> index a74ba8d84ba7..e072d610f2f9 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_svm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
> @@ -926,15 +926,14 @@ nouveau_pfns_map(struct nouveau_svmm *svmm, struct
> mm_struct *mm,
> unsigned long addr, u64 *pfns, unsigned long npages)
> {
> struct nouveau_pfnmap_args *args = nouveau_pfns_to_args(pfns);
> - int ret;
>
> args->p.addr = addr;
> args->p.size = npages << PAGE_SHIFT;
>
> mutex_lock(>mutex);
>
> - ret = nvif_object_ioctl(>vmm->vmm.object, args,
> - struct_size(args, p.phys, npages), NULL);
> + nvif_object_ioctl(>vmm->vmm.object, args,
> + struct_size(args, p.phys, npages), NULL);
nvif_object_ioctl() may return -ENOSYS. Seeing other call sites have
comments like:
114 /*XXX: warn? */
134 /*XXX: warn? */
or other places where the return code isn't checked makes me think
that a better approach would be to
1. plumb error codes back up through nouveau_pfns_map() (and other
call sites of nvif_object_ioctl)
2. consider making nvif_object_ioctl __must_check
>
> mutex_unlock(>mutex);
> }
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/amd/display: remove unused matching_stream_ptrs variable
Jimmy, can you review?
The change LGTM; but I'm not sure if there was something else intended here.
On Sat, Mar 25, 2023 at 6:45 AM Tom Rix wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc_link_enc_cfg.c:625:6: error:
> variable 'matching_stream_ptrs' set but not used
> [-Werror,-Wunused-but-set-variable]
> int matching_stream_ptrs = 0;
> ^
> This variable is not used so remove it.
>
> Signed-off-by: Tom Rix
> ---
> drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> b/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> index 41198c729d90..30c0644d4418 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_enc_cfg.c
> @@ -622,7 +622,6 @@ bool link_enc_cfg_validate(struct dc *dc, struct dc_state
> *state)
> int i, j;
> uint8_t valid_count = 0;
> uint8_t dig_stream_count = 0;
> - int matching_stream_ptrs = 0;
> int eng_ids_per_ep_id[MAX_PIPES] = {0};
> int ep_ids_per_eng_id[MAX_PIPES] = {0};
> int valid_bitmap = 0;
> @@ -645,9 +644,7 @@ bool link_enc_cfg_validate(struct dc *dc, struct dc_state
> *state)
> struct link_enc_assignment assignment =
> state->res_ctx.link_enc_cfg_ctx.link_enc_assignments[i];
>
> if (assignment.valid) {
> - if (assignment.stream == state->streams[i])
> - matching_stream_ptrs++;
> - else
> + if (assignment.stream != state->streams[i])
> valid_stream_ptrs = false;
> }
> }
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/vmwgfx: remove unused mksstat_init_record function
On Fri, Mar 24, 2023 at 12:54 PM Tom Rix wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/vmwgfx/vmwgfx_msg.c:716:21: error: unused function
> 'mksstat_init_record' [-Werror,-Wunused-function]
> static inline char *mksstat_init_record(mksstat_kern_stats_t stat_idx,
> ^
> This function is not used so remove it.
>
> Signed-off-by: Tom Rix
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 26 --
> 1 file changed, 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> index e76976a95a1e..ca1a3fe44fa5 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
> @@ -702,32 +702,6 @@ static inline void hypervisor_ppn_remove(PPN64 pfn)
> /* Header to the text description of mksGuestStat instance descriptor */
> #define MKSSTAT_KERNEL_DESCRIPTION "vmwgfx"
>
> -/**
> - * mksstat_init_record: Initializes an MKSGuestStatCounter-based record
> - * for the respective mksGuestStat index.
> - *
> - * @stat_idx: Index of the MKSGuestStatCounter-based mksGuestStat record.
> - * @pstat: Pointer to array of MKSGuestStatCounterTime.
> - * @pinfo: Pointer to array of MKSGuestStatInfoEntry.
> - * @pstrs: Pointer to current end of the name/description sequence.
> - * Return: Pointer to the new end of the names/description sequence.
> - */
> -
> -static inline char *mksstat_init_record(mksstat_kern_stats_t stat_idx,
> - MKSGuestStatCounterTime *pstat, MKSGuestStatInfoEntry *pinfo, char
> *pstrs)
> -{
> - char *const pstrd = pstrs +
> strlen(mksstat_kern_name_desc[stat_idx][0]) + 1;
> - strcpy(pstrs, mksstat_kern_name_desc[stat_idx][0]);
> - strcpy(pstrd, mksstat_kern_name_desc[stat_idx][1]);
> -
> - pinfo[stat_idx].name.s = pstrs;
> - pinfo[stat_idx].description.s = pstrd;
> - pinfo[stat_idx].flags = MKS_GUEST_STAT_FLAG_NONE;
This was the last user of MKS_GUEST_STAT_FLAG_NONE, is there anything
else to clean up there? Otherwise LGTM.
Reviewed-by: Nick Desaulniers
> - pinfo[stat_idx].stat.counter = (MKSGuestStatCounter
> *)[stat_idx];
> -
> - return pstrd + strlen(mksstat_kern_name_desc[stat_idx][1]) + 1;
> -}
> -
> /**
> * mksstat_init_record_time: Initializes an MKSGuestStatCounterTime-based
> record
> * for the respective mksGuestStat index.
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/vmwgfx: remove unused vmw_overlay function
On Tue, Mar 21, 2023 at 11:24 AM Tom Rix wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c:56:35: error:
> unused function 'vmw_overlay' [-Werror,-Wunused-function]
> static inline struct vmw_overlay *vmw_overlay(struct drm_device *dev)
> ^
> This function is not used, so remove it.
>
> Signed-off-by: Tom Rix
Thanks for the patch!
Reviewed-by: Nick Desaulniers
> ---
> drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
> b/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
> index 8d171d71cb8a..7e112319a23c 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
> @@ -53,12 +53,6 @@ struct vmw_overlay {
> struct vmw_stream stream[VMW_MAX_NUM_STREAMS];
> };
>
> -static inline struct vmw_overlay *vmw_overlay(struct drm_device *dev)
> -{
> - struct vmw_private *dev_priv = vmw_priv(dev);
> - return dev_priv ? dev_priv->overlay_priv : NULL;
> -}
> -
> struct vmw_escape_header {
> uint32_t cmd;
> SVGAFifoCmdEscape body;
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/kmb: remove unused set_test_mode_src_osc_freq_target_low, hi_bits functions
On Sat, Mar 18, 2023 at 6:39 AM Tom Rix wrote:
>
> clang with W=1 reports
> drivers/gpu/drm/kmb/kmb_dsi.c:822:2: error: unused function
> 'set_test_mode_src_osc_freq_target_low_bits' [-Werror,-Wunused-function]
> set_test_mode_src_osc_freq_target_low_bits(struct kmb_dsi *kmb_dsi,
> ^
> drivers/gpu/drm/kmb/kmb_dsi.c:834:2: error: unused function
> 'set_test_mode_src_osc_freq_target_hi_bits' [-Werror,-Wunused-function]
> set_test_mode_src_osc_freq_target_hi_bits(struct kmb_dsi *kmb_dsi,
> ^
> These static functions are not used, so remove them.
>
> Signed-off-by: Tom Rix
Thanks for the patch!
Reviewed-by: Nick Desaulniers
> ---
> drivers/gpu/drm/kmb/kmb_dsi.c | 28
> 1 file changed, 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c
> index cf7cf0b07541..ed99b14375aa 100644
> --- a/drivers/gpu/drm/kmb/kmb_dsi.c
> +++ b/drivers/gpu/drm/kmb/kmb_dsi.c
> @@ -818,34 +818,6 @@ static void test_mode_send(struct kmb_dsi *kmb_dsi, u32
> dphy_no,
> }
> }
>
> -static inline void
> - set_test_mode_src_osc_freq_target_low_bits(struct kmb_dsi *kmb_dsi,
> - u32 dphy_no,
> - u32 freq)
> -{
> - /* Typical rise/fall time=166, refer Table 1207 databook,
> -* sr_osc_freq_target[7:0]
> -*/
> - test_mode_send(kmb_dsi, dphy_no, TEST_CODE_SLEW_RATE_DDL_CYCLES,
> - (freq & 0x7f));
> -}
> -
> -static inline void
> - set_test_mode_src_osc_freq_target_hi_bits(struct kmb_dsi *kmb_dsi,
> - u32 dphy_no,
> - u32 freq)
> -{
> - u32 data;
> -
> - /* Flag this as high nibble */
> - data = ((freq >> 6) & 0x1f) | (1 << 7);
> -
> - /* Typical rise/fall time=166, refer Table 1207 databook,
> -* sr_osc_freq_target[11:7]
> -*/
> - test_mode_send(kmb_dsi, dphy_no, TEST_CODE_SLEW_RATE_DDL_CYCLES,
> data);
> -}
> -
> static void mipi_tx_get_vco_params(struct vco_params *vco)
> {
> int i;
> --
> 2.27.0
>
--
Thanks,
~Nick Desaulniers
Re: Linux 6.3-rc3
+ Masahiro and linux-kbuild for the proposal
On Wed, Mar 22, 2023 at 9:56 AM Linus Torvalds
wrote:
>
> On Wed, Mar 22, 2023 at 9:40 AM Sedat Dilek wrote:
> >
> > You have to pass `make LLVM=1` in any case... to `oldconfig` or when
> > adding any MAKEFLAGS like -j${number-of-available-cpus}.
>
> I actually think we should look (again) at just making the compiler
> choice (and the prefix) be a Kconfig option.
>
> That would simplify *so* many use cases.
>
> It used to be that gcc was "THE compiler" and anything else was just
> an odd toy special case, but that's clearly not true any more.
<3
>
> So it would be lovely to make the kernel choice a Kconfig choice - so
> you'd set it only at config time, and then after that a kernel build
> wouldn't need special flags any more, and you'd never need to play
> games with GNUmakefile or anything like that.
>
> Yes, you'd still use environment variables (or make arguments) for
> that initial Kconfig, but that's no different from the other
> environment variables we already have, like KCONFIG_SEED that kconfig
> uses internally, but also things like "$(ARCH)" that we already use
> *inside* the Kconfig files themselves.
>
> I really dislike how you have to set ARCH and CROSS_COMPILE etc
> externally, and can't just have them *in* the config file.
Not needing CROSS_COMPILE for LLVM=1 has been great. ;)
(Still need it for ARCH=s390 until LLD gets s390 support though)
>
> So when you do cross-compiles, right now you have to do something like
>
> make ARCH=i386 allmodconfig
>
> to build the .config file, but then you have to *repeat* that
> ARCH=i386 when you actually build things:
>
> make ARCH=i386
>
> because the ARCH choice ends up being in the .config file, but the
> makefiles themselves always take it from the environment.
>
> There are good historical reasons for our behavior (and probably a
> number of extant practical reasons too), but it's a bit annoying, and
> it would be lovely if we could start moving away from this model.
>
> Linus
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] gpu: host1x: fix uninitialized variable use
On Fri, Jan 27, 2023 at 11:14:00PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> The error handling for platform_get_irq() failing no longer
> works after a recent change, clang now points this out with
> a warning:
>
> drivers/gpu/host1x/dev.c:520:6: error: variable 'syncpt_irq' is uninitialized
> when used here [-Werror,-Wuninitialized]
> if (syncpt_irq < 0)
> ^~
>
> Fix this by removing the variable and checking the correct
> error status.
>
> Fixes: 625d4ffb438c ("gpu: host1x: Rewrite syncpoint interrupt handling")
> Signed-off-by: Arnd Bergmann
Thanks Arnd, I saw some reports from kernelci about this, too.
https://lore.kernel.org/linux-next/?q=warning%3A+variable+%27syncpt_irq%27+is+uninitialized+when+used+here
Reported-by: "kernelci.org bot"
Reviewed-by: Nick Desaulniers
> ---
> drivers/gpu/host1x/dev.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
> index 4872d183d860..aae2efeef503 100644
> --- a/drivers/gpu/host1x/dev.c
> +++ b/drivers/gpu/host1x/dev.c
> @@ -487,7 +487,6 @@ static int host1x_get_resets(struct host1x *host)
> static int host1x_probe(struct platform_device *pdev)
> {
> struct host1x *host;
> - int syncpt_irq;
> int err;
>
> host = devm_kzalloc(>dev, sizeof(*host), GFP_KERNEL);
> @@ -517,8 +516,8 @@ static int host1x_probe(struct platform_device *pdev)
> }
>
> host->syncpt_irq = platform_get_irq(pdev, 0);
> - if (syncpt_irq < 0)
> - return syncpt_irq;
> + if (host->syncpt_irq < 0)
> + return host->syncpt_irq;
>
> mutex_init(>devices_lock);
> INIT_LIST_HEAD(>devices);
> --
> 2.39.0
>
>
Re: [PATCH v2 2/2] drm/vc4: hdmi: Fix pointer dereference before check
On Thu, Nov 10, 2022 at 02:47:52PM +0100, José Expósito wrote:
> Commit 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug") introduced
> the vc4_hdmi_reset_link() function. This function dereferences the
> "connector" pointer before checking whether it is NULL or not.
>
> Rework variable assignment to avoid this issue.
>
> Fixes: 6bed2ea3cb38 ("drm/vc4: hdmi: Reset link on hotplug")
> Signed-off-by: José Expósito
> ---
> drivers/gpu/drm/vc4/vc4_hdmi.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index a49f88e5d2b9..6b223a5fcf6f 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -318,8 +318,8 @@ static int reset_pipe(struct drm_crtc *crtc,
> static int vc4_hdmi_reset_link(struct drm_connector *connector,
> struct drm_modeset_acquire_ctx *ctx)
> {
> - struct drm_device *drm = connector->dev;
> - struct vc4_hdmi *vc4_hdmi = connector_to_vc4_hdmi(connector);
> + struct drm_device *drm;
> + struct vc4_hdmi *vc4_hdmi;
Hi, I think this change, or another in this area recently, is causing
the following warning. PTAL
drivers/gpu/drm/vc4/vc4_hdmi.c:351:14: warning: variable 'vc4_hdmi' is
uninitialized when used here [-Wuninitialized]
mutex_lock(_hdmi->mutex);
^~~~
drivers/gpu/drm/vc4/vc4_hdmi.c:322:27: note: initialize the variable 'vc4_hdmi'
to silence this warning
struct vc4_hdmi *vc4_hdmi;
^
= NULL
> struct drm_connector_state *conn_state;
> struct drm_crtc_state *crtc_state;
> struct drm_crtc *crtc;
> @@ -330,6 +330,7 @@ static int vc4_hdmi_reset_link(struct drm_connector
> *connector,
> if (!connector)
> return 0;
>
> + drm = connector->dev;
> ret = drm_modeset_lock(>mode_config.connection_mutex, ctx);
> if (ret)
> return ret;
> @@ -347,6 +348,7 @@ static int vc4_hdmi_reset_link(struct drm_connector
> *connector,
> if (!crtc_state->active)
> return 0;
>
> + vc4_hdmi = connector_to_vc4_hdmi(connector);
> if (!vc4_hdmi_supports_scrambling(vc4_hdmi))
> return 0;
>
> --
> 2.25.1
>
>
Re: [PATCH v2] overflow: Introduce overflows_type() and castable_to_type()
+ Arnd
On Mon, Sep 26, 2022 at 12:11 PM Kees Cook wrote:
> ---
> v2:
> - fix comment typo
> - wrap clang pragma to avoid GCC warnings
> - style nit cleanups
> - rename __castable_to_type() to castable_to_type()
> - remove prior overflows_type() definition
> v1: https://lore.kernel.org/lkml/20220926003743.409911-1-keesc...@chromium.org
> diff --git a/lib/overflow_kunit.c b/lib/overflow_kunit.c
> index f385ca652b74..fffc3f86181d 100644
> --- a/lib/overflow_kunit.c
> +++ b/lib/overflow_kunit.c
> @@ -16,6 +16,11 @@
> #include
> #include
>
> +/* We're expecting to do a lot of "always true" or "always false" tests. */
> +#ifdef CONFIG_CC_IS_CLANG
> +#pragma clang diagnostic ignored
> "-Wtautological-constant-out-of-range-compare"
> +#endif
Any chance we can reuse parts of __diag_ignore or __diag_clang from
include/linux/compiler_types.h or include/linux/compiler-clang.h
respectively?
Those are needed for pragmas within preprocessor macros, which we
don't have here, but I suspect they may be more concise to use here.
> +#define TEST_SAME_TYPE(t1, t2, same) do {\
> + typeof(t1) __t1h = type_max(t1);\
> + typeof(t1) __t1l = type_min(t1);\
> + typeof(t2) __t2h = type_max(t2);\
> + typeof(t2) __t2l = type_min(t2); \
Can we use __auto_type here rather than typeof(macro expansion)?
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/amd: Return NULL instead of false in dcn201_acquire_idle_pipe_for_layer()
On Thu, Sep 30, 2021 at 10:10 AM Alex Deucher wrote: > > Applied. Thanks! > > Alex > > On Thu, Sep 30, 2021 at 12:23 PM Nathan Chancellor wrote: > > > > Clang warns: Any chance AMDGPU folks can look into adding clang to the CI roster? -- Thanks, ~Nick Desaulniers
Re: [PATCH 3/3] drm/i915: Enable -Wsometimes-uninitialized
On Tue, Aug 24, 2021 at 3:54 PM Nathan Chancellor wrote:
>
> This warning helps catch uninitialized variables. It should have been
> enabled at the same time as commit b2423184ac33 ("drm/i915: Enable
> -Wuninitialized") but I did not realize they were disabled separately.
> Enable it now that i915 is clean so that it stays that way.
>
> Signed-off-by: Nathan Chancellor
Thanks for the series!
Reviewed-by: Nick Desaulniers
> ---
> drivers/gpu/drm/i915/Makefile | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 642a5b5a1b81..335ba9f43d8f 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -19,7 +19,6 @@ subdir-ccflags-y += $(call cc-disable-warning,
> missing-field-initializers)
> subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable)
> # clang warnings
> subdir-ccflags-y += $(call cc-disable-warning, sign-compare)
> -subdir-ccflags-y += $(call cc-disable-warning, sometimes-uninitialized)
> subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides)
> subdir-ccflags-y += $(call cc-disable-warning, frame-address)
> subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
> --
> 2.33.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/i915: Clean up disabled warnings
On Tue, Aug 24, 2021 at 4:23 PM Nathan Chancellor wrote:
>
> i915 enables a wider set of warnings with '-Wall -Wextra' then disables
> several with cc-disable-warning. If an unknown flag gets added to
> KBUILD_CFLAGS when building with clang, all subsequent calls to
> cc-{disable-warning,option} will fail, meaning that all of these
> warnings do not get disabled [1].
>
> A separate series will address the root cause of the issue by not adding
> these flags when building with clang [2]; however, the symptom of these
> extra warnings appearing can be addressed separately by just removing
> the calls to cc-disable-warning, which makes the build ever so slightly
> faster because the compiler does not need to be called as much before
> building.
>
> The following warnings are supported by GCC 4.9 and clang 10.0.1, which
> are the minimum supported versions of these compilers so the call to
> cc-disable-warning is not necessary. Masahiro cleaned this up for the
> reset of the kernel in commit 4c8dd95a723d ("kbuild: add some extra
> warning flags unconditionally").
>
> * -Wmissing-field-initializers
> * -Wsign-compare
> * -Wtype-limits
> * -Wunused-parameter
>
> -Wunused-but-set-variable was implemented in clang 13.0.0 and
> -Wframe-address was implemented in clang 12.0.0 so the
> cc-disable-warning calls are kept for these two warnings.
>
> Lastly, -Winitializer-overrides is clang's version of -Woverride-init,
> which is disabled for the specific files that are problematic. clang
> added a compatibility alias in clang 8.0.0 so -Winitializer-overrides
> can be removed.
>
> [1]: https://lore.kernel.org/r/202108210311.cbtcgoul-...@intel.com/
> [2]: https://lore.kernel.org/r/20210824022640.2170859-1-nat...@kernel.org/
>
> Signed-off-by: Nathan Chancellor
Thanks for the patch! Do you need to re-ping, rebase, or resend that
other series?
Reviewed-by: Nick Desaulniers
> ---
>
> NOTE: This is based on my series to enable -Wsometimes-initialized here:
>
> https://lore.kernel.org/r/20210824225427.2065517-1-nat...@kernel.org/
>
> I sent it separately as this can go into whatever release but I would
> like for that series to go into 5.15.
>
> drivers/gpu/drm/i915/Makefile | 10 --
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 335ba9f43d8f..6b38547543b1 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -13,13 +13,11 @@
> # will most likely get a sudden build breakage... Hopefully we will fix
> # new warnings before CI updates!
> subdir-ccflags-y := -Wall -Wextra
> -subdir-ccflags-y += $(call cc-disable-warning, unused-parameter)
> -subdir-ccflags-y += $(call cc-disable-warning, type-limits)
> -subdir-ccflags-y += $(call cc-disable-warning, missing-field-initializers)
> +subdir-ccflags-y += -Wno-unused-parameter
> +subdir-ccflags-y += -Wno-type-limits
> +subdir-ccflags-y += -Wno-missing-field-initializers
> +subdir-ccflags-y += -Wno-sign-compare
> subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable)
> -# clang warnings
> -subdir-ccflags-y += $(call cc-disable-warning, sign-compare)
> -subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides)
> subdir-ccflags-y += $(call cc-disable-warning, frame-address)
> subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
>
>
> base-commit: fb43ebc83e069625cfeeb2490efc3ffa0013bfa4
> prerequisite-patch-id: 31c28450ed7e8785dce967a16db6d52eff3d7d6d
> prerequisite-patch-id: 372dfa0e07249f207acc1942ab0e39b13ff229b2
> prerequisite-patch-id: 1a585fa6cda50c32ad1e3ac8235d3cff1b599978
> --
> 2.33.0
>
--
Thanks,
~Nick Desaulniers
Re: [PATCH 01/64] media: omap3isp: Extract struct group for memcpy() region
On Fri, Jul 30, 2021 at 9:44 AM Kees Cook wrote:
>
> On Fri, Jul 30, 2021 at 12:00:54PM +0300, Dan Carpenter wrote:
> > On Fri, Jul 30, 2021 at 10:38:45AM +0200, David Sterba wrote:
> > > Then is explicit memset the only reliable way accross all compiler
> > > flavors and supported versions?
> > >
> >
> > The = { } initializer works. It's only when you start partially
> > initializing the struct that it doesn't initialize holes.
>
> No, partial works. It's when you _fully_ initialize the struct where the
> padding doesn't get initialized. *sob*
I'm pretty sure that this has more to do with whether or not the
compiler applies SROA then observes uses of the individual members or
not.
>
> struct foo {
> u8 flag;
> /* padding */
> void *ptr;
> };
>
> These are fine:
>
> struct foo ok1 = { };
> struct foo ok2 = { .flag = 7 };
> struct foo ok3 = { .ptr = NULL };
>
> This is not:
>
> struct foo bad = { .flag = 7, .ptr = NULL };
>
> (But, of course, it depends on padding size, compiler version, and
> architecture. i.e. things remain unreliable.)
>
> --
--
Thanks,
~Nick Desaulniers
Re: [PATCH 34/64] fortify: Detect struct member overflows in memcpy() at compile-time
, int pad)
> +static __always_inline void memcpy_and_pad(void *dest, size_t dest_len,
> + const void *src, size_t count,
> + int pad)
Why __always_inline here?
> {
> if (dest_len > count) {
> memcpy(dest, src, count);
> diff --git a/lib/Makefile b/lib/Makefile
> index 083a19336e20..74523fd394bd 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -370,7 +370,8 @@ TEST_FORTIFY_LOG = test_fortify.log
> quiet_cmd_test_fortify = TEST$@
>cmd_test_fortify = $(CONFIG_SHELL) $(srctree)/scripts/test_fortify.sh \
> $< $@ "$(NM)" $(CC) $(c_flags) \
> - $(call cc-disable-warning,fortify-source)
> + $(call cc-disable-warning,fortify-source) \
> + -DKBUILD_EXTRA_WARN1
>
> targets += $(TEST_FORTIFY_LOGS)
> clean-files += $(TEST_FORTIFY_LOGS)
> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> index faa9d8e4e2c5..4d205bf5993c 100644
> --- a/lib/string_helpers.c
> +++ b/lib/string_helpers.c
> @@ -884,6 +884,12 @@ char *strreplace(char *s, char old, char new)
> EXPORT_SYMBOL(strreplace);
>
> #ifdef CONFIG_FORTIFY_SOURCE
> +/* These are placeholders for fortify compile-time warnings. */
> +void __read_overflow2_field(void) { }
> +EXPORT_SYMBOL(__read_overflow2_field);
> +void __write_overflow_field(void) { }
> +EXPORT_SYMBOL(__write_overflow_field);
> +
Don't we rely on these being undefined for Clang to produce a linkage
failure (until https://reviews.llvm.org/D106030 has landed)? By
providing a symbol definition we can link against, I don't think
__compiletime_{warning|error} will warn at all with Clang?
> void fortify_panic(const char *name)
> {
> pr_emerg("detected buffer overflow in %s\n", name);
> diff --git a/lib/test_fortify/read_overflow2_field-memcpy.c
> b/lib/test_fortify/read_overflow2_field-memcpy.c
> new file mode 100644
> index ..de9569266223
> --- /dev/null
> +++ b/lib/test_fortify/read_overflow2_field-memcpy.c
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#define TEST \
> + memcpy(large, instance.buf, sizeof(instance.buf) + 1)
> +
> +#include "test_fortify.h"
> diff --git a/lib/test_fortify/write_overflow_field-memcpy.c
> b/lib/test_fortify/write_overflow_field-memcpy.c
> new file mode 100644
> index ..28cc81058dd3
> --- /dev/null
> +++ b/lib/test_fortify/write_overflow_field-memcpy.c
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#define TEST \
> + memcpy(instance.buf, large, sizeof(instance.buf) + 1)
> +
> +#include "test_fortify.h"
> --
I haven't read the whole series yet, but I assume test_fortify.h was
provided earlier in the series?
--
Thanks,
~Nick Desaulniers
Re: [PATCH] drm/amdgpu: make DRM_AMD_DC x86-only again
On Tue, Dec 8, 2020 at 2:18 PM Arnd Bergmann wrote: > > On Tue, Dec 8, 2020 at 7:21 PM 'Nick Desaulniers' via Clang Built > Linux wrote: > > > > On Tue, Dec 8, 2020 at 6:26 AM Arnd Bergmann wrote: > > > > > > On Mon, Dec 7, 2020 at 11:28 PM 'Nick Desaulniers' via Clang Built > > > Linux wrote: > > Hmm...no warnings for me with that config on next-20201208: > > $ make LLVM=1 -j71 olddefconfig > > $ make LLVM=1 -j71 > > ... > > $ clang --version > > clang version 12.0.0 (g...@github.com:llvm/llvm-project.git > > 1c98f984105e552daa83ed8e92c61fba0e401410) > > (ie. near ToT LLVM) > > > > I see CONFIG_KASAN=y in the .config, so that's a recurring theme with > > clang; excessive stack usage. It does seem a shame to disable a > > driver for a bunch of archs just due to KASAN stack usage. > > $ grep KASAN=y 0x9077925C_defconfig > > CONFIG_HAVE_ARCH_KASAN=y > > CONFIG_KASAN=y > > > > Is there a different branch of a different tree that I should be > > testing on instead? Otherwise, can you send the object file? > > I was on a slightly older linux-next, and the latest version contains > the patch "ubsan: enable for all*config builds" in linux-next, > which enables CONFIG_UBSAN_SANITIZE_ALL. It took me > an hour to figure out, but after turning that option off, the warning > is back. > > I'll send you the object for reference in a private mail, it's kind > of large. Got it, thanks. $ llvm-objdump -Dr --disassemble-symbols=dml30_ModeSupportAndSystemConfigurationFull display_mode_vba_30.o ... 1584f: 48 81 ec a0 08 00 00 subq$2208, %rsp ... $ python3 /android0/frame-larger-than/frame_larger_than.py display_mode_vba_30.o dml30_ModeSupportAndSystemConfigurationFull No dwarf info found in display_mode_vba_30.o $ llvm-readelf -S display_mode_vba_30.o | grep debug_info $ echo $? 1 Can you rebuild+resend with CONFIG_DEBUG_INFO enabled? frame_larger_than.py relies on the DWARF debug info to know what local variables occupy how much stack space. -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu: make DRM_AMD_DC x86-only again
On Tue, Dec 8, 2020 at 6:26 AM Arnd Bergmann wrote: > > On Mon, Dec 7, 2020 at 11:28 PM 'Nick Desaulniers' via Clang Built > Linux wrote: > > > > On Mon, Dec 7, 2020 at 2:17 PM Arnd Bergmann wrote: > > > > > > On Mon, Dec 7, 2020 at 11:08 PM 'Nick Desaulniers' via Clang Built > > > Linux wrote: > > > > > > > > On Mon, Dec 7, 2020 at 1:57 PM Arnd Bergmann wrote: > > > > > > > > > > > > > Right, looking at my latest randconfig logs, I see the same problem > > > > > on x86 > > > > > builds with clang as well, though I'm not entirely sure which other > > > > > configuration > > > > > options are needed to trigger it. > > > > > > > > > > So my patch can be disregarded, but I agree this needs a better fix, > > > > > either in clang or in the dcn driver. > > > > > > > > If you could give https://github.com/ClangBuiltLinux/frame-larger-than > > > > a spin again, I would appreciate any feedback. > > > > > > I've already tried it, but the tool doesn't seem to like me, I never > > > get the information out of it that I want. This time it failed because > > > it could not parse the .o file correctly. The tool has a dependency on a python library for parsing ELF; I've been having to teach it about various relocation types for non-x86_64 architectures; I'm sure the failure from that scenario is...gnarly. I don't know if my latest aarch64 fixes have been deployed (and it depends on how the library is distributed). > > > > Can you send me a config to reproduce the .o file? > > The one attached here should reproduce it on x86. Hmm...no warnings for me with that config on next-20201208: $ make LLVM=1 -j71 olddefconfig $ make LLVM=1 -j71 ... $ clang --version clang version 12.0.0 (g...@github.com:llvm/llvm-project.git 1c98f984105e552daa83ed8e92c61fba0e401410) (ie. near ToT LLVM) I see CONFIG_KASAN=y in the .config, so that's a recurring theme with clang; excessive stack usage. It does seem a shame to disable a driver for a bunch of archs just due to KASAN stack usage. $ grep KASAN=y 0x9077925C_defconfig CONFIG_HAVE_ARCH_KASAN=y CONFIG_KASAN=y Is there a different branch of a different tree that I should be testing on instead? Otherwise, can you send the object file? -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu: make DRM_AMD_DC x86-only again
On Mon, Dec 7, 2020 at 1:57 PM Arnd Bergmann wrote: > > On Mon, Dec 7, 2020 at 9:50 PM Christian König > wrote: > > Am 07.12.20 um 21:47 schrieb Alex Deucher: > > > On Fri, Dec 4, 2020 at 3:13 AM Arnd Bergmann wrote: > > >> From: Arnd Bergmann > > >> > > >> As the DRM_AMD_DC_DCN3_0 code was x86-only and fails to build on > > >> arm64, merging it into DRM_AMD_DC means that the top-level symbol > > >> is now x86-only as well. > > >> > > >> Compilation fails on arm64 with clang-12 with > > >> > > >> drivers/gpu/drm/amd/amdgpu/../display/dc/dml/dcn30/display_mode_vba_30.c:3641:6: > > >> error: stack frame size of 2416 bytes in function > > >> 'dml30_ModeSupportAndSystemConfigurationFull' > > >> [-Werror,-Wframe-larger-than=] > > >> void dml30_ModeSupportAndSystemConfigurationFull(struct display_mode_lib > > >> *mode_lib) > > >> > > >> I tried to see if the stack usage can be reduced, but this is code > > >> that is described as "This file is gcc-parsable HW gospel, coming > > >> straight from HW engineers." and is written in a way that is inherently > > >> nonportable and not meant to be understood by humans. > > >> > > >> There are probably no non-x86 users of this code, so simplify > > >> the dependency list accordingly. > > > + Daniel, Timothy > > > > > > Others contributed code to enable this on PPC64 and ARM64. > > > Unfortunately, we don't have these platforms to test with within AMD. > > > Does PPC64 have the same stack limitations as ARM64? Harry, Leo, can > > > you take a look at fixing the stack usage? > > > > This reminds me that I wanted to reply on this. > > > > 2416 is even to much on x86 if you add -Werror :) > > > > So this needs to be fixed anyway. > > Right, looking at my latest randconfig logs, I see the same problem on x86 > builds with clang as well, though I'm not entirely sure which other > configuration > options are needed to trigger it. > > So my patch can be disregarded, but I agree this needs a better fix, > either in clang or in the dcn driver. If you could give https://github.com/ClangBuiltLinux/frame-larger-than a spin again, I would appreciate any feedback. -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 17/17] mm: add mmu_notifier argument to follow_pfn
4:21: warning: performing pointer arithmetic
> > on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >writesb(PCI_IOBASE + addr, buffer, count);
> >~~ ^
> >include/asm-generic/io.h:643:21: warning: performing pointer arithmetic
> > on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >writesw(PCI_IOBASE + addr, buffer, count);
> >~~ ^
> >include/asm-generic/io.h:652:21: warning: performing pointer arithmetic
> > on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
> >writesl(PCI_IOBASE + addr, buffer, count);
> >~~ ^
> > >> arch/s390/kvm/../../../virt/kvm/kvm_main.c:1894:40: error: no member
> > >> named 'mmu_notifier' in 'struct kvm'
> >r = follow_pfn(vma, addr, , >mmu_notifier);
> > ~~~ ^
> >arch/s390/kvm/../../../virt/kvm/kvm_main.c:1909:41: error: no member
> > named 'mmu_notifier' in 'struct kvm'
> >r = follow_pfn(vma, addr, , >mmu_notifier);
> > ~~~ ^
> >20 warnings and 2 errors generated.
> >
> > vim +1894 arch/s390/kvm/../../../virt/kvm/kvm_main.c
> >
> > 1885
> > 1886static int hva_to_pfn_remapped(struct kvm *kvm, struct
> > vm_area_struct *vma,
> > 1887 unsigned long addr, bool
> > *async,
> > 1888 bool write_fault, bool
> > *writable,
> > 1889 kvm_pfn_t *p_pfn)
> > 1890{
> > 1891unsigned long pfn;
> > 1892int r;
> > 1893
> > > 1894r = follow_pfn(vma, addr, , >mmu_notifier);
> > 1895if (r) {
> > 1896/*
> > 1897 * get_user_pages fails for VM_IO and
> > VM_PFNMAP vmas and does
> > 1898 * not call the fault handler, so do it here.
> > 1899 */
> > 1900bool unlocked = false;
> > 1901r = fixup_user_fault(current->mm, addr,
> > 1902 (write_fault ?
> > FAULT_FLAG_WRITE : 0),
> > 1903 );
> > 1904if (unlocked)
> > 1905return -EAGAIN;
> > 1906if (r)
> > 1907return r;
> > 1908
> > 1909r = follow_pfn(vma, addr, ,
> > >mmu_notifier);
> > 1910if (r)
> > 1911return r;
> > 1912
> > 1913}
> > 1914
> > 1915if (writable)
> > 1916*writable = true;
> > 1917
> > 1918/*
> > 1919 * Get a reference here because callers of
> > *hva_to_pfn* and
> > 1920 * *gfn_to_pfn* ultimately call kvm_release_pfn_clean
> > on the
> > 1921 * returned pfn. This is only needed if the VMA has
> > VM_MIXEDMAP
> > 1922 * set, but the kvm_get_pfn/kvm_release_pfn_clean
> > pair will
> > 1923 * simply do nothing for reserved pfns.
> > 1924 *
> > 1925 * Whoever called remap_pfn_range is also going to
> > call e.g.
> > 1926 * unmap_mapping_range before the underlying pages
> > are freed,
> > 1927 * causing a call to our MMU notifier.
> > 1928 */
> > 1929kvm_get_pfn(pfn);
> > 1930
> > 1931*p_pfn = pfn;
> > 1932return 0;
> > 1933}
> > 1934
> >
> > ---
> > 0-DAY CI Kernel Test Service, Intel Corporation
> > https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>
> --
> You received this message because you are subscribed to the Google Groups
> "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to clang-built-linux+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/clang-built-linux/20201130142820.GN401619%40phenom.ffwll.local.
--
Thanks,
~Nick Desaulniers
___
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, 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. -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: drivers/gpu/drm/panel/panel-samsung-ld9040.c:240:12: warning: stack frame size of 8312 bytes in function 'ld9040_prepare'
On Sat, Jun 27, 2020 at 12:43 PM Vladimir Oltean wrote:
>
> Hi Nick,
>
> On Mon, 22 Jun 2020 at 19:50, Nick Desaulniers
> wrote:
> >
>
> > > I really don't get what's the problem here. The listing of
> > > ld9040_prepare at the given commit and with the given .config is:
> >
> > I wrote a tool to help debug these.
> > https://github.com/ClangBuiltLinux/frame-larger-than
> > If you fetch the randconfig and rebuild with debug info, that tool
> > will help show you which variables are used in which stack frames and
> > what their sizes are. Also note that strange things get dug up from
> > randconfigs.
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
>
> I ran your tool and it basically told me that all 11 calls to
Cool? No bugs running it? (I still need to extend support for many
architectures)
> ld9040_dcs_write from within ld9040_init are inlined by the compiler.
> Each of these ld9040_dcs_write functions calls ld9040_spi_write_word
> twice, so 22 inline calls to that. Now, sizeof(struct
> spi_transfer)=136 and sizeof(struct spi_message)=104, so, no wonder we
> run out of stack pretty quickly.
I'd expect these to have distinct lifetimes resulting in stack slot
reuse. When the compiler inlines functions, it introduces a lexical
scope. You can imagine it inlining the body, but within a new `{}`
delineated compound statement. Then the compiler knows that the
variables local to those scopes can't outlive each other, and can
reuse their stack slots in the containing function. Escape analysis
comes into play, too, but I'm not sure that's an issue here.
>
> But my question is: what's wrong with the code, if anything at all?
The general case we try to find+fix with this warning is excessively
large stack allocations that probably should be heap allocated,
percpu, or static. Also, the `noinline_for_stack` function annotation
is used frequently for this.
One known case of issues are the sanitizers, which can generally
prevent the reuse of stack slots. Were any of those set in this
config, since this was a randconfig? I'd check this first, then
consider if `noinline_for_stack` is appropriate on any of the related
functions.
> Why does the compiler try to inline it, and then complain that it's
> using too much stack
The flag -Wframe-larger-than= is a warning on semantics, not really an
optimization flag controlling the maximum stack depth of the function
being inlined into.
> when basically nobody appears to have asked it to
> inline it?
That's not really how inlining works. If you don't specify compiler
attributes, then the compiler can decide to inline or not at its
discretion. The `inline` keyword or its absence doesn't really affect
this. __attribute__((always_inline)) and __attribute__((noinline))
can give you more control, but there are hyper obscure edge cases
where even those don't work as advertised.
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] amdgpu: fix integer overflow on 32-bit architectures
See also
https://lore.kernel.org/lkml/cadnq5_ndtzh5_rgdwkj9c_42xlvrnccs5ddu1ysptfzp94k...@mail.gmail.com/T/#me707e09e92c6e487285e8bb382a607e4e782c249
On Tue, May 5, 2020 at 7:17 AM Christian König wrote:
>
> Am 05.05.20 um 16:15 schrieb Arnd Bergmann:
> > Multiplying 10 by four overruns a 'long' variable, as clang
> > points out:
> >
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:4160:53: error: overflow in
> > expression; result is -294967296 with type 'long'
> > [-Werror,-Winteger-overflow]
> > expires = ktime_get_mono_fast_ns() + NSEC_PER_SEC * 4L;
> >^
> > Make this a 'long long' constant instead.
> >
> > Fixes: 3f12acc8d6d4 ("drm/amdgpu: put the audio codec into suspend state
> > before gpu reset V3")
> > Signed-off-by: Arnd Bergmann
>
> Reviewed-by: Christian König
>
> > ---
> > I'm not sure the ktime_get_mono_fast_ns() call is necessary here
> > either. Is it intentional because ktime_get_ns() doesn't work
> > during a driver suspend, or just a mistake?
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 6f93af972b0a..2e07e3e6b036 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4157,7 +4157,7 @@ static int amdgpu_device_suspend_display_audio(struct
> > amdgpu_device *adev)
> >* the audio controller default autosuspend delay setting.
> >* 4S used here is guaranteed to cover that.
> >*/
> > - expires = ktime_get_mono_fast_ns() + NSEC_PER_SEC * 4L;
> > + expires = ktime_get_mono_fast_ns() + NSEC_PER_SEC * 4LL;
> >
> > while (!pm_runtime_status_suspended(&(p->dev))) {
> > if (!pm_runtime_suspend(&(p->dev)))
>
> --
> You received this message because you are subscribed to the Google Groups
> "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to clang-built-linux+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/clang-built-linux/e4a852b2-807b-bc73-7328-bcc399341085%40amd.com.
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/2] powerpc: Remove Xilinx PPC405/PPC440 support
On Mon, Mar 30, 2020 at 6:32 AM Michal Simek wrote:
>
> The latest Xilinx design tools called ISE and EDK has been released in
> October 2013. New tool doesn't support any PPC405/PPC440 new designs.
> These platforms are no longer supported and tested.
>
> PowerPC 405/440 port is orphan from 2013 by
> commit cdeb89943bfc ("MAINTAINERS: Fix incorrect status tag") and
> commit 19624236cce1 ("MAINTAINERS: Update Grant's email address and
> maintainership")
> that's why it is time to remove the support fot these platforms.
>
> Signed-off-by: Michal Simek
> Acked-by: Arnd Bergmann
Is this per chance related to:
https://lore.kernel.org/linux-next/1e0a9c45-e525-a3ac-b352-e236d8427...@xilinx.com/
We just hit that error today without our CI on ppc32 builds:
https://travis-ci.com/github/ClangBuiltLinux/continuous-integration/builds/157031633
> ---
>
> Changes in v2:
> - Based on my chat with Arnd I removed arch/powerpc/xmon/ changes done in
> v1 to keep them the same as before. (kbuild reported some issues with it
> too)
>
> Documentation/devicetree/bindings/xilinx.txt | 143 --
> Documentation/powerpc/bootwrapper.rst| 28 +-
> MAINTAINERS | 6 -
> arch/powerpc/Kconfig.debug | 2 +-
> arch/powerpc/boot/Makefile | 7 +-
> arch/powerpc/boot/dts/Makefile | 1 -
> arch/powerpc/boot/dts/virtex440-ml507.dts| 406
> arch/powerpc/boot/dts/virtex440-ml510.dts| 466 ---
> arch/powerpc/boot/ops.h | 1 -
> arch/powerpc/boot/serial.c | 5 -
> arch/powerpc/boot/uartlite.c | 79
> arch/powerpc/boot/virtex.c | 97
> arch/powerpc/boot/virtex405-head.S | 31 --
> arch/powerpc/boot/wrapper| 8 -
> arch/powerpc/configs/40x/virtex_defconfig| 75 ---
> arch/powerpc/configs/44x/virtex5_defconfig | 74 ---
> arch/powerpc/configs/ppc40x_defconfig| 8 -
> arch/powerpc/configs/ppc44x_defconfig| 8 -
> arch/powerpc/include/asm/xilinx_intc.h | 16 -
> arch/powerpc/include/asm/xilinx_pci.h| 21 -
> arch/powerpc/kernel/cputable.c | 39 --
> arch/powerpc/platforms/40x/Kconfig | 31 --
> arch/powerpc/platforms/40x/Makefile | 1 -
> arch/powerpc/platforms/40x/virtex.c | 54 ---
> arch/powerpc/platforms/44x/Kconfig | 37 --
> arch/powerpc/platforms/44x/Makefile | 2 -
> arch/powerpc/platforms/44x/virtex.c | 60 ---
> arch/powerpc/platforms/44x/virtex_ml510.c| 30 --
> arch/powerpc/platforms/Kconfig | 4 -
> arch/powerpc/sysdev/Makefile | 2 -
> arch/powerpc/sysdev/xilinx_intc.c| 88
> arch/powerpc/sysdev/xilinx_pci.c | 132 --
> drivers/char/Kconfig | 2 +-
> drivers/video/fbdev/Kconfig | 2 +-
> 34 files changed, 7 insertions(+), 1959 deletions(-)
> delete mode 100644 arch/powerpc/boot/dts/virtex440-ml507.dts
> delete mode 100644 arch/powerpc/boot/dts/virtex440-ml510.dts
> delete mode 100644 arch/powerpc/boot/uartlite.c
> delete mode 100644 arch/powerpc/boot/virtex.c
> delete mode 100644 arch/powerpc/boot/virtex405-head.S
> delete mode 100644 arch/powerpc/configs/40x/virtex_defconfig
> delete mode 100644 arch/powerpc/configs/44x/virtex5_defconfig
> delete mode 100644 arch/powerpc/include/asm/xilinx_intc.h
> delete mode 100644 arch/powerpc/include/asm/xilinx_pci.h
> delete mode 100644 arch/powerpc/platforms/40x/virtex.c
> delete mode 100644 arch/powerpc/platforms/44x/virtex.c
> delete mode 100644 arch/powerpc/platforms/44x/virtex_ml510.c
> delete mode 100644 arch/powerpc/sysdev/xilinx_intc.c
> delete mode 100644 arch/powerpc/sysdev/xilinx_pci.c
>
> diff --git a/Documentation/devicetree/bindings/xilinx.txt
> b/Documentation/devicetree/bindings/xilinx.txt
> index d058ace29345..28199b31fe5e 100644
> --- a/Documentation/devicetree/bindings/xilinx.txt
> +++ b/Documentation/devicetree/bindings/xilinx.txt
> @@ -86,149 +86,6 @@
> xlnx,use-parity = <0>;
> };
>
> - Some IP cores actually implement 2 or more logical devices. In
> - this case, the device should still describe the whole IP core with
> - a single node and add a child node for each logical device. The
> - ranges property can be used to translate from parent IP-core to the
> - registers of each device. In addition, the parent node should be
> - compatible with the bus type 'xlnx,compound', and should contain
> - #address-cells and #size-cells, as with any other bus. (Note: this
> - makes the assumption that both logical devices have the same bus
> - binding. If this is not true, then separate nodes should be used
> - for each logical device). The 'cell-index' property can be used to
> - enumerate logical devices within an
Re: [PATCH] drm/i915: Cast remain to unsigned long in eb_relocate_vma
On Fri, Feb 14, 2020 at 7:36 AM Michel Dänzer wrote: > > On 2020-02-14 12:49 p.m., Jani Nikula wrote: > > On Fri, 14 Feb 2020, Chris Wilson wrote: > >> Quoting Jani Nikula (2020-02-14 06:36:15) > >>> On Thu, 13 Feb 2020, Nathan Chancellor wrote: > >>>> A recent commit in clang added -Wtautological-compare to -Wall, which is > >>>> enabled for i915 after -Wtautological-compare is disabled for the rest > >>>> of the kernel so we see the following warning on x86_64: > >>>> > >>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1433:22: warning: > >>>> result of comparison of constant 576460752303423487 with expression of > >>>> type 'unsigned int' is always false > >>>> [-Wtautological-constant-out-of-range-compare] > >>>> if (unlikely(remain > N_RELOC(ULONG_MAX))) > >>>> ^ > >>>> ../include/linux/compiler.h:78:42: note: expanded from macro 'unlikely' > >>>> # define unlikely(x)__builtin_expect(!!(x), 0) > >>>> ^ > >>>> 1 warning generated. > >>>> > >>>> It is not wrong in the case where ULONG_MAX > UINT_MAX but it does not > >>>> account for the case where this file is built for 32-bit x86, where > >>>> ULONG_MAX == UINT_MAX and this check is still relevant. > >>>> > >>>> Cast remain to unsigned long, which keeps the generated code the same > >>>> (verified with clang-11 on x86_64 and GCC 9.2.0 on x86 and x86_64) and > >>>> the warning is silenced so we can catch more potential issues in the > >>>> future. > >>>> > >>>> Link: https://github.com/ClangBuiltLinux/linux/issues/778 > >>>> Suggested-by: Michel Dänzer > >>>> Signed-off-by: Nathan Chancellor > >>> > >>> Works for me as a workaround, > >> > >> But the whole point was that the compiler could see that it was > >> impossible and not emit the code. Doesn't this break that? > > > > It seems that goal and the warning are fundamentally incompatible. > > Not really: > > if (sizeof(remain) >= sizeof(unsigned long) && > unlikely(remain > N_RELOC(ULONG_MAX))) > return -EINVAL; > > In contrast to the cast, this doesn't generate any machine code on 64-bit: > > https://godbolt.org/z/GmUE4S > > but still generates the same code on 32-bit: > > https://godbolt.org/z/hAoz8L Exactly. This check is only a tautology when `sizeof(long) == sizeof(int)` (ie. ILP32 platforms, like 32b x86), notice how BOTH GCC AND Clang generate exactly the same code: https://godbolt.org/z/6ShrDM Both compilers eliminate the check when `-m32` is not set, and generate the exact same check otherwise. How about: ``` diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index d3f4f28e9468..25b9d3f3ad57 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1415,8 +1415,10 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct eb_vma *ev) urelocs = u64_to_user_ptr(entry->relocs_ptr); remain = entry->relocation_count; +#ifndef CONFIG_64BIT if (unlikely(remain > N_RELOC(ULONG_MAX))) return -EINVAL; +#endif /* * We must check that the entire relocation array is safe ``` We now have 4 proposed solutions: 1. https://lore.kernel.org/lkml/20191123195321.41305-1-natechancel...@gmail.com/ 2. https://lore.kernel.org/lkml/20200211050808.29463-1-natechancel...@gmail.com/ 3. https://lore.kernel.org/lkml/20200214054706.33870-1-natechancel...@gmail.com/ 4. my diff above Let's please come to a resolution on this. -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/edid: Distribute switch variables for initialization
On Fri, Mar 6, 2020 at 9:36 AM Nick Desaulniers wrote:
>
> On Fri, Mar 6, 2020 at 9:32 AM Kees Cook wrote:
> >
> > Variables declared in a switch statement before any case statements
> > cannot be automatically initialized with compiler instrumentation (as
> > they are not part of any execution flow). With GCC's proposed automatic
> > stack variable initialization feature, this triggers a warning (and they
> > don't get initialized). Clang's automatic stack variable initialization
> > (via CONFIG_INIT_STACK_ALL=y) doesn't throw a warning, but it also
> > doesn't initialize such variables[1]. Note that these warnings (or silent
>
> That's not good, have you filed a bug against Clang yet? It should at
> least warn when the corresponding stack init flag is set.
D'oh, link is below.
>
> > skipping) happen before the dead-store elimination optimization phase,
> > so even when the automatic initializations are later elided in favor of
> > direct initializations, the warnings remain.
> >
> > To avoid these problems, lift such variables up into the next code
> > block.
> >
> > drivers/gpu/drm/drm_edid.c: In function ‘drm_edid_to_eld’:
> > drivers/gpu/drm/drm_edid.c:4395:9: warning: statement will never be
> > executed [-Wswitch-unreachable]
> > 4395 | int sad_count;
> > | ^
> >
> > [1] https://bugs.llvm.org/show_bug.cgi?id=44916
> >
> > Signed-off-by: Kees Cook
>
> Reviewed-by: Nick Desaulniers
>
> > ---
> > v2: move into function block instead being switch-local (Ville Syrjälä)
> > ---
> > drivers/gpu/drm/drm_edid.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 805fb004c8eb..46cee78bc175 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4381,6 +4381,7 @@ static void drm_edid_to_eld(struct drm_connector
> > *connector, struct edid *edid)
> >
> > if (cea_revision(cea) >= 3) {
> > int i, start, end;
> > + int sad_count;
> >
> > if (cea_db_offsets(cea, , )) {
> > start = 0;
> > @@ -4392,8 +4393,6 @@ static void drm_edid_to_eld(struct drm_connector
> > *connector, struct edid *edid)
> > dbl = cea_db_payload_len(db);
> >
> > switch (cea_db_tag(db)) {
> > - int sad_count;
> > -
> > case AUDIO_BLOCK:
> > /* Audio Data Block, contains SADs */
> > sad_count = min(dbl / 3, 15 -
> > total_sad_count);
> > --
> > 2.20.1
> >
> >
> > --
> > Kees Cook
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Clang Built Linux" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to clang-built-linux+unsubscr...@googlegroups.com.
> > To view this discussion on the web visit
> > https://groups.google.com/d/msgid/clang-built-linux/202003060930.DDCCB6659%40keescook.
>
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/edid: Distribute switch variables for initialization
On Fri, Mar 6, 2020 at 9:32 AM Kees Cook wrote:
>
> Variables declared in a switch statement before any case statements
> cannot be automatically initialized with compiler instrumentation (as
> they are not part of any execution flow). With GCC's proposed automatic
> stack variable initialization feature, this triggers a warning (and they
> don't get initialized). Clang's automatic stack variable initialization
> (via CONFIG_INIT_STACK_ALL=y) doesn't throw a warning, but it also
> doesn't initialize such variables[1]. Note that these warnings (or silent
That's not good, have you filed a bug against Clang yet? It should at
least warn when the corresponding stack init flag is set.
> skipping) happen before the dead-store elimination optimization phase,
> so even when the automatic initializations are later elided in favor of
> direct initializations, the warnings remain.
>
> To avoid these problems, lift such variables up into the next code
> block.
>
> drivers/gpu/drm/drm_edid.c: In function ‘drm_edid_to_eld’:
> drivers/gpu/drm/drm_edid.c:4395:9: warning: statement will never be
> executed [-Wswitch-unreachable]
> 4395 | int sad_count;
> | ^
>
> [1] https://bugs.llvm.org/show_bug.cgi?id=44916
>
> Signed-off-by: Kees Cook
Reviewed-by: Nick Desaulniers
> ---
> v2: move into function block instead being switch-local (Ville Syrjälä)
> ---
> drivers/gpu/drm/drm_edid.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 805fb004c8eb..46cee78bc175 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4381,6 +4381,7 @@ static void drm_edid_to_eld(struct drm_connector
> *connector, struct edid *edid)
>
> if (cea_revision(cea) >= 3) {
> int i, start, end;
> + int sad_count;
>
> if (cea_db_offsets(cea, , )) {
> start = 0;
> @@ -4392,8 +4393,6 @@ static void drm_edid_to_eld(struct drm_connector
> *connector, struct edid *edid)
> dbl = cea_db_payload_len(db);
>
> switch (cea_db_tag(db)) {
> - int sad_count;
> -
> case AUDIO_BLOCK:
> /* Audio Data Block, contains SADs */
> sad_count = min(dbl / 3, 15 -
> total_sad_count);
> --
> 2.20.1
>
>
> --
> Kees Cook
>
> --
> You received this message because you are subscribed to the Google Groups
> "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to clang-built-linux+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/clang-built-linux/202003060930.DDCCB6659%40keescook.
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/i915: Disable -Wtautological-constant-out-of-range-compare
On Wed, Feb 12, 2020 at 9:17 AM Michel Dänzer wrote: > > On 2020-02-12 6:07 p.m., Nathan Chancellor wrote: > > On Wed, Feb 12, 2020 at 09:52:52AM +0100, Michel Dänzer wrote: > >> On 2020-02-11 9:39 p.m., Nathan Chancellor wrote: > >>> On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote: > >>>> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote: > >>>>> A recent commit in clang added -Wtautological-compare to -Wall, which is > >>>>> enabled for i915 so we see the following warning: > >>>>> > >>>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > >>>>> result of comparison of constant 576460752303423487 with expression of > >>>>> type 'unsigned int' is always false > >>>>> [-Wtautological-constant-out-of-range-compare] > >>>>> if (unlikely(remain > N_RELOC(ULONG_MAX))) > >>>>> ^ > >>>>> > >>>>> This warning only happens on x86_64 but that check is relevant for > >>>>> 32-bit x86 so we cannot remove it. > >>>> > >>>> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value > >>>> in both cases, and remain is a 32-bit value in both cases. How can it be > >>>> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)? > >>>> > >>> > >>> Hi Michel, > >>> > >>> Can't this condition be true when UINT_MAX == ULONG_MAX? > >> > >> Oh, right, I think I was wrongly thinking long had 64 bits even on 32-bit. > >> > >> > >> Anyway, this suggests a possible better solution: > >> > >> #if UINT_MAX == ULONG_MAX > >> if (unlikely(remain > N_RELOC(ULONG_MAX))) > >> return -EINVAL; > >> #endif > >> > >> > >> Or if that can't be used for some reason, something like > >> > >> if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX))) > >> return -EINVAL; > >> > >> should silence the warning. > > > > I do like this one better than the former. > > FWIW, one downside of this one compared to all alternatives (presumably) > is that it might end up generating actual code even on 64-bit, which > always ends up skipping the return. The warning is pointing out that the conditional is always false, which is correct on 64b. The check is only active for 32b. https://godbolt.org/z/oQrgT_ The cast silences the warning for 64b. (Note that GCC and Clang also generate precisely the same instruction sequences in my example, just GCC doesn't warn on such tautologies). -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] fbmem: Adjust indentation in fb_prepare_logo and fb_blank
On Tue, Dec 17, 2019 at 7:00 PM Nathan Chancellor
wrote:
>
> Clang warns:
>
> ../drivers/video/fbdev/core/fbmem.c:665:3: warning: misleading
> indentation; statement is not part of the previous 'else'
> [-Wmisleading-indentation]
> if (fb_logo.depth > 4 && depth > 4) {
> ^
> ../drivers/video/fbdev/core/fbmem.c:661:2: note: previous statement is
> here
> else
> ^
> ../drivers/video/fbdev/core/fbmem.c:1075:3: warning: misleading
> indentation; statement is not part of the previous 'if'
> [-Wmisleading-indentation]
> return ret;
> ^
> ../drivers/video/fbdev/core/fbmem.c:1072:2: note: previous statement is
> here
> if (!ret)
> ^
> 2 warnings generated.
>
> This warning occurs because there are spaces before the tabs on these
> lines. Normalize the indentation in these functions so that it is
> consistent with the Linux kernel coding style and clang no longer warns.
>
> Fixes: 1692b37c99d5 ("fbdev: Fix logo if logo depth is less than framebuffer
> depth")
> Link: https://github.com/ClangBuiltLinux/linux/issues/825
> Signed-off-by: Nathan Chancellor
Thanks for the patch!
Reviewed-by: Nick Desaulniers
> ---
> drivers/video/fbdev/core/fbmem.c | 36
> 1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c
> b/drivers/video/fbdev/core/fbmem.c
> index 0662b61fdb50..bf63cc0e6b65 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -662,20 +662,20 @@ int fb_prepare_logo(struct fb_info *info, int rotate)
> fb_logo.depth = 1;
>
>
> - if (fb_logo.depth > 4 && depth > 4) {
> - switch (info->fix.visual) {
> - case FB_VISUAL_TRUECOLOR:
> - fb_logo.needs_truepalette = 1;
> - break;
> - case FB_VISUAL_DIRECTCOLOR:
> - fb_logo.needs_directpalette = 1;
> - fb_logo.needs_cmapreset = 1;
> - break;
> - case FB_VISUAL_PSEUDOCOLOR:
> - fb_logo.needs_cmapreset = 1;
> - break;
> - }
> - }
> + if (fb_logo.depth > 4 && depth > 4) {
> + switch (info->fix.visual) {
> + case FB_VISUAL_TRUECOLOR:
> + fb_logo.needs_truepalette = 1;
> + break;
> + case FB_VISUAL_DIRECTCOLOR:
> + fb_logo.needs_directpalette = 1;
> + fb_logo.needs_cmapreset = 1;
> + break;
> + case FB_VISUAL_PSEUDOCOLOR:
> + fb_logo.needs_cmapreset = 1;
> + break;
> + }
> + }
>
> height = fb_logo.logo->height;
> if (fb_center_logo)
> @@ -1060,19 +1060,19 @@ fb_blank(struct fb_info *info, int blank)
> struct fb_event event;
> int ret = -EINVAL;
>
> - if (blank > FB_BLANK_POWERDOWN)
> - blank = FB_BLANK_POWERDOWN;
> + if (blank > FB_BLANK_POWERDOWN)
> + blank = FB_BLANK_POWERDOWN;
>
> event.info = info;
> event.data =
>
> if (info->fbops->fb_blank)
> - ret = info->fbops->fb_blank(blank, info);
> + ret = info->fbops->fb_blank(blank, info);
>
> if (!ret)
> fb_notifier_call_chain(FB_EVENT_BLANK, );
>
> - return ret;
> + return ret;
> }
> EXPORT_SYMBOL(fb_blank);
>
> --
> 2.24.1
>
> --
> You received this message because you are subscribed to the Google Groups
> "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to clang-built-linux+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/clang-built-linux/20191218030025.10064-1-natechancellor%40gmail.com.
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: Remove tautological compare in eb_relocate_vma
On Tue, Dec 3, 2019 at 5:42 AM Chris Wilson wrote: > > Quoting Nick Desaulniers (2019-12-02 19:18:20) > > On Sat, Nov 23, 2019 at 12:05 PM Chris Wilson > > wrote: > > > > > > Quoting Nathan Chancellor (2019-11-23 19:53:22) > > > > -Wtautological-compare was recently added to -Wall in LLVM, which > > > > exposed an if statement in i915 that is always false: > > > > > > > > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > > > > result of comparison of constant 576460752303423487 with expression of > > > > type 'unsigned int' is always false > > > > [-Wtautological-constant-out-of-range-compare] > > > > if (unlikely(remain > N_RELOC(ULONG_MAX))) > > > > ^ > > > > > > > > Since remain is an unsigned int, it can never be larger than UINT_MAX, > > > > which is less than ULONG_MAX / sizeof(struct > > > > drm_i915_gem_relocation_entry). > > > > Remove this statement to fix the warning. > > > > > > The check should remain as we do want to document the overflow > > > calculation, and it should represent the types used -- it's much easier > > > > What do you mean "represent the types used?" Are you concerned that > > the type of drm_i915_gem_exec_object2->relocation_count might change > > in the future? > > We may want to change the restriction, yes. > > > > to review a stub than trying to find a missing overflow check. If the > > > overflow cannot happen as the types are wide enough, no problem, the > > > compiler can remove the known false branch. > > > > What overflow are you trying to protect against here? > > These values are under user control, our validation steps should be > clear and easy to check. If we have the types wrong, if the checks are > wrong, we need to fix them. If the code is removed because it can be > evaluated by the compiler to be redundant, it is much harder for us to > verify that we have tried to validate user input. > > > > Tautology here has a purpose for conveying information to the reader. > > > > Well leaving a warning unaddressed is also not a solution. Either > > replace it with a comment or turn off the warning for your subdir. > > My personal preference would be to use a bunch of central macros for the > various type/kmalloc overflows, and have the warnings suppressed there > since they are very much about documenting user input validation. > -Chris Is kmalloc_array what you're looking for? Looks like it has the `check_mul_overflow` call in it. -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: Remove tautological compare in eb_relocate_vma
On Sat, Nov 23, 2019 at 12:05 PM Chris Wilson wrote: > > Quoting Nathan Chancellor (2019-11-23 19:53:22) > > -Wtautological-compare was recently added to -Wall in LLVM, which > > exposed an if statement in i915 that is always false: > > > > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning: > > result of comparison of constant 576460752303423487 with expression of > > type 'unsigned int' is always false > > [-Wtautological-constant-out-of-range-compare] > > if (unlikely(remain > N_RELOC(ULONG_MAX))) > > ^ > > > > Since remain is an unsigned int, it can never be larger than UINT_MAX, > > which is less than ULONG_MAX / sizeof(struct drm_i915_gem_relocation_entry). > > Remove this statement to fix the warning. > > The check should remain as we do want to document the overflow > calculation, and it should represent the types used -- it's much easier What do you mean "represent the types used?" Are you concerned that the type of drm_i915_gem_exec_object2->relocation_count might change in the future? > to review a stub than trying to find a missing overflow check. If the > overflow cannot happen as the types are wide enough, no problem, the > compiler can remove the known false branch. What overflow are you trying to protect against here? > > Tautology here has a purpose for conveying information to the reader. Well leaving a warning unaddressed is also not a solution. Either replace it with a comment or turn off the warning for your subdir. The warning here looks valid to me; you have a guard for something that's impossible. -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/6] drm/amd/display: fix dcn21 Makefile for clang
On Wed, Oct 2, 2019 at 2:24 PM Alex Deucher wrote: > > On Wed, Oct 2, 2019 at 5:19 PM Nick Desaulniers > wrote: > > > > Alex, do you know why the AMDGPU driver uses a different stack > > alignment (16B) than the rest of the x86 kernel? (see > > arch/x86/Makefile which uses 8B stack alignment). > > Not sure. Maybe Harry can comment. I think it was added for the > floating point stuff. Not sure if it's strictly required or not. Can you find out for me please who knows more about this and setup a chat with all of us? (I don't want to deride this patch's review thread, so let's start a new thread once we know more) We're facing some interesting runtime issues when built with Clang. -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/6] drm/amd/display: fix dcn21 Makefile for clang
On Wed, Oct 2, 2019 at 5:03 AM Arnd Bergmann wrote:
>
> Just like all the other variants, this one passes invalid
> compile-time options with clang after the new code got
> merged:
>
> clang: error: unknown argument: '-mpreferred-stack-boundary=4'
> scripts/Makefile.build:265: recipe for target
> 'drivers/gpu/drm/amd/amdgpu/../display/dc/dcn21/dcn21_resource.o' failed
>
> Use the same variant that we have for dcn20 to fix compilation.
>
> Fixes: eced51f9babb ("drm/amd/display: Add hubp block for Renoir (v2)")
> Signed-off-by: Arnd Bergmann
Thanks for the patch!
Reviewed-by: Nick Desaulniers
Tested-by: Nick Desaulniers
(Though I think it's already been merged)
Alex, do you know why the AMDGPU driver uses a different stack
alignment (16B) than the rest of the x86 kernel? (see
arch/x86/Makefile which uses 8B stack alignment).
> ---
> drivers/gpu/drm/amd/display/dc/dcn21/Makefile | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
> b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
> index 8cd9de8b1a7a..ef673bffc241 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dcn21/Makefile
> @@ -3,7 +3,17 @@
>
> DCN21 = dcn21_hubp.o dcn21_hubbub.o dcn21_resource.o
>
> -CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o := -mhard-float -msse
> -mpreferred-stack-boundary=4
> +ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
> + cc_stack_align := -mpreferred-stack-boundary=4
> +else ifneq ($(call cc-option, -mstack-alignment=16),)
> + cc_stack_align := -mstack-alignment=16
> +endif
> +
> +CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o := -mhard-float -msse
> $(cc_stack_align)
> +
> +ifdef CONFIG_CC_IS_CLANG
> +CFLAGS_$(AMDDALPATH)/dc/dcn21/dcn21_resource.o += -msse2
> +endif
>
> AMD_DAL_DCN21 = $(addprefix $(AMDDALPATH)/dc/dcn21/,$(DCN21))
>
> --
> 2.20.0
>
> --
> You received this message because you are subscribed to the Google Groups
> "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to clang-built-linux+unsubscr...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/clang-built-linux/20191002120136.1777161-5-arnd%40arndb.de.
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 6/6] [RESEND] drm/amdgpu: work around llvm bug #42576
On Wed, Oct 2, 2019 at 10:07 AM Nathan Chancellor wrote: > > On Wed, Oct 02, 2019 at 09:51:37AM -0700, 'Nick Desaulniers' via Clang Built > Linux wrote: > > > Apparently this bug is still present in both the released clang-9 > > > and the current development version of clang-10. > > > I was hoping we would not need a workaround in clang-9+, but > > > it seems that we do. Here's a fix: https://reviews.llvm.org/D68356 Can't possibly land until clang-10 though. > > > > I think I'd rather: > > 1. mark AMDGPU BROKEN if CC_IS_CLANG. There are numerous other issues > > building > >a working driver here. > > The only reason I am not thrilled about this is we will lose out on > warning coverage while the compiler bug gets fixed. I think the AMDGPU > drivers have been the single biggest source of clang warnings. > > I think something like: > > depends on CC_IS_GCC || (CC_IS_CLANG && COMPILE_TEST) > > would end up avoiding the runtime issues and give us warning coverage. > The only issue is that we would still need this patch... > > Cheers, > Nathan -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 6/6] [RESEND] drm/amdgpu: work around llvm bug #42576
> Apparently this bug is still present in both the released clang-9 > and the current development version of clang-10. > I was hoping we would not need a workaround in clang-9+, but > it seems that we do. I think I'd rather: 1. mark AMDGPU BROKEN if CC_IS_CLANG. There are numerous other issues building a working driver here. 2. Fix the compiler bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: linux-next: manual merge of the drm tree with the kbuild tree
On Sun, Sep 15, 2019 at 2:47 PM Mark Brown wrote:
>
> Hi all,
>
> Today's linux-next merge of the drm tree got a conflict in:
>
> drivers/gpu/drm/amd/display/dc/dml/Makefile
>
> between commit:
>
> 54b8ae66ae1a345 ("kbuild: change *FLAGS_.o to take the path
> relative to $(obj)")
>
> from the kbuild tree and commits:
>
> 0f0727d971f6fdf ("drm/amd/display: readd -msse2 to prevent Clang from
> emitting libcalls to undefined SW FP routines")
^ this patch is now broken due to the SHA above it.
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
index 2b399cfa72e6..ddb8d5649e79 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
@@ -19,7 +19,7 @@ endif
CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o := -mhard-float -msse
$(cc_stack_align)
ifdef CONFIG_CC_IS_CLANG
-CFLAGS_dcn20_resource.o += -msse2
+CFLAGS_$(AMDDALPATH)/dc/dcn20/dcn20_resource.o += -msse2
endif
AMD_DAL_DCN20 = $(addprefix $(AMDDALPATH)/dc/dcn20/,$(DCN20))
> 542816ff168d8a3 ("drm/amd/display: Add DCN2.1 changes to DML")
> b04641a3f4c54b0 ("drm/amd/display: Add Renoir DML")
> 057fc695e934a77 ("drm/amd/display: support "dummy pstate"")
>
> from the drm tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging. You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
>
> diff --cc drivers/gpu/drm/amd/display/dc/dml/Makefile
> index 83792e2c0f0e4,af2a864a6da05..0
> --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile
> @@@ -32,16 -32,29 +32,25 @@@ endi
>
> dml_ccflags := -mhard-float -msse $(cc_stack_align)
>
> + ifdef CONFIG_CC_IS_CLANG
> + dml_ccflags += -msse2
> + endif
> +
> -CFLAGS_display_mode_lib.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/display_mode_lib.o := $(dml_ccflags)
>
> ifdef CONFIG_DRM_AMD_DC_DCN2_0
> -CFLAGS_display_mode_vba.o := $(dml_ccflags)
> -CFLAGS_display_mode_vba_20.o := $(dml_ccflags)
> -CFLAGS_display_rq_dlg_calc_20.o := $(dml_ccflags)
> -CFLAGS_display_mode_vba_20v2.o := $(dml_ccflags)
> -CFLAGS_display_rq_dlg_calc_20v2.o := $(dml_ccflags)
> -endif
> +CFLAGS_$(AMDDALPATH)/dc/dml/display_mode_vba.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_mode_vba_20.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_rq_dlg_calc_20.o := $(dml_ccflags)
> ++CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_mode_vba_20v2.o := $(dml_ccflags)
> ++CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_rq_dlg_calc_20v2.o :=
> $(dml_ccflags)
> + ifdef CONFIG_DRM_AMD_DC_DCN2_1
> -CFLAGS_display_mode_vba_21.o := $(dml_ccflags)
> -CFLAGS_display_rq_dlg_calc_21.o := $(dml_ccflags)
> -endif
^ this endif should not be removed.
> -ifdef CONFIG_DRM_AMD_DCN3AG
> -CFLAGS_display_mode_vba_3ag.o := $(dml_ccflags)
> ++CFLAGS_$(AMDDALPATH)/dc/dml/dcn21/display_mode_vba_21.o := $(dml_ccflags)
> ++CFLAGS_$(AMDDALPATH)/dc/dml/dcn21/display_rq_dlg_calc_21.o := $(dml_ccflags)
> endif
> -CFLAGS_dml1_display_rq_dlg_calc.o := $(dml_ccflags)
> -CFLAGS_display_rq_dlg_helpers.o := $(dml_ccflags)
> -CFLAGS_dml_common_defs.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/dml1_display_rq_dlg_calc.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/display_rq_dlg_helpers.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/dml_common_defs.o := $(dml_ccflags)
>
> DML = display_mode_lib.o display_rq_dlg_helpers.o
> dml1_display_rq_dlg_calc.o \
> dml_common_defs.o
--
Thanks,
~Nick Desaulniers
Re: linux-next: manual merge of the drm tree with the kbuild tree
On Tue, Sep 3, 2019 at 11:46 PM Stephen Rothwell wrote:
>
> Hi all,
>
> Today's linux-next merge of the drm tree got conflicts in:
>
> drivers/gpu/drm/amd/display/dc/calcs/Makefile
> drivers/gpu/drm/amd/display/dc/dml/Makefile
> drivers/gpu/drm/amd/display/dc/dsc/Makefile
>
> between commit:
>
> 30851871d5ab ("kbuild: change *FLAGS_.o to take the path
> relative to $(obj)")
>
> from the kbuild tree and commit:
>
> 0f0727d971f6 ("drm/amd/display: readd -msse2 to prevent Clang from emitting
> libcalls to undefined SW FP routines")
>
> from the drm tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging. You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
My changes LGTM, thanks!
>
> --
> Cheers,
> Stephen Rothwell
>
> diff --cc drivers/gpu/drm/amd/display/dc/calcs/Makefile
> index d930df63772c,16614d73a5fc..
> --- a/drivers/gpu/drm/amd/display/dc/calcs/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/calcs/Makefile
> @@@ -32,9 -32,13 +32,13 @@@ endi
>
> calcs_ccflags := -mhard-float -msse $(cc_stack_align)
>
> + ifdef CONFIG_CC_IS_CLANG
> + calcs_ccflags += -msse2
> + endif
> +
> -CFLAGS_dcn_calcs.o := $(calcs_ccflags)
> -CFLAGS_dcn_calc_auto.o := $(calcs_ccflags)
> -CFLAGS_dcn_calc_math.o := $(calcs_ccflags) -Wno-tautological-compare
> +CFLAGS_$(AMDDALPATH)/dc/calcs/dcn_calcs.o := $(calcs_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/calcs/dcn_calc_auto.o := $(calcs_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/calcs/dcn_calc_math.o := $(calcs_ccflags)
> -Wno-tautological-compare
>
> BW_CALCS = dce_calcs.o bw_fixed.o custom_float.o
>
> diff --cc drivers/gpu/drm/amd/display/dc/dml/Makefile
> index 83792e2c0f0e,95fd2beca80c..
> --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile
> @@@ -32,16 -32,25 +32,20 @@@ endi
>
> dml_ccflags := -mhard-float -msse $(cc_stack_align)
>
> + ifdef CONFIG_CC_IS_CLANG
> + dml_ccflags += -msse2
> + endif
> +
> -CFLAGS_display_mode_lib.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/display_mode_lib.o := $(dml_ccflags)
>
> ifdef CONFIG_DRM_AMD_DC_DCN2_0
> -CFLAGS_display_mode_vba.o := $(dml_ccflags)
> -CFLAGS_display_mode_vba_20.o := $(dml_ccflags)
> -CFLAGS_display_rq_dlg_calc_20.o := $(dml_ccflags)
> -CFLAGS_display_mode_vba_20v2.o := $(dml_ccflags)
> -CFLAGS_display_rq_dlg_calc_20v2.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/display_mode_vba.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_mode_vba_20.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/dcn20/display_rq_dlg_calc_20.o := $(dml_ccflags)
> endif
> -ifdef CONFIG_DRM_AMD_DCN3AG
> -CFLAGS_display_mode_vba_3ag.o := $(dml_ccflags)
> -endif
> -CFLAGS_dml1_display_rq_dlg_calc.o := $(dml_ccflags)
> -CFLAGS_display_rq_dlg_helpers.o := $(dml_ccflags)
> -CFLAGS_dml_common_defs.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/dml1_display_rq_dlg_calc.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/display_rq_dlg_helpers.o := $(dml_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dml/dml_common_defs.o := $(dml_ccflags)
>
> DML = display_mode_lib.o display_rq_dlg_helpers.o
> dml1_display_rq_dlg_calc.o \
> dml_common_defs.o
> diff --cc drivers/gpu/drm/amd/display/dc/dsc/Makefile
> index c3922d6e7696,17db603f2d1f..
> --- a/drivers/gpu/drm/amd/display/dc/dsc/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dsc/Makefile
> @@@ -9,9 -9,14 +9,13 @@@ endi
>
> dsc_ccflags := -mhard-float -msse $(cc_stack_align)
>
> + ifdef CONFIG_CC_IS_CLANG
> + dsc_ccflags += -msse2
> + endif
> +
> -CFLAGS_rc_calc.o := $(dsc_ccflags)
> -CFLAGS_rc_calc_dpi.o := $(dsc_ccflags)
> -CFLAGS_codec_main_amd.o := $(dsc_ccflags)
> -CFLAGS_dc_dsc.o := $(dsc_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dsc/rc_calc.o := $(dsc_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dsc/rc_calc_dpi.o := $(dsc_ccflags)
> +CFLAGS_$(AMDDALPATH)/dc/dsc/dc_dsc.o := $(dsc_ccflags)
>
> DSC = dc_dsc.o rc_calc.o rc_calc_dpi.o
>
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: Remove redundant user_access_end() from __copy_from_user() error path
On Tue, Aug 6, 2019 at 5:59 AM Josh Poimboeuf wrote:
>
> On Mon, Aug 05, 2019 at 09:29:53PM +0200, Sedat Dilek wrote:
> > On Wed, Jul 31, 2019 at 2:25 PM Sedat Dilek wrote:
> > >
> > > On Fri, Jul 26, 2019 at 9:30 PM Chris Wilson
> > > wrote:
> > > >
> > > > Quoting Thomas Gleixner (2019-07-26 20:18:32)
> > > > > On Fri, 26 Jul 2019, Chris Wilson wrote:
> > > > > > Quoting Thomas Gleixner (2019-07-25 22:55:45)
> > > > > > > On Thu, 25 Jul 2019, Josh Poimboeuf wrote:
> > > > > > >
> > > > > > > > Objtool reports:
> > > > > > > >
> > > > > > > > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.o: warning:
> > > > > > > > objtool: .altinstr_replacement+0x36: redundant UACCESS disable
> > > > > > > >
> > > > > > > > __copy_from_user() already does both STAC and CLAC, so the
> > > > > > > > user_access_end() in its error path adds an extra unnecessary
> > > > > > > > CLAC.
> > > > > > > >
> > > > > > > > Fixes: 0b2c8f8b6b0c ("i915: fix missing user_access_end() in
> > > > > > > > page fault exception case")
> > > > > > > > Reported-by: Thomas Gleixner
> > > > > > > > Reported-by: Sedat Dilek
> > > > > > > > Acked-by: Peter Zijlstra (Intel)
> > > > > > > > Tested-by: Nick Desaulniers
> > > > > > > > Tested-by: Sedat Dilek
> > > > > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/617
> > > > > > > > Signed-off-by: Josh Poimboeuf
> > > > > > >
> > > > > > > Reviewed-by: Thomas Gleixner
> > > > > >
> > > > > > Which tree do you plan to apply it to? I can put in drm-intel, and
> > > > > > with
> > > > > > the fixes tag it will percolate through to 5.3 and beyond, but if
> > > > > > you
> > > > > > want to apply it directly to squash the build warnings, feel free.
> > > > >
> > > > > It would be nice to get it into 5.3. I can route it linuxwards if you
> > > > > give
> > > > > an Acked-by, but I'm happy to hand it to you :)
> > > >
> > > > Acked-by: Chris Wilson
> > >
> > > Thomas did you take this through tip tree after Chris' ACK?
> > >
> >
> > Hi,
> >
> > Gentle ping...
> > Thomas and Chris: Will someone of you pick this up?
> > As "objtool: Improve UACCESS coverage" [1] went trough tip tree I
> > highly appreciate to do so with this one.
>
> I think Thomas has gone on holiday, so hopefully Chris can pick it up
> after all.
tglx just picked up 2 other patches of mine, bumping just in case he's
not picking up patches while on vacation. ;)
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: Remove redundant user_access_end() from __copy_from_user() error path
On Thu, Aug 8, 2019 at 1:22 PM Thomas Gleixner wrote: > > tglx just picked up 2 other patches of mine, bumping just in case he's > > not picking up patches while on vacation. ;) > > I'm only half on vacation :) > > So I can pick it up. Thanks, will send half margaritas. -- Thanks, ~Nick Desaulniers
[PATCH] drm/amd/display: readd -msse2 to prevent Clang from emitting libcalls to undefined SW FP routines
arch/x86/Makefile disables SSE and SSE2 for the whole kernel. The
AMDGPU drivers modified in this patch re-enable SSE but not SSE2. Turn
on SSE2 to support emitting double precision floating point instructions
rather than calls to non-existent (usually available from gcc_s or
compiler_rt) floating point helper routines for Clang.
This was originally landed in:
commit 10117450735c ("drm/amd/display: add -msse2 to prevent Clang from
emitting libcalls to undefined SW FP routines")
but reverted in:
commit 193392ed9f69 ("Revert "drm/amd/display: add -msse2 to prevent Clang from
emitting libcalls to undefined SW FP routines"")
due to bugreports from GCC builds. Add guards to only do so for Clang.
Link: https://bugs.freedesktop.org/show_bug.cgi?id=109487
Link: https://github.com/ClangBuiltLinux/linux/issues/327
Suggested-by: Sedat Dilek
Suggested-by: Sami Tolvanen
Signed-off-by: Nick Desaulniers
---
drivers/gpu/drm/amd/display/dc/calcs/Makefile | 4
drivers/gpu/drm/amd/display/dc/dcn20/Makefile | 4
drivers/gpu/drm/amd/display/dc/dml/Makefile | 4
drivers/gpu/drm/amd/display/dc/dsc/Makefile | 4
4 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/amd/display/dc/calcs/Makefile
b/drivers/gpu/drm/amd/display/dc/calcs/Makefile
index 95f332ee3e7e..16614d73a5fc 100644
--- a/drivers/gpu/drm/amd/display/dc/calcs/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/calcs/Makefile
@@ -32,6 +32,10 @@ endif
calcs_ccflags := -mhard-float -msse $(cc_stack_align)
+ifdef CONFIG_CC_IS_CLANG
+calcs_ccflags += -msse2
+endif
+
CFLAGS_dcn_calcs.o := $(calcs_ccflags)
CFLAGS_dcn_calc_auto.o := $(calcs_ccflags)
CFLAGS_dcn_calc_math.o := $(calcs_ccflags) -Wno-tautological-compare
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
index e9721a906592..f57a3b281408 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
@@ -18,6 +18,10 @@ endif
CFLAGS_dcn20_resource.o := -mhard-float -msse $(cc_stack_align)
+ifdef CONFIG_CC_IS_CLANG
+CFLAGS_dcn20_resource.o += -msse2
+endif
+
AMD_DAL_DCN20 = $(addprefix $(AMDDALPATH)/dc/dcn20/,$(DCN20))
AMD_DISPLAY_FILES += $(AMD_DAL_DCN20)
diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile
b/drivers/gpu/drm/amd/display/dc/dml/Makefile
index 0bb7a20675c4..132ade1a234e 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile
@@ -32,6 +32,10 @@ endif
dml_ccflags := -mhard-float -msse $(cc_stack_align)
+ifdef CONFIG_CC_IS_CLANG
+dml_ccflags += -msse2
+endif
+
CFLAGS_display_mode_lib.o := $(dml_ccflags)
ifdef CONFIG_DRM_AMD_DC_DCN2_0
diff --git a/drivers/gpu/drm/amd/display/dc/dsc/Makefile
b/drivers/gpu/drm/amd/display/dc/dsc/Makefile
index e019cd9447e8..17db603f2d1f 100644
--- a/drivers/gpu/drm/amd/display/dc/dsc/Makefile
+++ b/drivers/gpu/drm/amd/display/dc/dsc/Makefile
@@ -9,6 +9,10 @@ endif
dsc_ccflags := -mhard-float -msse $(cc_stack_align)
+ifdef CONFIG_CC_IS_CLANG
+dsc_ccflags += -msse2
+endif
+
CFLAGS_rc_calc.o := $(dsc_ccflags)
CFLAGS_rc_calc_dpi.o := $(dsc_ccflags)
CFLAGS_codec_main_amd.o := $(dsc_ccflags)
--
2.22.0.657.g960e92d24f-goog
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/display: Support clang option for stack alignment
On Fri, Jul 12, 2019 at 2:37 AM Arnd Bergmann wrote:
>
> As previously fixed for dml in commit 4769278e5c7f ("amdgpu/dc/dml:
> Support clang option for stack alignment") and calcs in commit
> cc32ad8f559c ("amdgpu/dc/calcs: Support clang option for stack
> alignment"), dcn20 uses an option that is not available with clang:
>
> clang: error: unknown argument: '-mpreferred-stack-boundary=4'
> scripts/Makefile.build:281: recipe for target
> 'drivers/gpu/drm/amd/amdgpu/../display/dc/dcn20/dcn20_resource.o' failed
>
> Use the same trick that we have in the other two files.
Maybe time for a macro in Kbuild.include or some such, if we see this
pattern being repeated?
>
> Fixes: 7ed4e6352c16 ("drm/amd/display: Add DCN2 HW Sequencer and Resource")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/gpu/drm/amd/display/dc/dcn20/Makefile | 8 +++-
> drivers/gpu/drm/amd/display/dc/dsc/Makefile | 16
> 2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
> b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
> index 1b68de27ba74..e9721a906592 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/Makefile
> @@ -10,7 +10,13 @@ ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT
> DCN20 += dcn20_dsc.o
> endif
>
> -CFLAGS_dcn20_resource.o := -mhard-float -msse -mpreferred-stack-boundary=4
> +ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
> + cc_stack_align := -mpreferred-stack-boundary=4
> +else ifneq ($(call cc-option, -mstack-alignment=16),)
> + cc_stack_align := -mstack-alignment=16
> +endif
> +
> +CFLAGS_dcn20_resource.o := -mhard-float -msse $(cc_stack_align)
>
> AMD_DAL_DCN20 = $(addprefix $(AMDDALPATH)/dc/dcn20/,$(DCN20))
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dsc/Makefile
> b/drivers/gpu/drm/amd/display/dc/dsc/Makefile
> index c5d5b94e2604..e019cd9447e8 100644
> --- a/drivers/gpu/drm/amd/display/dc/dsc/Makefile
> +++ b/drivers/gpu/drm/amd/display/dc/dsc/Makefile
> @@ -1,10 +1,18 @@
> #
> # Makefile for the 'dsc' sub-component of DAL.
>
> -CFLAGS_rc_calc.o := -mhard-float -msse -mpreferred-stack-boundary=4
> -CFLAGS_rc_calc_dpi.o := -mhard-float -msse -mpreferred-stack-boundary=4
> -CFLAGS_codec_main_amd.o := -mhard-float -msse -mpreferred-stack-boundary=4
> -CFLAGS_dc_dsc.o := -mhard-float -msse -mpreferred-stack-boundary=4
> +ifneq ($(call cc-option, -mpreferred-stack-boundary=4),)
> + cc_stack_align := -mpreferred-stack-boundary=4
> +else ifneq ($(call cc-option, -mstack-alignment=16),)
> + cc_stack_align := -mstack-alignment=16
> +endif
> +
> +dsc_ccflags := -mhard-float -msse $(cc_stack_align)
> +
> +CFLAGS_rc_calc.o := $(dsc_ccflags)
> +CFLAGS_rc_calc_dpi.o := $(dsc_ccflags)
> +CFLAGS_codec_main_amd.o := $(dsc_ccflags)
> +CFLAGS_dc_dsc.o := $(dsc_ccflags)
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Cleanup of -Wunused-const-variable in drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
On Thu, Jun 13, 2019 at 1:50 PM Sean Paul wrote:
>
> On Thu, Jun 13, 2019 at 4:13 PM Nathan Huckleberry wrote:
> >
> > Hey all,
> >
> > I'm looking into cleaning up ignored warnings in the kernel so we can
> > remove compiler flags to ignore warnings.
> >
> > There are several unused variables in dpu_formats.c
> > ('dpu_format_map_tile', 'dpu_format_map_p010',
> > 'dpu_format_map_p010_ubwc', 'dpu_format_map_tp10_ubwc').
> > They look like modifiers that were never implemented. I'd like to
> > remove these variables if there are no plans moving forward to
> > implement them. Otherwise I'll just leave them.
>
> We can probably remove them for now and if someone wants to add
> support, they can dredge them back up.
Yep, this has been the feedback for other patches for this warning
when the code was dead or not obviously some kind of
bug/typo/copy-pasta.
Nathan, please submit a patch removing the dead code; it may be
reverted later when it's actually wired up. Nothing is truly lost w/
git*.
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Cleanup of -Wunused-const-variable in drivers/gpu/drm/msm/disp/dpu1/dpu_formats.c
On Fri, Jun 14, 2019 at 2:43 AM Jani Nikula wrote: > No opinion on the said variables above, but, FWIW, personally I think > it's fine to add the cflags to supress warnings locally where needed in > order to be able to achieve the greater goal of removing the cflags > globally. I think there's on the order of ~10 of these: https://github.com/ClangBuiltLinux/linux/issues?q=is%3Aissue+is%3Aopen+label%3A-Wunused-const-variable Nathan's got a pretty good handle on just fixing them. > In drivers/gpu/drm/i915/Makefile we actually go for much stricter > warnings than the kernel defaults, and disable a more limited and > tailored set of warnings. I like this. > > You can do this both on a subdir and file level with subdir-ccflags-y > and CFLAGS_filename.o, respectively. That said, I have used this trick before, but I feel like the fewer people that know about this trick, the less it can be [ab]used. -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/komeda: Use memset to initialize config_id
On Wed, Apr 24, 2019 at 11:27 PM Nathan Chancellor
wrote:
>
> Clang warns:
>
> drivers/gpu/drm/arm/display/komeda/komeda_dev.c:76:38: warning: suggest
> braces around initialization of subobject [-Wmissing-braces]
> union komeda_config_id config_id = {0,};
> ^
> {}
> 1 warning generated.
>
> One way to fix these warnings is to add additional braces like Clang
> suggests; however, there has been a bit of push back from some
> maintainers, who just prefer memset as it is unambiguous, doesn't
> depend on a particular compiler version, and properly initializes all
> subobjects [1][2]. Do that here so there are no more warnings.
>
> [1]:
> https://lore.kernel.org/lkml/022e41c0-8465-dc7a-a45c-64187ecd9...@amd.com/
> [2]:
> https://lore.kernel.org/lkml/20181128.215241.702406654469517539.da...@davemloft.net/
>
> Fixes: 4cc734cb79a8 ("drm/komeda: Add sysfs attribute: core_id and config_id")
> Link: https://github.com/ClangBuiltLinux/linux/issues/457
> Signed-off-by: Nathan Chancellor
> ---
> drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> index 9d6c31cca875..e605a518f59a 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
> @@ -73,9 +73,10 @@ config_id_show(struct device *dev, struct device_attribute
> *attr, char *buf)
> {
> struct komeda_dev *mdev = dev_to_mdev(dev);
> struct komeda_pipeline *pipe = mdev->pipelines[0];
> - union komeda_config_id config_id = {0,};
Haven't seen a trailing comma too often. Thanks for the patch.
Reviewed-by: Nick Desaulniers
> + union komeda_config_id config_id;
> int i;
>
> + memset(_id, 0, sizeof(config_id));
> config_id.max_line_sz = pipe->layers[0]->hsize_in.end;
> config_id.n_pipelines = mdev->n_pipelines;
> config_id.n_scalers = pipe->n_scalers;
> --
> 2.21.0
>
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Clang warning in drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
On Wed, Mar 20, 2019 at 2:37 AM Koenig, Christian wrote: > > Am 20.03.19 um 05:34 schrieb Nathan Chancellor: > > On Wed, Mar 20, 2019 at 01:31:27AM +, Pan, Xinhui wrote: > >> these two enumerated types are same for now. both of them might change in > >> the future. Please consider if the YAGNI principle applies here. https://martinfowler.com/bliki/Yagni.html > >> > >> I have not used clang, but would .block_id = (int)head->block fix your > >> warning? If such change is acceptable, I can make one then. My preference on solutions, in order: 1. One enum (this is the simplest most type safe solution). Add another enum when you actually need it. Otherwise, YAGNI. 2. Safe casting function (like the one Nathan supplied, maybe with WARN_ONCE rather than WARN). This ensures that at least if the types diverge you get a runtime warning. A compile-time warning would be preferred, but I haven't taken the time to think through how that might be implemented. 3. Cast to int (this has been used in other places throughout the kernel, but provides the weakest type safety and doesn't catch future divergence). 4. Disabling the warning. (I almost never advocate for this). > Another question is if it is such a good idea to just silence the warning? For the kernel, it seems that each maintainer can choose what to apply to their subsystem. I would recommend against disabling additional warnings that aren't disable kernel-wide for most cases. -Wenum-conversion has spotted many bugs. While the enums in question today are not different, they MIGHT eventually diverge and lead to bugs, like the others we've found and fixed throughout the kernel. So I would recommend fixing now, and be insulated in the future. -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/powerplay: Zero initialize num_of_levels in vega20_set_single_dpm_table
On Tue, Mar 19, 2019 at 6:00 PM Nathan Chancellor
wrote:
>
> When building with -Wsometimes-uninitialized, Clang warns:
>
> drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:456:2: warning:
> variable 'num_of_levels' is used uninitialized whenever '?:' condition
> is false [-Wsometimes-uninitialized]
> smu_read_smc_arg(smu, _of_levels);
> ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:608:3: note:
> expanded from macro 'smu_read_smc_arg'
> ((smu)->funcs->read_smc_arg? (smu)->funcs->read_smc_arg((smu), (arg))
> : 0)
> ^~
> drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:457:7: note:
> uninitialized use occurs here
> if (!num_of_levels) {
> ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:456:2: note: remove
> the '?:' if its condition is always true
> smu_read_smc_arg(smu, _of_levels);
> ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/inc/amdgpu_smu.h:608:3: note:
> expanded from macro 'smu_read_smc_arg'
> ((smu)->funcs->read_smc_arg? (smu)->funcs->read_smc_arg((smu), (arg))
> : 0)
> ^
> drivers/gpu/drm/amd/amdgpu/../powerplay/vega20_ppt.c:446:27: note:
> initialize the variable 'num_of_levels' to silence this warning
> uint32_t i, num_of_levels, clk;
> ^
> = 0
> 1 warning generated.
>
> The if statement it mentions as potentially problematic is currently
> always true because the read_smc_arg callback is assigned at the
> bottom of this file but Clang can't tell that. If the callback were
> ever to disappear, num_of_levels would never be initialized. Just
> zero initialize it to ensure that the intent behind this code
> remains the same.
Thanks for the simple fix.
Reviewed-by: Nick Desaulniers
>
> Fixes: 870b996f955f ("drm/amd/powerplay: set defalut dpm table for smu")
> Link: https://github.com/ClangBuiltLinux/linux/issues/425
> Signed-off-by: Nathan Chancellor
> ---
> drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> index 7e9e8ad9a300..41e6f49c9cb6 100644
> --- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
> @@ -443,7 +443,7 @@ vega20_set_single_dpm_table(struct smu_context *smu,
> PPCLK_e clk_id)
> {
> int ret = 0;
> - uint32_t i, num_of_levels, clk;
> + uint32_t i, num_of_levels = 0, clk;
>
> ret = smu_send_smc_msg_with_param(smu,
> SMU_MSG_GetDpmFreqByIndex,
> --
> 2.21.0
>
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH] drm/i915: Zero initialize this_cpu in busywait_stop
On Fri, Mar 8, 2019 at 12:27 AM Chris Wilson wrote:
>
> Quoting Nathan Chancellor (2019-03-08 01:20:24)
> > When building with -Wsometimes-uninitialized, Clang warns:
> >
> > drivers/gpu/drm/i915/i915_request.c:1032:6: warning: variable 'this_cpu'
> > is used uninitialized whenever '&&' condition is false
> > [-Wsometimes-uninitialized]
> >
> > time_after expands to use two typecheck with logical ANDs between them.
> > typecheck evaluates to 1 but Clang clearly gets confused with the logic
> > that as semantic analysis happens early in the pipeline. Fix this by
> > just zero initializing this_cpu as it will always be properly
> > initialized before the comparison below.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/415
> > Signed-off-by: Nathan Chancellor
> > ---
> >
> > Alternatively, this can be solved by having the return value of
> > local_clock_us(_cpu) be a local variable but this seems less
> > controversial.
>
> I'll just wait for clang to be fixed, as this severely undermines any
> respect I have for its semantic analysis.
> -Chris
I'm still playing around with this in Godbolt (my hunch is that GNU C
statement expressions are maybe inlined as part of GCC's early
inlining phase). For example:
https://godbolt.org/z/G54s5z
If you change `typecheck(unsigned long, a)` and `typecheck(unsigned
long, b)` in `time_after()` both to `1` (what `typecheck` would
evaluate to), then the warning goes away. But a further
simplification shows that GNU C statement expressions are not the
problem:
https://godbolt.org/z/KzCN8m
I need to keep investigating, but there may be more we can do on the
compiler side.
It seems that another workaround that avoid default initialization is
to just create another local for the temporary expression that
provably initialized this_cpu, ie.
diff --git a/drivers/gpu/drm/i915/i915_request.c
b/drivers/gpu/drm/i915/i915_request.c
index c2a5c48c7541..5b90b5c35c8b 100644
--- a/drivers/gpu/drm/i915/i915_request.c
+++ b/drivers/gpu/drm/i915/i915_request.c
@@ -1028,8 +1028,9 @@ static unsigned long local_clock_us(unsigned int *cpu)
static bool busywait_stop(unsigned long timeout, unsigned int cpu)
{
unsigned int this_cpu;
+ unsigned long local_clock = local_clock_us(_cpu);
- if (time_after(local_clock_us(_cpu), timeout))
+ if (time_after(local_clock, timeout))
return true;
return this_cpu != cpu;
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vmwgfx: Zero initialize handle in vmw_execbuf_process
On Thu, Mar 7, 2019 at 2:26 PM Nathan Chancellor wrote: > > When building with -Wsometimes-uninitialized, Clang warns: > > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:3964:7: warning: variable > 'handle' is used uninitialized whenever '?:' condition is false > [-Wsometimes-uninitialized] > > It's not wrong; however, in practice, this is never an issue because > the value of handle isn't used when user_fence_rep is NULL because > vmw_execbuf_copy_fence_user returns immediately when that is the case. Still, it's better not to conditionally pass unitialized memory to functions. Invariants like "don't touch this possibly unitialized memory in THIS case" don't always pass from maintainer to maintainer. Thanks for the patch. Reviewed-by: Nick Desaulniers > Just zero initialize this variable so that Clang no longer warns. > > Link: https://github.com/ClangBuiltLinux/linux/issues/397 > Signed-off-by: Nathan Chancellor > --- > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > index 88b8178d4687..4ba06c2828a1 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c > @@ -3829,7 +3829,7 @@ int vmw_execbuf_process(struct drm_file *file_priv, > struct vmw_sw_context *sw_context = _priv->ctx; > struct vmw_fence_obj *fence = NULL; > struct vmw_cmdbuf_header *header; > - uint32_t handle; > + uint32_t handle = 0; > int ret; > int32_t out_fence_fd = -1; > struct sync_file *sync_file = NULL; > -- > 2.21.0 > > -- > You received this message because you are subscribed to the Google Groups > "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to clang-built-linux+unsubscr...@googlegroups.com. > To post to this group, send email to clang-built-li...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/clang-built-linux/20190307222611.18100-1-natechancellor%40gmail.com. > For more options, visit https://groups.google.com/d/optout. -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/display: add -msse2 to prevent Clang from emitting libcalls to undefined SW FP routines
Suggestions: 1. revert patch 2. get me the disassembly from gcc of the translation unit in question. 3. land patch that adds clang guards or something different based on 2. Revert first; ask questions later. On Tue, Jan 29, 2019 at 11:01 AM Wentland, Harry wrote: > > On 2019-01-29 1:56 p.m., Guenter Roeck wrote: > > On Tue, Jan 29, 2019 at 10:30:31AM -0500, Alex Deucher wrote: > >> On Fri, Jan 25, 2019 at 10:29 AM Wentland, Harry > >> wrote: > >>> > >>> On 2019-01-24 7:52 p.m., ndesaulni...@google.com wrote: > >>>> arch/x86/Makefile disables SSE and SSE2 for the whole kernel. The > >>>> AMDGPU drivers modified in this patch re-enable SSE but not SSE2. Turn > >>>> on SSE2 to support emitting double precision floating point instructions > >>>> rather than calls to non-existent (usually available from gcc_s or > >>>> compiler_rt) floating point helper routines. > >>>> > >>>> Link: > >>>> https://gcc.gnu.org/onlinedocs/gccint/Soft-float-library-routines.html > >>>> Link: https://github.com/ClangBuiltLinux/linux/issues/327 > >>>> Cc: sta...@vger.kernel.org # 4.19 > >>>> Reported-by: S, Shirish > >>>> Reported-by: Matthias Kaehlcke > >>>> Suggested-by: James Y Knight > >>>> Suggested-by: Nathan Chancellor > >>>> Signed-off-by: Nick Desaulniers > >>>> Tested-by: Guenter Roeck > >>> > >>> Reviewed-by: Harry Wentland > >>> > >>> and applied. > >>> > >> > >> This patch causes a segfault: > >> https://bugs.freedesktop.org/show_bug.cgi?id=109487 > >> > >> Any ideas? > >> > > > > Set -msse2 only for clang ? I suspect, though, that this might only > > solve the compile problem. If I understand correctly, the first > > warning in the log is due to BREAK_TO_DEBUGGER(), suggesting that > > something is seriously wrong. Maybe enabling sse2 results in different > > results from floating point operations. > > > > Unfortunately I don't have a system with the affected GPU, > > so I can't run any tests on real hardware. Unless someone can test > > with real hardware, I think we have no choice but to revert the > > patch. > > > > Might be a good idea. Even though, best to revert for now until we understand > what's going on. > > It seems like people at AMD building with gcc 5.4 are fine, but those using > gcc 7.3 or 8.2 experience the crash. > > Harry > > > Guenter > > > >> Alex > >> > >>> Harry > >>> > >>>> --- > >>>> drivers/gpu/drm/amd/display/dc/calcs/Makefile | 2 +- > >>>> drivers/gpu/drm/amd/display/dc/dml/Makefile | 2 +- > >>>> 2 files changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/amd/display/dc/calcs/Makefile > >>>> b/drivers/gpu/drm/amd/display/dc/calcs/Makefile > >>>> index 95f332ee3e7e..dc85a3c088af 100644 > >>>> --- a/drivers/gpu/drm/amd/display/dc/calcs/Makefile > >>>> +++ b/drivers/gpu/drm/amd/display/dc/calcs/Makefile > >>>> @@ -30,7 +30,7 @@ else ifneq ($(call cc-option, -mstack-alignment=16),) > >>>> cc_stack_align := -mstack-alignment=16 > >>>> endif > >>>> > >>>> -calcs_ccflags := -mhard-float -msse $(cc_stack_align) > >>>> +calcs_ccflags := -mhard-float -msse -msse2 $(cc_stack_align) > >>>> > >>>> CFLAGS_dcn_calcs.o := $(calcs_ccflags) > >>>> CFLAGS_dcn_calc_auto.o := $(calcs_ccflags) > >>>> diff --git a/drivers/gpu/drm/amd/display/dc/dml/Makefile > >>>> b/drivers/gpu/drm/amd/display/dc/dml/Makefile > >>>> index d97ca6528f9d..33c7d7588712 100644 > >>>> --- a/drivers/gpu/drm/amd/display/dc/dml/Makefile > >>>> +++ b/drivers/gpu/drm/amd/display/dc/dml/Makefile > >>>> @@ -30,7 +30,7 @@ else ifneq ($(call cc-option, -mstack-alignment=16),) > >>>> cc_stack_align := -mstack-alignment=16 > >>>> endif > >>>> > >>>> -dml_ccflags := -mhard-float -msse $(cc_stack_align) > >>>> +dml_ccflags := -mhard-float -msse -msse2 $(cc_stack_align) > >>>> > >>>> CFLAGS_display_mode_lib.o := $(dml_ccflags) > >>>> CFLAGS_display_pipe_clocks.o := $(dml_ccflags) > >>>> > >>> ___ > >>> amd-gfx mailing list > >>> amd-...@lists.freedesktop.org > >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/i915: Disable -Wuninitialized
On Fri, Jan 25, 2019 at 11:34 PM Nick Desaulniers
wrote:
>
> On Fri, Jan 25, 2019 at 11:13 PM Nathan Chancellor
> wrote:
> >
> > This warning is disabled by default in scripts/Makefile.extrawarn when
> > W= is not provided but this Makefile adds -Wall after this warning is
> > disabled so it shows up in the build when it shouldn't:
> >
> > In file included from drivers/gpu/drm/i915/intel_breadcrumbs.c:895:
> > drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c:350:34: error:
> > variable 'wq' is uninitialized when used within its own initialization
> > [-Werror,-Wuninitialized]
> > DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
> > ^~
> > ./include/linux/wait.h:74:63: note: expanded from macro
> > 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK'
> > struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name)
> > ^~~~
> > ./include/linux/wait.h:72:33: note: expanded from macro
> > '__WAIT_QUEUE_HEAD_INIT_ONSTACK'
> > ({ init_waitqueue_head(); name; })
> >^~~~
> > 1 error generated.
> >
> > Explicitly disable the warning like commit 46e2068081e9 ("drm/i915:
> > Disable some extra clang warnings").
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/220
> > Signed-off-by: Nathan Chancellor
>
> Reviewed-by: Nick Desaulniers
gah! I am on my work device, in that case it should be:
Reviewed-by: Nick Desaulniers
>
> probably could give Chris Wilson the suggested by tag.
> https://lore.kernel.org/lkml/154513398652.1108.7150969916024071452@skylake-alporthouse-com/
>
> > ---
> >
> > v1 -> v2:
> >
> > * Rather than disable the warning for the problematic folder, disable it
> > for the entire folder like Matthias's commit.
>
> Thanks for following up with a v2.
>
> --
> Thanks,
> ~Nick Desaulniers
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/i915: Disable -Wuninitialized
On Fri, Jan 25, 2019 at 11:13 PM Nathan Chancellor
wrote:
>
> This warning is disabled by default in scripts/Makefile.extrawarn when
> W= is not provided but this Makefile adds -Wall after this warning is
> disabled so it shows up in the build when it shouldn't:
>
> In file included from drivers/gpu/drm/i915/intel_breadcrumbs.c:895:
> drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c:350:34: error:
> variable 'wq' is uninitialized when used within its own initialization
> [-Werror,-Wuninitialized]
> DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
> ^~
> ./include/linux/wait.h:74:63: note: expanded from macro
> 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK'
> struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name)
> ^~~~
> ./include/linux/wait.h:72:33: note: expanded from macro
> '__WAIT_QUEUE_HEAD_INIT_ONSTACK'
> ({ init_waitqueue_head(); name; })
>^~~~
> 1 error generated.
>
> Explicitly disable the warning like commit 46e2068081e9 ("drm/i915:
> Disable some extra clang warnings").
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/220
> Signed-off-by: Nathan Chancellor
Reviewed-by: Nick Desaulniers
probably could give Chris Wilson the suggested by tag.
https://lore.kernel.org/lkml/154513398652.1108.7150969916024071452@skylake-alporthouse-com/
> ---
>
> v1 -> v2:
>
> * Rather than disable the warning for the problematic folder, disable it
> for the entire folder like Matthias's commit.
Thanks for following up with a v2.
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: Disable -Wuninitialized for intel_breadcrumbs.o
On Tue, Dec 18, 2018 at 11:01 AM Nathan Chancellor wrote: > On Tue, Dec 18, 2018 at 11:53:06AM +, Chris Wilson wrote: > > The other false-positive clang-6 gave was for local_clock_us(). > > Presumably that one is fixed? > > With this patch, I can build i915 using defconfig and allyesconfig > without any warnings with tip-of-tree Clang. Also, please let us know about any other bugs found via testing with Clang: https://github.com/ClangBuiltLinux/linux/issues We're happy to take a look! -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/display: Pass app_tf by value rather than by reference
L)` could be replaced with `if (app_tf != transfer_func_unknown)` hoisted from `build_vrr_infopacket_fs2_data`? (There's only one caller of `build_vrr_infopacket_fs2_data` today, maybe fine to leave the unconditional call and check). Either way, I suspect without the change to drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c, that this fails to compile? -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/display: Pass app_tf by value rather than by reference
On Tue, Dec 11, 2018 at 1:42 PM Nathan Chancellor
wrote:
>
> On Tue, Dec 11, 2018 at 01:25:00PM -0800, Nick Desaulniers wrote:
> > On Mon, Dec 10, 2018 at 3:42 PM Nathan Chancellor
> > wrote:
> > >
> > > Clang warns when an expression that equals zero is used as a null
> > > pointer constant (in lieu of NULL):
> > >
> > > drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:4435:3:
> > > warning: expression which evaluates to zero treated as a null pointer
> > > constant of type 'const enum color_transfer_func *'
> > > [-Wnon-literal-null-conversion]
> > > TRANSFER_FUNC_UNKNOWN,
> > > ^
> > > 1 warning generated.
> > >
> > > This warning is caused by commit bb47de736661 ("drm/amdgpu: Set FreeSync
> > > state using drm VRR properties") and it could be solved by using NULL
> > > instead of TRANSFER_FUNC_UNKNOWN or casting TRANSFER_FUNC_UNKNOWN as a
> > > pointer. However, after looking into it, there doesn't appear to be a
> > > good reason to pass app_tf by reference as it is never mutated along the
> > > way. This is the only code path in which app_tf is used:
> > >
> > > mod_freesync_build_vrr_infopacket ->
> > > build_vrr_infopacket_v2 ->
> > > build_vrr_infopacket_fs2_data
> > >
> > > Neither mod_freesync_build_vrr_infopacket or build_vrr_infopacket_v2
> > > modify app_tf's value and build_vrr_infopacket_fs2_data expects just
> > > the value so we can avoid dereferencing anything by just passing in
> > > app_tf's value to mod_freesync_build_vrr_infopacket and
> > > build_vrr_infopacket_v2.
> > >
> > > There is no functional change because build_vrr_infopacket_fs2_data
> > > doesn't do anything if TRANSFER_FUNC_UNKNOWN is passed to it, the same
> > > as not calling build_vrr_infopacket_fs2_data at all like before this
> > > change when NULL was used for app_tf.
> >
> > Nathan,
> > Thanks for sending this patch. I was hoping to provide review sooner,
> > but have been quite busy lately.
> >
>
> Late review is better than no review, I appeciate you taking the time to
> do this!
>
> > Yeah, especially for LP64 targets, the pointer to enum is larger than
> > just the enum, and if it's not being updated ("in/out paramter")
> > there's no need to pass by pointer.
> >
>
> Thanks for confirming!
>
> > >
> > > Signed-off-by: Nathan Chancellor
> > > ---
> > > drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 7 +++
> > > drivers/gpu/drm/amd/display/modules/inc/mod_freesync.h | 2 +-
> > > 2 files changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > index 620a171620ee..520665a9d81a 100644
> > > --- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > +++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
> > > @@ -656,7 +656,7 @@ static void build_vrr_infopacket_v1(enum signal_type
> > > signal,
> > >
> > > static void build_vrr_infopacket_v2(enum signal_type signal,
> > > const struct mod_vrr_params *vrr,
> > > - const enum color_transfer_func *app_tf,
> > > + enum color_transfer_func app_tf,
> > > struct dc_info_packet *infopacket)
> > > {
> > > unsigned int payload_size = 0;
> > > @@ -664,8 +664,7 @@ static void build_vrr_infopacket_v2(enum signal_type
> > > signal,
> > > build_vrr_infopacket_header_v2(signal, infopacket, _size);
> > > build_vrr_infopacket_data(vrr, infopacket);
> > >
> > > - if (app_tf != NULL)
> > > - build_vrr_infopacket_fs2_data(*app_tf, infopacket);
> > > + build_vrr_infopacket_fs2_data(app_tf, infopacket);
> > >
> > > build_vrr_infopacket_checksum(_size, infopacket);
> > >
> > > @@ -676,7 +675,7 @@ void mod_freesync_build_vrr_infopacket(struct
> > > mod_freesync *mod_freesync,
> > > const struct dc_stream_state *stream,
> > > const struct mod_vrr_params *vrr,
> > > enum vrr_packet_type packet_type,
> > > - const enum color_transfer_func *app_tf,
> > > + enum color_transfer_func app_
Re: [PATCH] drm/i915: Disable -Wuninitialized for intel_breadcrumbs.o
On Thu, Oct 25, 2018 at 12:36 PM Nathan Chancellor
wrote:
>
> This warning is disabled by default in scripts/Makefile.extrawarn when
> W= is not provided but this Makefile adds -Wall after this warning is
> disabled so it shows up in the build when it shouldn't:
>
> In file included from drivers/gpu/drm/i915/intel_breadcrumbs.c:895:
> drivers/gpu/drm/i915/selftests/intel_breadcrumbs.c:350:34: error:
> variable 'wq' is uninitialized when used within its own initialization
> [-Werror,-Wuninitialized]
> DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
> ^~
> ./include/linux/wait.h:74:63: note: expanded from macro
> 'DECLARE_WAIT_QUEUE_HEAD_ONSTACK'
> struct wait_queue_head name = __WAIT_QUEUE_HEAD_INIT_ONSTACK(name)
> ^~~~
> ./include/linux/wait.h:72:33: note: expanded from macro
> '__WAIT_QUEUE_HEAD_INIT_ONSTACK'
> ({ init_waitqueue_head(); name; })
>^~~~
> 1 error generated.
>
> This warning looks to be a false positive given that init_waitqueue_head
> initializes name before it is used. Rather than disable the warning for
> the full folder like commit 46e2068081e9 ("drm/i915: Disable some extra
cc author/reviewer of 46e2068081e9.
I'm fine with the patch as is, unless others prefer to disable it for
the whole subdir? We could be playing whack-a-mole in the future
disabling this warning for other translation units.
> clang warnings"), just disable it for the one problematic file because
> it could be a useful warning for other cases.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/220
> Signed-off-by: Nathan Chancellor
> ---
> drivers/gpu/drm/i915/Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 1c2857f13ad4..f36c420afb99 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -26,6 +26,7 @@ subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
>
> # Fine grained warnings disable
> CFLAGS_i915_pci.o = $(call cc-disable-warning, override-init)
> +CFLAGS_intel_breadcrumbs.o = $(call cc-disable-warning, uninitialized)
> CFLAGS_intel_fbdev.o = $(call cc-disable-warning, override-init)
>
> subdir-ccflags-y += \
> --
> 2.19.1
>
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH] drm/i915: Convert _print_param to a macro
On Wed, Oct 10, 2018 at 11:21 PM Jani Nikula wrote: > > On Wed, 10 Oct 2018, Nick Desaulniers wrote: > > On Wed, Oct 10, 2018 at 1:30 PM Michal Wajdeczko > > wrote: > >> > >> On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula > >> wrote: > >> > >> > On Tue, 09 Oct 2018, Nick Desaulniers wrote: > >> >> On Tue, Oct 9, 2018 at 10:14 AM Nathan Chancellor > >> >> wrote: > >> >>> > >> >>> When building the kernel with Clang with defconfig and CONFIG_64BIT > >> >>> disabled, vmlinux fails to link because of the BUILD_BUG in > >> >>> _print_param. > >> >>> > >> >>> ld: drivers/gpu/drm/i915/i915_params.o: in function `i915_params_dump': > >> >>> i915_params.c:(.text+0x56): undefined reference to > >> >>> `__compiletime_assert_191' > >> >>> > >> >>> This function is semantically invalid unless the code is first inlined > >> >>> then constant folded, which doesn't work for Clang because semantic > >> >>> analysis happens before optimization/inlining. Converting this function > >> >>> to a macro avoids this problem and allows Clang to properly remove the > >> >>> BUILD_BUG during optimization. > >> >> > >> >> Thanks Nathan for the patch. To provide more context, Clang does > >> >> semantic analysis before optimization, where as GCC does these > >> >> together (IIUC). So the above link error is from the naked > >> >> BUILD_BUG(). Clang can't evaluate the __builtin_strcmp's statically > >> >> until inlining has occurred, but that optimization happens after > >> >> semantic analysis. To do the inlining before semantic analysis, we > >> >> MUST leverage the preprocessor, which runs before the compiler starts > >> >> doing semantic analysis. I suspect this code is not valid for GCC > >> >> unless optimizations are enabled (the kernel only does compile with > >> >> optimizations turned on). This change allows us to build this > >> >> translation unit with Clang. > >> >> > >> >> Acked-by: Nick Desaulniers > >> >> (Note: this is the change I suggested, so not sure whether Acked-by or > >> >> Reviewed-by is more appropriate). > >> > > >> > *Sad trombone* > >> > > >> > I'd rather see us converting more macros to static inlines than the > >> > other way round. > >> > > >> > I'll let others chime in if they have any better ideas, otherwise I'll > >> > apply this one. > >> > >> Option 1: Just drop BUILD_BUG() from _print_param() function. > > > > I was also thinking of this. > > So does this fix the issue? Yes, that should do the trick. I assume this macro can also be rewritten to use __builtin_types_compatible_p and __builtin_choose_expr (rather than tokenizing the type and using __builtin_strcmp), but maybe an exercise for another day. We're happy with the simplest fix acceptable for now. > > diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > index bd6bd8879cab..8d71886b5f03 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -184,7 +184,8 @@ static __always_inline void _print_param(struct > drm_printer *p, > else if (!__builtin_strcmp(type, "char *")) > drm_printf(p, "i915.%s=%s\n", name, *(const char **)x); > else > - BUILD_BUG(); > + WARN_ONCE(1, "no printer defined for param type %s > (i915.%s)\n", > + type, name); > } > > /** > > --- > > > > >> > >> Option 2: Use aliases instead of real types in param() macros. > > > > Will that affect other users of I915_PARAMS_FOR_EACH than _print_param? > > > > Either way, thanks for the help towards resolving this! We appreciate it! > > > >> > >> Aliases can be same as in linux/moduleparam.h (charp|int|uint|bool) > >> We can convert aliases back to real types but it will also allow > >> to construct proper names for dedicated functions - see [1] > >> > >> Michal > >> > >> [1] https://patchwork.freedesktop.org/patch/255928/ > > I can't find this on the list; was this sent just to patchwork or what? > > BR, > Jani. > > > >> > >> > >>
Re: [Intel-gfx] [PATCH] drm/i915: Convert _print_param to a macro
On Wed, Oct 10, 2018 at 1:30 PM Michal Wajdeczko
wrote:
>
> On Wed, 10 Oct 2018 14:01:40 +0200, Jani Nikula
> wrote:
>
> > On Tue, 09 Oct 2018, Nick Desaulniers wrote:
> >> On Tue, Oct 9, 2018 at 10:14 AM Nathan Chancellor
> >> wrote:
> >>>
> >>> When building the kernel with Clang with defconfig and CONFIG_64BIT
> >>> disabled, vmlinux fails to link because of the BUILD_BUG in
> >>> _print_param.
> >>>
> >>> ld: drivers/gpu/drm/i915/i915_params.o: in function `i915_params_dump':
> >>> i915_params.c:(.text+0x56): undefined reference to
> >>> `__compiletime_assert_191'
> >>>
> >>> This function is semantically invalid unless the code is first inlined
> >>> then constant folded, which doesn't work for Clang because semantic
> >>> analysis happens before optimization/inlining. Converting this function
> >>> to a macro avoids this problem and allows Clang to properly remove the
> >>> BUILD_BUG during optimization.
> >>
> >> Thanks Nathan for the patch. To provide more context, Clang does
> >> semantic analysis before optimization, where as GCC does these
> >> together (IIUC). So the above link error is from the naked
> >> BUILD_BUG(). Clang can't evaluate the __builtin_strcmp's statically
> >> until inlining has occurred, but that optimization happens after
> >> semantic analysis. To do the inlining before semantic analysis, we
> >> MUST leverage the preprocessor, which runs before the compiler starts
> >> doing semantic analysis. I suspect this code is not valid for GCC
> >> unless optimizations are enabled (the kernel only does compile with
> >> optimizations turned on). This change allows us to build this
> >> translation unit with Clang.
> >>
> >> Acked-by: Nick Desaulniers
> >> (Note: this is the change I suggested, so not sure whether Acked-by or
> >> Reviewed-by is more appropriate).
> >
> > *Sad trombone*
> >
> > I'd rather see us converting more macros to static inlines than the
> > other way round.
> >
> > I'll let others chime in if they have any better ideas, otherwise I'll
> > apply this one.
>
> Option 1: Just drop BUILD_BUG() from _print_param() function.
I was also thinking of this.
>
> Option 2: Use aliases instead of real types in param() macros.
Will that affect other users of I915_PARAMS_FOR_EACH than _print_param?
Either way, thanks for the help towards resolving this! We appreciate it!
>
> Aliases can be same as in linux/moduleparam.h (charp|int|uint|bool)
> We can convert aliases back to real types but it will also allow
> to construct proper names for dedicated functions - see [1]
>
> Michal
>
> [1] https://patchwork.freedesktop.org/patch/255928/
>
>
> >
> > BR,
> > Jani.
> >
> >>
> >>>
> >>> The output of 'objdump -D' is identically before and after this change
> >>> for GCC regardless of if CONFIG_64BIT is set and allows Clang to link
> >>> the kernel successfully with or without CONFIG_64BIT set.
> >>>
> >>> Link: https://github.com/ClangBuiltLinux/linux/issues/191
> >>> Suggested-by: Nick Desaulniers
> >>> Signed-off-by: Nathan Chancellor
> >>> ---
> >>> drivers/gpu/drm/i915/i915_params.c | 29 +
> >>> 1 file changed, 13 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_params.c
> >>> b/drivers/gpu/drm/i915/i915_params.c
> >>> index 295e981e4a39..a0f20b9b6f2d 100644
> >>> --- a/drivers/gpu/drm/i915/i915_params.c
> >>> +++ b/drivers/gpu/drm/i915/i915_params.c
> >>> @@ -174,22 +174,19 @@ i915_param_named(enable_dpcd_backlight, bool,
> >>> 0600,
> >>> i915_param_named(enable_gvt, bool, 0400,
> >>> "Enable support for Intel GVT-g graphics virtualization host
> >>> support(default:false)");
> >>>
> >>> -static __always_inline void _print_param(struct drm_printer *p,
> >>> -const char *name,
> >>> -const char *type,
> >>> -const void *x)
> >>> -{
> >>> - if (!__builtin_strcmp(type, "bool"))
> >>> - drm_printf(p, "i915.%s=%s\n", name, yesno(*(const bool
> >>> *)x));
> >>> - else if (!__buil
Re: [PATCH] drm/i915: Convert _print_param to a macro
On Tue, Oct 9, 2018 at 10:14 AM Nathan Chancellor
wrote:
>
> When building the kernel with Clang with defconfig and CONFIG_64BIT
> disabled, vmlinux fails to link because of the BUILD_BUG in
> _print_param.
>
> ld: drivers/gpu/drm/i915/i915_params.o: in function `i915_params_dump':
> i915_params.c:(.text+0x56): undefined reference to
> `__compiletime_assert_191'
>
> This function is semantically invalid unless the code is first inlined
> then constant folded, which doesn't work for Clang because semantic
> analysis happens before optimization/inlining. Converting this function
> to a macro avoids this problem and allows Clang to properly remove the
> BUILD_BUG during optimization.
Thanks Nathan for the patch. To provide more context, Clang does
semantic analysis before optimization, where as GCC does these
together (IIUC). So the above link error is from the naked
BUILD_BUG(). Clang can't evaluate the __builtin_strcmp's statically
until inlining has occurred, but that optimization happens after
semantic analysis. To do the inlining before semantic analysis, we
MUST leverage the preprocessor, which runs before the compiler starts
doing semantic analysis. I suspect this code is not valid for GCC
unless optimizations are enabled (the kernel only does compile with
optimizations turned on). This change allows us to build this
translation unit with Clang.
Acked-by: Nick Desaulniers
(Note: this is the change I suggested, so not sure whether Acked-by or
Reviewed-by is more appropriate).
>
> The output of 'objdump -D' is identically before and after this change
> for GCC regardless of if CONFIG_64BIT is set and allows Clang to link
> the kernel successfully with or without CONFIG_64BIT set.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/191
> Suggested-by: Nick Desaulniers
> Signed-off-by: Nathan Chancellor
> ---
> drivers/gpu/drm/i915/i915_params.c | 29 +
> 1 file changed, 13 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> index 295e981e4a39..a0f20b9b6f2d 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -174,22 +174,19 @@ i915_param_named(enable_dpcd_backlight, bool, 0600,
> i915_param_named(enable_gvt, bool, 0400,
> "Enable support for Intel GVT-g graphics virtualization host
> support(default:false)");
>
> -static __always_inline void _print_param(struct drm_printer *p,
> -const char *name,
> -const char *type,
> -const void *x)
> -{
> - if (!__builtin_strcmp(type, "bool"))
> - drm_printf(p, "i915.%s=%s\n", name, yesno(*(const bool *)x));
> - else if (!__builtin_strcmp(type, "int"))
> - drm_printf(p, "i915.%s=%d\n", name, *(const int *)x);
> - else if (!__builtin_strcmp(type, "unsigned int"))
> - drm_printf(p, "i915.%s=%u\n", name, *(const unsigned int *)x);
> - else if (!__builtin_strcmp(type, "char *"))
> - drm_printf(p, "i915.%s=%s\n", name, *(const char **)x);
> - else
> - BUILD_BUG();
> -}
> +#define _print_param(p, name, type, x)
> \
> +do {
> \
> + if (!__builtin_strcmp(type, "bool"))
> \
> + drm_printf(p, "i915.%s=%s\n", name, yesno(*(const bool *)x));
> \
> + else if (!__builtin_strcmp(type, "int"))
> \
> + drm_printf(p, "i915.%s=%d\n", name, *(const int *)x);
> \
> + else if (!__builtin_strcmp(type, "unsigned int"))
> \
> + drm_printf(p, "i915.%s=%u\n", name, *(const unsigned int
> *)x); \
> + else if (!__builtin_strcmp(type, "char *"))
> \
> + drm_printf(p, "i915.%s=%s\n", name, *(const char **)x);
> \
> + else
> \
> + BUILD_BUG();
> \
> +} while (0)
>
> /**
> * i915_params_dump - dump i915 modparams
> --
> 2.19.0
>
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/amd/display: Use proper enums in process_channel_reply
On Thu, Sep 27, 2018 at 11:08 AM Nathan Chancellor
wrote:
>
> On Thu, Sep 27, 2018 at 11:06:33AM -0700, Nathan Chancellor wrote:
> > Clang warns when one enumerated type is implicitly converted to another.
> >
> > drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning:
> > implicit conversion from enumeration type 'enum
> > aux_channel_operation_result' to different enumeration type 'enum
> > aux_transaction_reply' [-Wenum-conversion]
> > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> > ~ ^~~
> > drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19:
> > warning: implicit conversion from enumeration type 'enum
> > aux_channel_operation_result' to different enumeration type 'enum
> > aux_transaction_reply' [-Wenum-conversion]
> > reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> > ~ ^~~
> >
> > The current enum is incorrect, it should be from aux_transaction_reply,
> > so use AUX_TRANSACTION_REPLY_HPD_DISCON.
> >
> > Reported-by: Nick Desaulniers
> > Suggested-by: Nick Desaulniers
> > Signed-off-by: Nathan Chancellor
Reviewed-by: Nick Desaulniers
> > ---
> >
> > v1 -> v2:
> >
> > * Rather than change status to an integer, use the proper enumerated
> > type from aux_transaction reply as suggested by Nick and confirmed
> > by Henry.
>
> Sigh Harry, sorry...
Thanks for the patch, Nathan. Don't worry about sending a v3 over
this; its below the commit line so this detail wont get committed.
>
> >
> > drivers/gpu/drm/amd/display/dc/dce/dce_aux.c| 2 +-
> > .../gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c| 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
> > b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
> > index 3f5b2e6f7553..aaeb7faac0c4 100644
> > --- a/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
> > +++ b/drivers/gpu/drm/amd/display/dc/dce/dce_aux.c
> > @@ -312,7 +312,7 @@ static void process_channel_reply(
> >
> > /* in case HPD is LOW, exit AUX transaction */
> > if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) {
> > - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> > + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
> > return;
> > }
> >
> > diff --git
> > a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
> > b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
> > index 8eee8ace1259..59c3ed43d609 100644
> > --- a/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
> > +++ b/drivers/gpu/drm/amd/display/dc/i2caux/dce110/aux_engine_dce110.c
> > @@ -346,7 +346,7 @@ static void process_channel_reply(
> >
> > /* in case HPD is LOW, exit AUX transaction */
> > if ((sw_status & AUX_SW_STATUS__AUX_SW_HPD_DISCON_MASK)) {
> > - reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> > + reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
> > return;
> > }
> >
> > --
> > 2.19.0
> >
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/powerplay: Change id parameter type in pp_atomfwctrl_get_clk_information_by_clkid
On Fri, Sep 21, 2018 at 2:01 PM Nathan Chancellor
wrote:
>
> Clang generates warnings when one enumerated type is implicitly
> converted to another.
>
> drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/ppatomfwctrl.c:532:57:
> warning: implicit conversion from enumeration type 'enum
> atom_smu11_syspll0_clock_id' to different enumeration type 'BIOS_CLKID'
> (aka 'enum atom_smu9_syspll0_clock_id') [-Wenum-conversion]
> if (!pp_atomfwctrl_get_clk_information_by_clkid(hwmgr,
> SMU11_SYSPLL0_SOCCLK_ID, ))
Grepping the call sites, via:
$ grep -r pp_atomfwctrl_get_clk_information_by_clkid
shows that sometimes this function is called with instances of:
enum atom_smu9_syspll0_clock_id
enum atom_smu11_syspll0_clock_id
before your patch, the function defined to take a BIOS_CLKID, which is
typedef'd to the *9* enum:
typedef enum atom_smu9_syspll0_clock_id BIOS_CLKID;
While the current values are < 256, the enums are not packed, so you
should use either int or a union of the two types.
Do the maintainers have a preference, before sending a v2?
>
> In this case, that is expected behavior. To make that clear to Clang
> without explicitly casting these values, change id's type to uint8_t
> in pp_atomfwctrl_get_clk_information_by_clkid so no conversion happens.
>
> Reported-by: Nick Desaulniers
> Signed-off-by: Nathan Chancellor
> ---
> drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.c | 3 ++-
> drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.h | 2 +-
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.c
> index d27c1c9df286..4588bddf8b33 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.c
> @@ -488,7 +488,8 @@ int pp_atomfwctrl_get_gpio_information(struct pp_hwmgr
> *hwmgr,
> return 0;
> }
>
> -int pp_atomfwctrl_get_clk_information_by_clkid(struct pp_hwmgr *hwmgr,
> BIOS_CLKID id, uint32_t *frequency)
> +int pp_atomfwctrl_get_clk_information_by_clkid(struct pp_hwmgr *hwmgr,
> + uint8_t id, uint32_t
> *frequency)
> {
> struct amdgpu_device *adev = hwmgr->adev;
> struct atom_get_smu_clock_info_parameters_v3_1 parameters;
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.h
> b/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.h
> index 22e21668c93a..fe9e8ceef50e 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.h
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/ppatomfwctrl.h
> @@ -236,7 +236,7 @@ int pp_atomfwctrl_get_vbios_bootup_values(struct pp_hwmgr
> *hwmgr,
> int pp_atomfwctrl_get_smc_dpm_information(struct pp_hwmgr *hwmgr,
> struct pp_atomfwctrl_smc_dpm_parameters *param);
> int pp_atomfwctrl_get_clk_information_by_clkid(struct pp_hwmgr *hwmgr,
> - BIOS_CLKID id, uint32_t *frequency);
> + uint8_t id, uint32_t *frequency);
>
> #endif
>
> --
> 2.19.0
>
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/display: Change status's type in aux_reply_transaction_data
On Fri, Sep 21, 2018 at 2:55 PM Nathan Chancellor
wrote:
>
> Clang warns when one enumerated type is implicitly converted to another.
>
> drivers/gpu/drm/amd/amdgpu/../display/dc/dce/dce_aux.c:315:19: warning:
> implicit conversion from enumeration type 'enum
> aux_channel_operation_result' to different enumeration type 'enum
> aux_transaction_reply' [-Wenum-conversion]
> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> ~ ^~~
> drivers/gpu/drm/amd/amdgpu/../display/dc/i2caux/dce110/aux_engine_dce110.c:349:19:
> warning: implicit conversion from enumeration type 'enum
> aux_channel_operation_result' to different enumeration type 'enum
> aux_transaction_reply' [-Wenum-conversion]
> reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
> ~ ^~~
I think the enum is actually wrong here. I think the correct fix would be:
- reply->status = AUX_CHANNEL_OPERATION_FAILED_HPD_DISCON;
+ reply->status = AUX_TRANSACTION_REPLY_HPD_DISCON;
The identifiers are so similar, my guess was that it was easy to mix
them up. This looks like an actual bug to me, since the identifiers
have different values between the 2 different enums.
>
> Instead of implicitly or explicitly converting between types, just
> change status to type uint8_t (since its max size is 255) which avoids
> this construct altogether.
>
> Reported-by: Nick Desaulniers
> Signed-off-by: Nathan Chancellor
> ---
> drivers/gpu/drm/amd/display/dc/dc_ddc_types.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> index 05c8c31d8b31..97e1d4d19263 100644
> --- a/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> +++ b/drivers/gpu/drm/amd/display/dc/dc_ddc_types.h
> @@ -79,7 +79,7 @@ enum aux_transaction_reply {
> };
>
> struct aux_reply_transaction_data {
> - enum aux_transaction_reply status;
> + uint8_t status;
> uint32_t length;
> uint8_t *data;
> };
> --
> 2.19.0
>
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] backlight: lm3639: Unconditionally call led_classdev_unregister
On Fri, Sep 21, 2018 at 4:10 PM Nathan Chancellor
wrote:
>
> On Fri, Sep 21, 2018 at 03:48:50PM -0700, Nick Desaulniers wrote:
> > On Fri, Sep 21, 2018 at 1:23 PM Nathan Chancellor
> > wrote:
> > >
> > > Clang warns that the address of a pointer will always evaluated as true
> > > in a boolean context.
> > >
> > > drivers/video/backlight/lm3639_bl.c:403:14: warning: address of
> > > 'pchip->cdev_torch' will always evaluate to 'true'
> > > [-Wpointer-bool-conversion]
> > > if (>cdev_torch)
> > > ~~ ~~~^~
> > > drivers/video/backlight/lm3639_bl.c:405:14: warning: address of
> > > 'pchip->cdev_flash' will always evaluate to 'true'
> > > [-Wpointer-bool-conversion]
> > > if (>cdev_flash)
> > > ~~ ~~~^~
> > > 2 warnings generated.
> > >
> > > These statements have been present since 2012, introduced by
> > > commit 0f59858d5119 ("backlight: add new lm3639 backlight
> > > driver"). Given that they have been called unconditionally since
> > > then presumably without any issues, removing the always true if
> > > statements to fix the warnings without any real world changes.
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/119
> > > Signed-off-by: Nathan Chancellor
> > > ---
> > >
> > > Alternatively, it's possible the address wasn't supposed to be taken or
> > > the dev in these structs should be checked instead. I don't have this
> > > hardware to make that call so I would appreciate some review and
> > > opinions on what was intended here.
> > >
> > > Thanks!
> > >
> > > drivers/video/backlight/lm3639_bl.c | 6 ++
> > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/video/backlight/lm3639_bl.c
> > > b/drivers/video/backlight/lm3639_bl.c
> > > index cd50df5807ea..086611c7bc03 100644
> > > --- a/drivers/video/backlight/lm3639_bl.c
> > > +++ b/drivers/video/backlight/lm3639_bl.c
> > > @@ -400,10 +400,8 @@ static int lm3639_remove(struct i2c_client *client)
> > >
> > > regmap_write(pchip->regmap, REG_ENABLE, 0x00);
> > >
> > > - if (>cdev_torch)
> > > - led_classdev_unregister(>cdev_torch);
> > > - if (>cdev_flash)
> > > - led_classdev_unregister(>cdev_flash);
> > > + led_classdev_unregister(>cdev_torch);
> > > + led_classdev_unregister(>cdev_flash);
> >
> > led_classdev_unregister() requires that its arg is non-null (as it
> > dereferences it without any kind of check). It's not clear that
> > i2c_get_clientdata() can never return a null pointer, so I think all
> > references to pchip in this function should instead be guarded with a
> > null check. Would you mind making that change and sending a v2?
> >
>
> Hi Nick,
>
> I did a quick grep throughout the tree and I didn't see any place where
> there were null checks for i2c_get_clientdata, leading me to believe
> that such a check isn't necessary although I am nowhere close to an expert
> into this stuff.
This seems to be the case. We should start using
__attribute__((returns_nonnull)) (gated on gcc 5+).
I *think* that the device's driver_data is actually set in
drivers/video/backlight/backlight.c. Looks like
CONFIG_BACKLIGHT_LM3639 depends on CONFIG_BACKLIGHT_CLASS_DEVICE so I
feel more confident in your patch.
I would still prefer the maintainers to review though.
> I'm not sure I follow the rest of the request though,
> where should the check be? Before regmap_write?
>
> Furthermore, the probe function seems to make sure all of these get
> initialized properly, doesn't remove imply that probe was successful?
>
> Thank you for the comment and review!
> Nathan
>
> > > if (pchip->bled)
> > > device_remove_file(&(pchip->bled->dev),
> > > _attr_bled_mode);
> > > return 0;
> > > --
> > > 2.19.0
> > >
> >
> >
> > --
> > Thanks,
> > ~Nick Desaulniers
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] backlight: lm3639: Unconditionally call led_classdev_unregister
On Fri, Sep 21, 2018 at 1:23 PM Nathan Chancellor
wrote:
>
> Clang warns that the address of a pointer will always evaluated as true
> in a boolean context.
>
> drivers/video/backlight/lm3639_bl.c:403:14: warning: address of
> 'pchip->cdev_torch' will always evaluate to 'true'
> [-Wpointer-bool-conversion]
> if (>cdev_torch)
> ~~ ~~~^~
> drivers/video/backlight/lm3639_bl.c:405:14: warning: address of
> 'pchip->cdev_flash' will always evaluate to 'true'
> [-Wpointer-bool-conversion]
> if (>cdev_flash)
> ~~ ~~~^~
> 2 warnings generated.
>
> These statements have been present since 2012, introduced by
> commit 0f59858d5119 ("backlight: add new lm3639 backlight
> driver"). Given that they have been called unconditionally since
> then presumably without any issues, removing the always true if
> statements to fix the warnings without any real world changes.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/119
> Signed-off-by: Nathan Chancellor
> ---
>
> Alternatively, it's possible the address wasn't supposed to be taken or
> the dev in these structs should be checked instead. I don't have this
> hardware to make that call so I would appreciate some review and
> opinions on what was intended here.
>
> Thanks!
>
> drivers/video/backlight/lm3639_bl.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/backlight/lm3639_bl.c
> b/drivers/video/backlight/lm3639_bl.c
> index cd50df5807ea..086611c7bc03 100644
> --- a/drivers/video/backlight/lm3639_bl.c
> +++ b/drivers/video/backlight/lm3639_bl.c
> @@ -400,10 +400,8 @@ static int lm3639_remove(struct i2c_client *client)
>
> regmap_write(pchip->regmap, REG_ENABLE, 0x00);
>
> - if (>cdev_torch)
> - led_classdev_unregister(>cdev_torch);
> - if (>cdev_flash)
> - led_classdev_unregister(>cdev_flash);
> + led_classdev_unregister(>cdev_torch);
> + led_classdev_unregister(>cdev_flash);
led_classdev_unregister() requires that its arg is non-null (as it
dereferences it without any kind of check). It's not clear that
i2c_get_clientdata() can never return a null pointer, so I think all
references to pchip in this function should instead be guarded with a
null check. Would you mind making that change and sending a v2?
> if (pchip->bled)
> device_remove_file(&(pchip->bled->dev), _attr_bled_mode);
> return 0;
> --
> 2.19.0
>
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects
On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
wrote:
>
> Clang warns if there are missing braces around a subobject
> initializer.
>
> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
> around initialization of subobject [-Wmissing-braces]
> struct amdgpu_task_info task_info = { 0 };
> ^
> {}
> 1 warning generated.
>
> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
> around initialization of subobject [-Wmissing-braces]
> struct amdgpu_task_info task_info = { 0 };
> ^
> {}
> 1 warning generated.
>
> Reported-by: Nick Desaulniers
> Signed-off-by: Nathan Chancellor
> ---
> drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> index 9333109b210d..968cc1b8cdff 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
> @@ -1444,7 +1444,7 @@ static int gmc_v8_0_process_interrupt(struct
> amdgpu_device *adev,
> gmc_v8_0_set_fault_enable_default(adev, false);
>
> if (printk_ratelimit()) {
> - struct amdgpu_task_info task_info = { 0 };
> + struct amdgpu_task_info task_info = { { 0 } };
Hi Nathan,
Thanks for this patch. I discussed this syntax with our language
lawyers. Turns out, this is not quite correct, as you're now saying
"initialize the first subobject to zero, but not the rest of the
object." -Wmissing-field-initializers would highlight this, but it's
not part of -Wall. It would be more correct to zero initialize the
full struct, including all of its subobjects with `= {};`.
>
> amdgpu_vm_get_task_info(adev, entry->pasid, _info);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 72f8018fa2a8..a781a5027212 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -259,7 +259,7 @@ static int gmc_v9_0_process_interrupt(struct
> amdgpu_device *adev,
> }
>
> if (printk_ratelimit()) {
> - struct amdgpu_task_info task_info = { 0 };
> + struct amdgpu_task_info task_info = { { 0 } };
>
> amdgpu_vm_get_task_info(adev, entry->pasid, _info);
>
> --
> 2.18.0
>
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu: Add braces to initialize task_info subojects
On Wed, Sep 12, 2018 at 1:24 PM Richard Smith wrote:
>
> On Wed, Sep 12, 2018 at 10:38 AM Nick Desaulniers
> wrote:
>>
>> On Tue, Sep 11, 2018 at 5:26 PM Nathan Chancellor
>> wrote:
>> >
>> > Clang warns if there are missing braces around a subobject
>> > initializer.
>> >
>> > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c:1447:41: warning: suggest braces
>> > around initialization of subobject [-Wmissing-braces]
>> > struct amdgpu_task_info task_info = { 0 };
>> > ^
>> > {}
>> > 1 warning generated.
>> >
>> > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c:262:41: warning: suggest braces
>> > around initialization of subobject [-Wmissing-braces]
>> > struct amdgpu_task_info task_info = { 0 };
>> > ^
>> > {}
>> > 1 warning generated.
>> >
>> > Reported-by: Nick Desaulniers
>> > Signed-off-by: Nathan Chancellor
>> > ---
>> > drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c | 2 +-
>> > drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 2 +-
>> > 2 files changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> > b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> > index 9333109b210d..968cc1b8cdff 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>> > @@ -1444,7 +1444,7 @@ static int gmc_v8_0_process_interrupt(struct
>> > amdgpu_device *adev,
>> > gmc_v8_0_set_fault_enable_default(adev, false);
>> >
>> > if (printk_ratelimit()) {
>> > - struct amdgpu_task_info task_info = { 0 };
>> > + struct amdgpu_task_info task_info = { { 0 } };
>>
>> Hi Nathan,
>> Thanks for this patch. I discussed this syntax with our language
>> lawyers. Turns out, this is not quite correct, as you're now saying
>> "initialize the first subobject to zero, but not the rest of the
>> object." -Wmissing-field-initializers would highlight this, but it's
>> not part of -Wall. It would be more correct to zero initialize the
>> full struct, including all of its subobjects with `= {};`.
>
>
> Sorry, I think I've caused some confusion here.
>
> Elements with an omitted initializer get implicitly zero-initialized. In C++,
> it's idiomatic to write `= {}` to perform aggregate zero-initialization, but
> in C, that's invalid because at least one initializer is syntactically
> required within the braces. As a result, `= {0}` is an idiomatic way to
> perform zero-initialization of an aggregate in C.
That doesn't seem to be the case:
https://godbolt.org/z/TZzfo6 shouldn't Clang warn in the case of bar()?
> Clang intends to suppress the -Wmissing-braces in that case; if the warning
> is still being produced in a recent version of Clang, that's a bug. However,
> the warning suppression was added between Clang 5 and Clang 6, so it's very
> plausible that the compiler being used here is simply older than the warning
> fix.
>
> (Long story short: the change here seems fine, but should be unnecessary as
> of Clang 6.)
The warning was identified from clang-8 ToT synced yesterday.
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/panel: simple: fix -Wliteral-conversion warning
On Thu, Aug 9, 2018 at 3:00 AM Thierry Reding wrote:
>
> On Fri, Jun 15, 2018 at 03:37:49PM -0700, Nick Desaulniers wrote:
> > Fixes commit 8cfe83419cdb ("drm/panel: simple: Add
> > support for KEO TX31D200VM0BAA")
> >
> > drivers/gpu/drm/panel/panel-simple.c:1250:27: warning: implicit conversion
> > from
> > 'double' to 'u32' (aka 'unsigned int') changes value from 33.5 to 33
> > [-Wliteral-conversion]
> > .vfront_porch = { 6, 21, 33.5 },
> > ~^~~~
> > drivers/gpu/drm/panel/panel-simple.c:1251:26: warning: implicit conversion
> > from
> > 'double' to 'u32' (aka 'unsigned int') changes value from 33.5 to 33
> > [-Wliteral-conversion]
> > .vback_porch = { 6, 21, 33.5 },
> >~^~~~
> >
> > Signed-off-by: Nick Desaulniers
> > ---
> > Alternatively, should these be rounded up to 34? I'm guessing the
> > current behaviour (truncation) is correct since that's how the patch was
> > operating.
>
> Looks like this is a duplicate of what was merged as:
>
> commit c9b6be7dc13e2f87592ee4c9812cb450dba484d5
> Author: Stefan Agner
> Date: Thu Apr 19 23:20:03 2018 +0200
Ah, cool. I need to be better about checking maintainers' trees.
Just curious, how come a patch from April hasn't hit torvalds/linux yet?
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/panel: simple: fix -Wliteral-conversion warning
bumping for review.
On Fri, Jun 15, 2018 at 3:38 PM Nick Desaulniers
wrote:
>
> Fixes commit 8cfe83419cdb ("drm/panel: simple: Add
> support for KEO TX31D200VM0BAA")
>
> drivers/gpu/drm/panel/panel-simple.c:1250:27: warning: implicit conversion
> from
> 'double' to 'u32' (aka 'unsigned int') changes value from 33.5 to 33
> [-Wliteral-conversion]
> .vfront_porch = { 6, 21, 33.5 },
> ~^~~~
> drivers/gpu/drm/panel/panel-simple.c:1251:26: warning: implicit conversion
> from
> 'double' to 'u32' (aka 'unsigned int') changes value from 33.5 to 33
> [-Wliteral-conversion]
> .vback_porch = { 6, 21, 33.5 },
> ~^~~~
>
> Signed-off-by: Nick Desaulniers
> ---
> Alternatively, should these be rounded up to 34? I'm guessing the
> current behaviour (truncation) is correct since that's how the patch was
> operating.
>
> drivers/gpu/drm/panel/panel-simple.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index cbf1ab404ee7..12bcbd1dd77b 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1247,8 +1247,8 @@ static const struct display_timing
> koe_tx31d200vm0baa_timing = {
> .hback_porch = { 16, 36, 56 },
> .hsync_len = { 8, 8, 8 },
> .vactive = { 480, 480, 480 },
> - .vfront_porch = { 6, 21, 33.5 },
> - .vback_porch = { 6, 21, 33.5 },
> + .vfront_porch = { 6, 21, 33 },
> + .vback_porch = { 6, 21, 33 },
> .vsync_len = { 8, 8, 8 },
> .flags = DISPLAY_FLAGS_DE_HIGH,
> };
> --
> 2.18.0.rc1.244.gcf134e6275-goog
>
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/nouveau/nvif: remove const attribute from nvif_mclass
Similar to commit 0bf8bf50eddc ("module: Remove
const attribute from alias for MODULE_DEVICE_TABLE")
Fixes many -Wduplicate-decl-specifier warnings due to the combination of
const typeof() of already const variables.
Signed-off-by: Nick Desaulniers
---
Changes since v1: added additional space after statements.
drivers/gpu/drm/nouveau/include/nvif/object.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvif/object.h
b/drivers/gpu/drm/nouveau/include/nvif/object.h
index a2d5244ff2b7..3d7e485a9043 100644
--- a/drivers/gpu/drm/nouveau/include/nvif/object.h
+++ b/drivers/gpu/drm/nouveau/include/nvif/object.h
@@ -78,7 +78,7 @@ struct nvif_mclass {
#define nvif_mclass(o,m) ({
\
struct nvif_object *object = (o); \
struct nvif_sclass *sclass;\
- const typeof(m[0]) *mclass = (m); \
+ typeof(m[0]) *mclass = (m);\
int ret = -ENODEV; \
int cnt, i, j; \
\
--
2.18.0.rc1.244.gcf134e6275-goog
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/nouveau/nvif: remove const attribute from nvif_mclass
On Fri, Jun 15, 2018 at 3:56 PM Nick Desaulniers
wrote:
>
> Similar to commit 0bf8bf50eddc ("module: Remove
> const attribute from alias for MODULE_DEVICE_TABLE")
>
> Fixes many -Wduplicate-decl-specifier warnings due to the combination of
> const typeof() of already const variables.
>
> Signed-off-by: Nick Desaulniers
> ---
> drivers/gpu/drm/nouveau/include/nvif/object.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/include/nvif/object.h
> b/drivers/gpu/drm/nouveau/include/nvif/object.h
> index a2d5244ff2b7..7f188f66931e 100644
> --- a/drivers/gpu/drm/nouveau/include/nvif/object.h
> +++ b/drivers/gpu/drm/nouveau/include/nvif/object.h
> @@ -78,7 +78,7 @@ struct nvif_mclass {
> #define nvif_mclass(o,m) ({
> \
> struct nvif_object *object = (o);
> \
> struct nvif_sclass *sclass;
> \
> - const typeof(m[0]) *mclass = (m);
> \
> + typeof(m[0]) *mclass = (m); \
Sorry, this messes up the spaces at the end, will send a v2.
> int ret = -ENODEV;
> \
> int cnt, i, j;
> \
>
> \
> --
> 2.18.0.rc1.244.gcf134e6275-goog
>
--
Thanks,
~Nick Desaulniers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/panel: simple: fix -Wliteral-conversion warning
Fixes commit 8cfe83419cdb ("drm/panel: simple: Add
support for KEO TX31D200VM0BAA")
drivers/gpu/drm/panel/panel-simple.c:1250:27: warning: implicit conversion from
'double' to 'u32' (aka 'unsigned int') changes value from 33.5 to 33
[-Wliteral-conversion]
.vfront_porch = { 6, 21, 33.5 },
~^~~~
drivers/gpu/drm/panel/panel-simple.c:1251:26: warning: implicit conversion from
'double' to 'u32' (aka 'unsigned int') changes value from 33.5 to 33
[-Wliteral-conversion]
.vback_porch = { 6, 21, 33.5 },
~^~~~
Signed-off-by: Nick Desaulniers
---
Alternatively, should these be rounded up to 34? I'm guessing the
current behaviour (truncation) is correct since that's how the patch was
operating.
drivers/gpu/drm/panel/panel-simple.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c
b/drivers/gpu/drm/panel/panel-simple.c
index cbf1ab404ee7..12bcbd1dd77b 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1247,8 +1247,8 @@ static const struct display_timing
koe_tx31d200vm0baa_timing = {
.hback_porch = { 16, 36, 56 },
.hsync_len = { 8, 8, 8 },
.vactive = { 480, 480, 480 },
- .vfront_porch = { 6, 21, 33.5 },
- .vback_porch = { 6, 21, 33.5 },
+ .vfront_porch = { 6, 21, 33 },
+ .vback_porch = { 6, 21, 33 },
.vsync_len = { 8, 8, 8 },
.flags = DISPLAY_FLAGS_DE_HIGH,
};
--
2.18.0.rc1.244.gcf134e6275-goog
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/nouveau/nvif: remove const attribute from nvif_mclass
Similar to commit 0bf8bf50eddc ("module: Remove
const attribute from alias for MODULE_DEVICE_TABLE")
Fixes many -Wduplicate-decl-specifier warnings due to the combination of
const typeof() of already const variables.
Signed-off-by: Nick Desaulniers
---
drivers/gpu/drm/nouveau/include/nvif/object.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvif/object.h
b/drivers/gpu/drm/nouveau/include/nvif/object.h
index a2d5244ff2b7..7f188f66931e 100644
--- a/drivers/gpu/drm/nouveau/include/nvif/object.h
+++ b/drivers/gpu/drm/nouveau/include/nvif/object.h
@@ -78,7 +78,7 @@ struct nvif_mclass {
#define nvif_mclass(o,m) ({
\
struct nvif_object *object = (o); \
struct nvif_sclass *sclass;\
- const typeof(m[0]) *mclass = (m); \
+ typeof(m[0]) *mclass = (m); \
int ret = -ENODEV; \
int cnt, i, j; \
\
--
2.18.0.rc1.244.gcf134e6275-goog
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] video/hdmi: prefer strlcpy to strncpy
On Mon, May 28, 2018 at 11:59 PM, Nick Desaulniers wrote: > Fixes a stringop-truncation warning from gcc-8. > > Signed-off-by: Nick Desaulniers > --- > drivers/video/hdmi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > index 111a0ab..46c184c 100644 > --- a/drivers/video/hdmi.c > +++ b/drivers/video/hdmi.c > @@ -168,8 +168,8 @@ int hdmi_spd_infoframe_init(struct hdmi_spd_infoframe > *frame, > frame->version = 1; > frame->length = HDMI_SPD_INFOFRAME_SIZE; > > - strncpy(frame->vendor, vendor, sizeof(frame->vendor)); > - strncpy(frame->product, product, sizeof(frame->product)); > + strlcpy(frame->vendor, vendor, sizeof(frame->vendor)); > + strlcpy(frame->product, product, sizeof(frame->product)); > > return 0; > } > -- > 2.7.4 > Eric points out this can leak kernel memory if size is less than length of src. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] video/hdmi: prefer strlcpy to strncpy
Fixes a stringop-truncation warning from gcc-8. Signed-off-by: Nick Desaulniers --- drivers/video/hdmi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index 111a0ab..46c184c 100644 --- a/drivers/video/hdmi.c +++ b/drivers/video/hdmi.c @@ -168,8 +168,8 @@ int hdmi_spd_infoframe_init(struct hdmi_spd_infoframe *frame, frame->version = 1; frame->length = HDMI_SPD_INFOFRAME_SIZE; - strncpy(frame->vendor, vendor, sizeof(frame->vendor)); - strncpy(frame->product, product, sizeof(frame->product)); + strlcpy(frame->vendor, vendor, sizeof(frame->vendor)); + strlcpy(frame->product, product, sizeof(frame->product)); return 0; } -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/i915: Avoid enum conversion warning
Fixes the following enum conversion warning:
drivers/gpu/drm/i915/intel_ddi.c:1481:30: error: implicit conversion
from enumeration type 'enum port' to different enumeration type 'enum
intel_dpll_id' [-Werror,-Wenum-conversion]
enum intel_dpll_id pll_id = port;
~~ ^~~~
Suggested-by: Daniel Vetter <dan...@ffwll.ch>
Signed-off-by: Nick Desaulniers <nick.desaulni...@gmail.com>
---
drivers/gpu/drm/i915/intel_ddi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 933c18fd4258..3c346c8cbf78 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1477,8 +1477,8 @@ static void bxt_ddi_clock_get(struct intel_encoder
*encoder,
struct intel_crtc_state *pipe_config)
{
struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
- enum port port = intel_ddi_get_encoder_port(encoder);
- enum intel_dpll_id pll_id = port;
+ enum intel_dpll_id pll_id =
+ (enum intel_dpll_id)intel_ddi_get_encoder_port(encoder);
pipe_config->port_clock = bxt_calc_pll_link(dev_priv, pll_id);
--
2.14.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915: Avoid enum conversion warning
Fixes the following enum conversion warning:
drivers/gpu/drm/i915/intel_ddi.c:1481:30: error: implicit conversion
from enumeration type 'enum port' to different enumeration type 'enum
intel_dpll_id' [-Werror,-Wenum-conversion]
enum intel_dpll_id pll_id = port;
~~ ^~~~
Signed-off-by: Nick Desaulniers <nick.desaulni...@gmail.com>
---
drivers/gpu/drm/i915/intel_ddi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 933c18fd4258..f9de45316901 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1478,7 +1478,7 @@ static void bxt_ddi_clock_get(struct intel_encoder
*encoder,
{
struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
enum port port = intel_ddi_get_encoder_port(encoder);
- enum intel_dpll_id pll_id = port;
+ uint32_t pll_id = port;
pipe_config->port_clock = bxt_calc_pll_link(dev_priv, pll_id);
--
2.14.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915: Mark wait_for_engine() as maybe_unused
Signed-off-by: Nick Desaulniers <nick.desaulni...@gmail.com> ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i915/gvt: remove redundant -Wall
ping for review ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915/gvt: remove redundant -Wall
This flag is already set in the top level Makefile of the kernel. Also, by having set CONFIG_DRM_I915_GVT, thereby appending -Wall to ccflags, you undo all the -Wno-* cflags previously set in the Make variable KBUILD_CFLAGS. For example: cc foo.c -Wall -Wno-format -Wall resets -Wformat. Signed-off-by: Nick Desaulniers <nick.desaulni...@gmail.com> --- I verified that -Wall was redundant by compiling with V=1: make V=1 -j4 drivers/gpu/drm/i915/i915_gem_request.o drivers/gpu/drm/i915/gvt/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile index b123c20e2097..f5486cb94818 100644 --- a/drivers/gpu/drm/i915/gvt/Makefile +++ b/drivers/gpu/drm/i915/gvt/Makefile @@ -3,6 +3,6 @@ GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \ interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \ execlist.o scheduler.o sched_policy.o render.o cmd_parser.o -ccflags-y += -I$(src) -I$(src)/$(GVT_DIR) -Wall +ccflags-y += -I$(src) -I$(src)/$(GVT_DIR) i915-y += $(addprefix $(GVT_DIR)/, $(GVT_SOURCE)) obj-$(CONFIG_DRM_I915_GVT_KVMGT) += $(GVT_DIR)/kvmgt.o -- 2.11.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/i915: mark wait_for_engine() __maybe_unused
This solves a warning when compiling the driver with Clang, -Werror enabled,
and CONFIG_DRM_I915_DEBUG_GEM unset, since Clang warns that:
drivers/gpu/drm/i915/i915_gem.c:3274:12: error: function
'wait_for_engine' is not needed
and will not be emitted [-Werror,-Wunneeded-internal-declaration]
static int wait_for_engine(struct intel_engine_cs *engine, int
timeout_ms)
^
Signed-off-by: Nick Desaulniers <nick.desaulni...@gmail.com>
---
Additionally, it only has one call site. Should I mark it inline, too, while
I'm at it?
drivers/gpu/drm/i915/i915_gem.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b6ac3df18b58..73b82fb94b0e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3271,7 +3271,8 @@ static int wait_for_timeline(struct i915_gem_timeline
*tl, unsigned int flags)
return 0;
}
-static int wait_for_engine(struct intel_engine_cs *engine, int timeout_ms)
+static __maybe_unused int wait_for_engine(
+ struct intel_engine_cs *engine, int timeout_ms)
{
return wait_for(intel_engine_is_idle(engine), timeout_ms);
}
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/mm: fix duplicate 'const' declaration specifier
ah seems like there's more of these: drivers/gpu/drm/drm_mm.c:922 surprised compiling drivers/gpu/drm/drm_mm.o did not catch this the first time... ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/mm: fix duplicate 'const' declaration specifier
Please disregard this patch. I think I may have found a bug in Clang, or at least an incompatibility with GCC 7.1. Clang bug: https://bugs.llvm.org/show_bug.cgi?id=32985 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/mm: fix duplicate 'const' declaration specifier
Found with -Wduplicate-decl-specifier, a relatively new compiler flag in
GCC7, and Clang.
list_for_each_entry() eventually calls container_of(), which marks the
loop variable as const. The first argument to list_for_each_entry() is
a type, which should not already be marked const, otherwise the loop
variable is marked const twice.
While this particular call site does not modify the loop variable,
trying to do so would already result in a compile time failure, so we
can remove the current const. Other call sites do not mark the loop
variable const.
Signed-off-by: Nick Desaulniers <nick.desaulni...@gmail.com>
---
include/drm/drm_mm.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h
index 49b292e98fec..6716af9290c9 100644
--- a/include/drm/drm_mm.h
+++ b/include/drm/drm_mm.h
@@ -281,7 +281,7 @@ static inline u64 drm_mm_hole_node_start(const struct
drm_mm_node *hole_node)
return __drm_mm_hole_node_start(hole_node);
}
-static inline u64 __drm_mm_hole_node_end(const struct drm_mm_node *hole_node)
+static inline u64 __drm_mm_hole_node_end(struct drm_mm_node *hole_node)
{
return list_next_entry(hole_node, node_list)->start;
}
@@ -297,7 +297,7 @@ static inline u64 __drm_mm_hole_node_end(const struct
drm_mm_node *hole_node)
* Returns:
* End of the subsequent hole.
*/
-static inline u64 drm_mm_hole_node_end(const struct drm_mm_node *hole_node)
+static inline u64 drm_mm_hole_node_end(struct drm_mm_node *hole_node)
{
return __drm_mm_hole_node_end(hole_node);
}
--
2.11.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
