[Intel-xe] [PATCH 0/3] drm/xe: Fix building with clang

[Intel-xe] [PATCH 0/3] drm/xe: Fix building with clang

[Michał Winiarski]

Clang uses a different set of warnings than GCC, some of which are
causing Xe build failures.
Let's fix it.

Michał Winiarski (3):
  drm/xe: Fix header guard warning
  drm/i915: Fix uninitialized variable warning
  drm/xe: Fix format string security warning

 drivers/gpu/drm/i915/display/intel_dsb.c      | 12 ++++++------
 drivers/gpu/drm/xe/xe_gt_idle_sysfs.c         |  2 +-
 drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.h |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

-- 
2.42.0

[Intel-xe] [PATCH 1/3] drm/xe: Fix header guard warning

[Michał Winiarski]

Additional underscore in the header guard causes the build to fail with:

drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.h:6:9: error: '_XE_ENGINE_CLASS_SYSFS_H_' is used as a header guard here, followed by #define of a different macro [-Werror,-Wheader-guard]

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.h b/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.h
index 60469fde41478..ec5ba673b314b 100644
--- a/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.h
+++ b/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.h
@@ -4,7 +4,7 @@
  */
 
 #ifndef _XE_ENGINE_CLASS_SYSFS_H_
-#define _XE_ENGINE_CLASS_SYSFS_H__
+#define _XE_ENGINE_CLASS_SYSFS_H_
 
 #include <linux/kobject.h>
 
-- 
2.42.0

[Intel-xe] [PATCH 2/3] drm/i915: Fix uninitialized variable warning

[Michał Winiarski]

When used with Xe, "val" uses as function argument name clashes with
"val" used inside iosys_map_* macro causing the build to fail with:

drivers/gpu/drm/i915/display/intel_dsb.c:87:46: error: variable 'val' is uninitialized when used within its own initialization [-Werror,-Wuninitialized]

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 796b1b36dc598..8d89c8f700d68 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -79,12 +79,12 @@ static u32 dsb_ggtt_offset(struct intel_dsb *dsb)
 #endif
 }
 
-static void dsb_write(struct intel_dsb *dsb, u32 idx, u32 val)
+static void dsb_write(struct intel_dsb *dsb, u32 idx, u32 value)
 {
 #ifdef I915
-	dsb->cmd_buf[idx] = val;
+	dsb->cmd_buf[idx] = value;
 #else
-	iosys_map_wr(&dsb->obj->vmap, idx * 4, u32, val);
+	iosys_map_wr(&dsb->obj->vmap, idx * 4, u32, value);
 #endif
 }
 
@@ -97,12 +97,12 @@ static u32 dsb_read(struct intel_dsb *dsb, u32 idx)
 #endif
 }
 
-static void dsb_memset(struct intel_dsb *dsb, u32 idx, u32 val, u32 sz)
+static void dsb_memset(struct intel_dsb *dsb, u32 idx, u32 value, u32 sz)
 {
 #ifdef I915
-	memset(&dsb->cmd_buf[idx], val, sz);
+	memset(&dsb->cmd_buf[idx], value, sz);
 #else
-	iosys_map_memset(&dsb->obj->vmap, idx * 4, val, sz);
+	iosys_map_memset(&dsb->obj->vmap, idx * 4, value, sz);
 #endif
 }
 
-- 
2.42.0

[Intel-xe] [PATCH 3/3] drm/xe: Fix format string security warning

[Michał Winiarski]

While "gtidle->name" is not user-controllable string, it's still a bad
practice to pass variable as a format string, it also causes the build
to fail with:

drivers/gpu/drm/xe/xe_gt_idle_sysfs.c:88:26: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/xe/xe_gt_idle_sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_gt_idle_sysfs.c b/drivers/gpu/drm/xe/xe_gt_idle_sysfs.c
index 7238e96a116cf..8df9840811cda 100644
--- a/drivers/gpu/drm/xe/xe_gt_idle_sysfs.c
+++ b/drivers/gpu/drm/xe/xe_gt_idle_sysfs.c
@@ -85,7 +85,7 @@ static ssize_t name_show(struct device *dev,
 {
 	struct xe_gt_idle *gtidle = dev_to_gtidle(dev);
 
-	return sysfs_emit(buff, gtidle->name);
+	return sysfs_emit(buff, "%s\n", gtidle->name);
 }
 static DEVICE_ATTR_RO(name);
 
-- 
2.42.0

Re: [Intel-xe] [PATCH 1/3] drm/xe: Fix header guard warning

[Rodrigo Vivi]

On Wed, Oct 18, 2023 at 01:58:24PM +0200, Michał Winiarski wrote:
> Additional underscore in the header guard causes the build to fail with:
> 
> drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.h:6:9: error: '_XE_ENGINE_CLASS_SYSFS_H_' is used as a header guard here, followed by #define of a different macro [-Werror,-Wheader-guard]
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.h b/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.h
> index 60469fde41478..ec5ba673b314b 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.h
> +++ b/drivers/gpu/drm/xe/xe_hw_engine_class_sysfs.h
> @@ -4,7 +4,7 @@
>   */
>  
>  #ifndef _XE_ENGINE_CLASS_SYSFS_H_
> -#define _XE_ENGINE_CLASS_SYSFS_H__
> +#define _XE_ENGINE_CLASS_SYSFS_H_
>  
>  #include <linux/kobject.h>
>  
> -- 
> 2.42.0
> 

Re: [Intel-xe] [PATCH 2/3] drm/i915: Fix uninitialized variable warning

[Rodrigo Vivi]

On Wed, Oct 18, 2023 at 01:58:25PM +0200, Michał Winiarski wrote:
> When used with Xe, "val" uses as function argument name clashes with
> "val" used inside iosys_map_* macro causing the build to fail with:
> 
> drivers/gpu/drm/i915/display/intel_dsb.c:87:46: error: variable 'val' is uninitialized when used within its own initialization [-Werror,-Wuninitialized]
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 796b1b36dc598..8d89c8f700d68 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -79,12 +79,12 @@ static u32 dsb_ggtt_offset(struct intel_dsb *dsb)
>  #endif
>  }
>  
> -static void dsb_write(struct intel_dsb *dsb, u32 idx, u32 val)
> +static void dsb_write(struct intel_dsb *dsb, u32 idx, u32 value)
>  {
>  #ifdef I915
> -	dsb->cmd_buf[idx] = val;
> +	dsb->cmd_buf[idx] = value;
>  #else
> -	iosys_map_wr(&dsb->obj->vmap, idx * 4, u32, val);
> +	iosys_map_wr(&dsb->obj->vmap, idx * 4, u32, value);
>  #endif
>  }
>  
> @@ -97,12 +97,12 @@ static u32 dsb_read(struct intel_dsb *dsb, u32 idx)
>  #endif
>  }
>  
> -static void dsb_memset(struct intel_dsb *dsb, u32 idx, u32 val, u32 sz)
> +static void dsb_memset(struct intel_dsb *dsb, u32 idx, u32 value, u32 sz)
>  {
>  #ifdef I915
> -	memset(&dsb->cmd_buf[idx], val, sz);
> +	memset(&dsb->cmd_buf[idx], value, sz);
>  #else
> -	iosys_map_memset(&dsb->obj->vmap, idx * 4, val, sz);
> +	iosys_map_memset(&dsb->obj->vmap, idx * 4, value, sz);
>  #endif
>  }
>  
> -- 
> 2.42.0
> 

Re: [Intel-xe] [PATCH 3/3] drm/xe: Fix format string security warning

[Rodrigo Vivi]

On Wed, Oct 18, 2023 at 01:58:26PM +0200, Michał Winiarski wrote:
> While "gtidle->name" is not user-controllable string, it's still a bad
> practice to pass variable as a format string, it also causes the build
> to fail with:
> 
> drivers/gpu/drm/xe/xe_gt_idle_sysfs.c:88:26: error: format string is not a string literal (potentially insecure) [-Werror,-Wformat-security]
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_gt_idle_sysfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt_idle_sysfs.c b/drivers/gpu/drm/xe/xe_gt_idle_sysfs.c
> index 7238e96a116cf..8df9840811cda 100644
> --- a/drivers/gpu/drm/xe/xe_gt_idle_sysfs.c
> +++ b/drivers/gpu/drm/xe/xe_gt_idle_sysfs.c
> @@ -85,7 +85,7 @@ static ssize_t name_show(struct device *dev,
>  {
>  	struct xe_gt_idle *gtidle = dev_to_gtidle(dev);
>  
> -	return sysfs_emit(buff, gtidle->name);
> +	return sysfs_emit(buff, "%s\n", gtidle->name);
>  }
>  static DEVICE_ATTR_RO(name);
>  
> -- 
> 2.42.0
> 

Re: [Intel-xe] [PATCH 2/3] drm/i915: Fix uninitialized variable warning

[Lucas De Marchi]

On Wed, Oct 18, 2023 at 01:58:25PM +0200, Michał Winiarski wrote:
>When used with Xe, "val" uses as function argument name clashes with
>"val" used inside iosys_map_* macro causing the build to fail with:
>
>drivers/gpu/drm/i915/display/intel_dsb.c:87:46: error: variable 'val' is uninitialized when used within its own initialization [-Werror,-Wuninitialized]
>
>Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>

I'd rather change the macro so it doesn't use a commonly-used variable
name. I usually use a _ suffix on macro arguments because of that.

I should had done that in commit e62f25e8b3cd ("iosys-map: Add a few more helpers")

wouldn't this be better?

diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h
index 1b06d074ade0..78b3856c475a 100644
--- a/include/linux/iosys-map.h
+++ b/include/linux/iosys-map.h
@@ -414,11 +414,11 @@ static inline void iosys_map_memset(struct iosys_map *dst, size_t offset,
   * supported), use iosys_map_memcpy_to()
   */
  #define iosys_map_wr(map__, offset__, type__, val__) ({				\
-	type__ val = (val__);							\
+	type__ val_ = (val__);							\
  	if ((map__)->is_iomem) {						\
-		__iosys_map_wr_io(val, (map__)->vaddr_iomem + (offset__), type__);\
+		__iosys_map_wr_io(val_, (map__)->vaddr_iomem + (offset__), type__);\
  	} else {								\
-		__iosys_map_wr_sys(val, (map__)->vaddr + (offset__), type__);	\
+		__iosys_map_wr_sys(val_, (map__)->vaddr + (offset__), type__);	\
  	}									\
  })

Re: [Intel-xe] [PATCH 2/3] drm/i915: Fix uninitialized variable warning

[Upadhyay, Tejas]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Lucas
> De Marchi
> Sent: Wednesday, October 18, 2023 10:25 PM
> To: Winiarski, Michal <michal.winiarski@intel.com>
> Cc: intel-xe@lists.freedesktop.org
> Subject: Re: [Intel-xe] [PATCH 2/3] drm/i915: Fix uninitialized variable warning
> 
> On Wed, Oct 18, 2023 at 01:58:25PM +0200, Michał Winiarski wrote:
> >When used with Xe, "val" uses as function argument name clashes with
> >"val" used inside iosys_map_* macro causing the build to fail with:
> >
> >drivers/gpu/drm/i915/display/intel_dsb.c:87:46: error: variable 'val'
> >is uninitialized when used within its own initialization
> >[-Werror,-Wuninitialized]
> >
> >Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> 
> I'd rather change the macro so it doesn't use a commonly-used variable
> name. I usually use a _ suffix on macro arguments because of that.
> 
> I should had done that in commit e62f25e8b3cd ("iosys-map: Add a few more
> helpers")
> 
> wouldn't this be better?
> 
> diff --git a/include/linux/iosys-map.h b/include/linux/iosys-map.h index
> 1b06d074ade0..78b3856c475a 100644
> --- a/include/linux/iosys-map.h
> +++ b/include/linux/iosys-map.h
> @@ -414,11 +414,11 @@ static inline void iosys_map_memset(struct
> iosys_map *dst, size_t offset,
>    * supported), use iosys_map_memcpy_to()
>    */
>   #define iosys_map_wr(map__, offset__, type__, val__) ({
> 		\
> -	type__ val = (val__);
> 	\
> +	type__ val_ = (val__);
> 	\
>   	if ((map__)->is_iomem) {
> 	\
> -		__iosys_map_wr_io(val, (map__)->vaddr_iomem + (offset__),
> type__);\
> +		__iosys_map_wr_io(val_, (map__)->vaddr_iomem +
> (offset__), type__);\
>   	} else {								\
> -		__iosys_map_wr_sys(val, (map__)->vaddr + (offset__),
> type__);	\
> +		__iosys_map_wr_sys(val_, (map__)->vaddr + (offset__),
> type__);	\
>   	}
> 	\

Prefix _ in defines is general practice to avoid use of same name rest execution of code. So it looks ok to me,

Acked-by: Tejas Upadhyay <tejas.upadhyay@intel.com>

>   })

Re: [Intel-xe] [PATCH 2/3] drm/i915: Fix uninitialized variable warning

[Jani Nikula]

On Wed, 18 Oct 2023, Michał Winiarski <michal.winiarski@intel.com> wrote:
> When used with Xe, "val" uses as function argument name clashes with
> "val" used inside iosys_map_* macro causing the build to fail with:
>
> drivers/gpu/drm/i915/display/intel_dsb.c:87:46: error: variable 'val' is uninitialized when used within its own initialization [-Werror,-Wuninitialized]

Whatever the outcome of the discussion, this patch must *not* be applied
to drm-xe-next. It needs to go to upstream i915 first.

BR,
Jani.

>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsb.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 796b1b36dc598..8d89c8f700d68 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -79,12 +79,12 @@ static u32 dsb_ggtt_offset(struct intel_dsb *dsb)
>  #endif
>  }
>  
> -static void dsb_write(struct intel_dsb *dsb, u32 idx, u32 val)
> +static void dsb_write(struct intel_dsb *dsb, u32 idx, u32 value)
>  {
>  #ifdef I915
> -	dsb->cmd_buf[idx] = val;
> +	dsb->cmd_buf[idx] = value;
>  #else
> -	iosys_map_wr(&dsb->obj->vmap, idx * 4, u32, val);
> +	iosys_map_wr(&dsb->obj->vmap, idx * 4, u32, value);
>  #endif
>  }
>  
> @@ -97,12 +97,12 @@ static u32 dsb_read(struct intel_dsb *dsb, u32 idx)
>  #endif
>  }
>  
> -static void dsb_memset(struct intel_dsb *dsb, u32 idx, u32 val, u32 sz)
> +static void dsb_memset(struct intel_dsb *dsb, u32 idx, u32 value, u32 sz)
>  {
>  #ifdef I915
> -	memset(&dsb->cmd_buf[idx], val, sz);
> +	memset(&dsb->cmd_buf[idx], value, sz);
>  #else
> -	iosys_map_memset(&dsb->obj->vmap, idx * 4, val, sz);
> +	iosys_map_memset(&dsb->obj->vmap, idx * 4, value, sz);
>  #endif
>  }

-- 
Jani Nikula, Intel