Re: [PATCH 1/1] drm/amd/powerplay: initialize a variable before using it

2017-11-04 Thread Nicolas Iooss 
On Sun, Sep 3, 2017 at 2:00 PM, Nicolas Iooss
<nicolas.iooss_li...@m4x.org> wrote:
>
> Function vega10_apply_state_adjust_rules() only initializes
> stable_pstate_sclk_dpm_percentage when
> data->registry_data.stable_pstate_sclk_dpm_percentage is not between 1
> and 100. The variable is then used to compute stable_pstate_sclk, which
> therefore uses an uninitialized value.
>
> Fix this by initializing stable_pstate_sclk_dpm_percentage to
> data->registry_data.stable_pstate_sclk_dpm_percentage.
>
> This issue has been found while building the kernel with clang. The
> compiler reported a -Wsometimes-uninitialized warning.
>
> Fixes: f83a9991648b ("drm/amd/powerplay: add Vega10 powerplay support (v5)")
> Signed-off-by: Nicolas Iooss <nicolas.iooss_li...@m4x.org>
> ---
>  drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> index 197174e562d2..c8d28f78cd47 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> @@ -3043,6 +3043,8 @@ static int vega10_apply_state_adjust_rules(struct 
> pp_hwmgr *hwmgr,
>
> if (phm_cap_enabled(hwmgr->platform_descriptor.platformCaps,
> PHM_PlatformCaps_StablePState)) {
> +   stable_pstate_sclk_dpm_percentage =
> +   data->registry_data.stable_pstate_sclk_dpm_percentage;
> PP_ASSERT_WITH_CODE(
> data->registry_data.stable_pstate_sclk_dpm_percentage 
> >= 1 &&
> data->registry_data.stable_pstate_sclk_dpm_percentage 
> <= 100,
> --
> 2.14.1

Hello,
I have not received any comment on the above patch that I sent two
months ago, and the issue which is fixed by it still exists in today's
linux-next code [1]. Could you please review this patch?

Thanks,
Nicolas

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c#n3137
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/1] drm/amd/powerplay: initialize a variable before using it

2017-09-03 Thread Nicolas Iooss 
Function vega10_apply_state_adjust_rules() only initializes
stable_pstate_sclk_dpm_percentage when
data->registry_data.stable_pstate_sclk_dpm_percentage is not between 1
and 100. The variable is then used to compute stable_pstate_sclk, which
therefore uses an uninitialized value.

Fix this by initializing stable_pstate_sclk_dpm_percentage to
data->registry_data.stable_pstate_sclk_dpm_percentage.

This issue has been found while building the kernel with clang. The
compiler reported a -Wsometimes-uninitialized warning.

Fixes: f83a9991648b ("drm/amd/powerplay: add Vega10 powerplay support (v5)")
Signed-off-by: Nicolas Iooss <nicolas.iooss_li...@m4x.org>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
index 197174e562d2..c8d28f78cd47 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
@@ -3043,6 +3043,8 @@ static int vega10_apply_state_adjust_rules(struct 
pp_hwmgr *hwmgr,
 
if (phm_cap_enabled(hwmgr->platform_descriptor.platformCaps,
PHM_PlatformCaps_StablePState)) {
+   stable_pstate_sclk_dpm_percentage =
+   data->registry_data.stable_pstate_sclk_dpm_percentage;
PP_ASSERT_WITH_CODE(
data->registry_data.stable_pstate_sclk_dpm_percentage 
>= 1 &&
data->registry_data.stable_pstate_sclk_dpm_percentage 
<= 100,
-- 
2.14.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/1] drm/sti: use seq_puts to display a string

2017-03-31 Thread Nicolas Iooss 
gdp_dbg_ctl() uses seq_printf() to display a color format name even
though there is no format string. When using -Wformat-string, gcc
reports the following warning:

drivers/gpu/drm/sti/sti_gdp.c: In function 'gdp_dbg_ctl':
drivers/gpu/drm/sti/sti_gdp.c:150:18: warning: format not a string
literal and no format arguments [-Wformat-security]
seq_printf(s, gdp_format_to_str[i].name);
  ^

Silence this warning by using seq_puts() instead.

Signed-off-by: Nicolas Iooss <nicolas.iooss_li...@m4x.org>
---
 drivers/gpu/drm/sti/sti_gdp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sti/sti_gdp.c b/drivers/gpu/drm/sti/sti_gdp.c
index 86279f5022c2..1bbfcf01d401 100644
--- a/drivers/gpu/drm/sti/sti_gdp.c
+++ b/drivers/gpu/drm/sti/sti_gdp.c
@@ -147,7 +147,7 @@ static void gdp_dbg_ctl(struct seq_file *s, int val)
seq_puts(s, "\tColor:");
for (i = 0; i < ARRAY_SIZE(gdp_format_to_str); i++) {
if (gdp_format_to_str[i].format == (val & 0x1F)) {
-   seq_printf(s, gdp_format_to_str[i].name);
+   seq_puts(s, gdp_format_to_str[i].name);
break;
}
}
-- 
2.11.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/1] drm/amd/powerplay: fix misspelling in header guard

2017-01-22 Thread Nicolas Iooss 
In smu7_clockpowergating.h, the #ifndef statement which prevents
multiple inclusions of the header file uses _SMU7_CLOCK_POWER_GATING_H_
but the following #define statement uses _SMU7_CLOCK__POWER_GATING_H_.

Signed-off-by: Nicolas Iooss <nicolas.iooss_li...@m4x.org>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h 
b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h
index d52a28c343e3..c96ed9ed7eaf 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_clockpowergating.h
@@ -22,7 +22,7 @@
  */
 
 #ifndef _SMU7_CLOCK_POWER_GATING_H_
-#define _SMU7_CLOCK__POWER_GATING_H_
+#define _SMU7_CLOCK_POWER_GATING_H_
 
 #include "smu7_hwmgr.h"
 #include "pp_asicblocks.h"
-- 
2.11.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/1] drm/i915/gvt: verify functions types in new_mmio_info()

2016-12-26 Thread Nicolas Iooss 
The current prototype of new_mmio_info() uses void* for parameters read
and write, which are functions with precise calling conventions
(argument types and return type). Write down these conventions in
new_mmio_info() definition.

This has been reported by the following warnings when clang is used to
build the kernel:

drivers/gpu/drm/i915/gvt/handlers.c:124:21: error: pointer type
mismatch ('void *' and 'int (*)(struct intel_vgpu *, unsigned int,
void *, unsigned int)') [-Werror,-Wpointer-type-mismatch]
info->read = read ? read : intel_vgpu_default_mmio_read;
  ^    
drivers/gpu/drm/i915/gvt/handlers.c:125:23: error: pointer type
mismatch ('void *' and 'int (*)(struct intel_vgpu *, unsigned int,
void *, unsigned int)') [-Werror,-Wpointer-type-mismatch]
info->write = write ? write : intel_vgpu_default_mmio_write;
^ ~   ~

This allows the compiler to detect that sbi_ctl_mmio_write() returns a
"bool" value instead of an expected "int" one. Fix this.

Signed-off-by: Nicolas Iooss 
---
 drivers/gpu/drm/i915/gvt/handlers.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/handlers.c 
b/drivers/gpu/drm/i915/gvt/handlers.c
index 522809710312..052e57124c0a 100644
--- a/drivers/gpu/drm/i915/gvt/handlers.c
+++ b/drivers/gpu/drm/i915/gvt/handlers.c
@@ -93,7 +93,8 @@ static void write_vreg(struct intel_vgpu *vgpu, unsigned int 
offset,
 static int new_mmio_info(struct intel_gvt *gvt,
u32 offset, u32 flags, u32 size,
u32 addr_mask, u32 ro_mask, u32 device,
-   void *read, void *write)
+   int (*read)(struct intel_vgpu *, unsigned int, void *, unsigned 
int),
+   int (*write)(struct intel_vgpu *, unsigned int, void *, 
unsigned int))
 {
struct intel_gvt_mmio_info *info, *p;
u32 start, end, i;
@@ -974,7 +975,7 @@ static int sbi_data_mmio_read(struct intel_vgpu *vgpu, 
unsigned int offset,
return 0;
 }

-static bool sbi_ctl_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
+static int sbi_ctl_mmio_write(struct intel_vgpu *vgpu, unsigned int offset,
void *p_data, unsigned int bytes)
 {
u32 data;
-- 
2.11.0

[PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value

2016-10-23 Thread Nicolas Iooss 
Hello,

I sent the patch below a few weeks ago. I got some comments (cf. [1])
which looked good, but the patch has not been merged in linux-next yet.
Do I need to fix something (like rewrite the commit message) in order to
get it merged?

Thanks,
Nicolas

[1] https://patchwork.freedesktop.org/patch/108941/

On 04/09/16 20:58, Nicolas Iooss wrote:
> When building the kernel with clang and some warning flags, the compiler
> reports that the return value of dcs_get_backlight() may be
> uninitialized:
> 
> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:53:2: error: variable
> 'data' is used uninitialized whenever 'for' loop exits because its
> condition is false [-Werror,-Wsometimes-uninitialized]
> for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
> ^~~
> drivers/gpu/drm/i915/intel_dsi.h:126:49: note: expanded from macro
> 'for_each_dsi_port'
> #define for_each_dsi_port(__port, __ports_mask)
> for_each_port_masked(__port, __ports_mask)
> ^~
> drivers/gpu/drm/i915/i915_drv.h:322:26: note: expanded from macro
> 'for_each_port_masked'
> for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++)  \
> ^
> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:60:9: note:
> uninitialized use occurs here
> return data;
>^~~~
> 
> As intel_dsi->dcs_backlight_ports seems to be always initialized to a
> non-null value, the content of the for loop is always executed and there
> is no bug in the current code. Nevertheless the compiler has no way of
> knowing that assumption, so initialize variable 'data' to silence the
> warning here.
> 
> Signed-off-by: Nicolas Iooss 
> ---
>  drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c 
> b/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c
> index ac7c6020c443..eec45856f910 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c
> @@ -46,7 +46,7 @@ static u32 dcs_get_backlight(struct intel_connector 
> *connector)
>   struct intel_encoder *encoder = connector->encoder;
>   struct intel_dsi *intel_dsi = enc_to_intel_dsi(>base);
>   struct mipi_dsi_device *dsi_device;
> - u8 data;
> + u8 data = 0;
>   enum port port;
>  
>   /* FIXME: Need to take care of 16 bit brightness level */
>

[Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value

2016-09-12 Thread Nicolas Iooss 
On 08/09/16 16:31, Dave Gordon wrote:
> On 08/09/16 00:02, Nicolas Iooss wrote:
>> On 07/09/16 18:03, Dave Gordon wrote:
>>> On 06/09/16 21:36, Nicolas Iooss wrote:
>>>> On 06/09/16 12:21, Dave Gordon wrote:
>>>>> On 04/09/16 19:58, Nicolas Iooss wrote:
>>>>>> When building the kernel with clang and some warning flags, the
>>>>>> compiler
>>>>>> reports that the return value of dcs_get_backlight() may be
>>>>>> uninitialized:
>>>>>>
>>>>>> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:53:2: error:
>>>>>> variable
>>>>>> 'data' is used uninitialized whenever 'for' loop exits because
>>>>>> its
>>>>>> condition is false [-Werror,-Wsometimes-uninitialized]
>>>>>> for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
>>>>>> ^~~
>>>>>> drivers/gpu/drm/i915/intel_dsi.h:126:49: note: expanded from
>>>>>> macro
>>>>>> 'for_each_dsi_port'
>>>>>> #define for_each_dsi_port(__port, __ports_mask)
>>>>>> for_each_port_masked(__port,
>>>>>> __ports_mask)
>>>>>>
>>>>>> ^~
>>>>>> drivers/gpu/drm/i915/i915_drv.h:322:26: note: expanded from macro
>>>>>> 'for_each_port_masked'
>>>>>> for ((__port) = PORT_A; (__port) < I915_MAX_PORTS;
>>>>>> (__port)++)  \
>>>>>> ^
>>>>>> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:60:9: note:
>>>>>> uninitialized use occurs here
>>>>>> return data;
>>>>>>^~~~
>>>>>>
>>>>>> As intel_dsi->dcs_backlight_ports seems to be always initialized to a
>>>>>> non-null value, the content of the for loop is always executed and
>>>>>> there
>>>>>> is no bug in the current code. Nevertheless the compiler has no
>>>>>> way of
>>>>>> knowing that assumption, so initialize variable 'data' to silence the
>>>>>> warning here.
>>>>>>
>>>>>> Signed-off-by: Nicolas Iooss 
>>>>>
>>>>> Interesting ... there are two things that could lead to this
>>>>> (possibly)
>>>>> incorrect analysis. Either it thinks the loop could be executed zero
>>>>> times, which would be a deficiency in the compiler, as the initialiser
>>>>> and loop bound are both known (different) constants:
>>>>>
>>>>> enum port {
>>>>> PORT_A = 0,
>>>>> PORT_B,
>>>>> PORT_C,
>>>>> PORT_D,
>>>>> PORT_E,
>>>>> I915_MAX_PORTS
>>>>> };
>>>>>
>>>>> or, it doesn't understand that because we've passed  to another
>>>>> function, it can have been set by the callee. It may be extra
>>>>> confusing
>>>>> that the callee takes (void *); or it may be being ultra-sophisticated
>>>>> in its analysis and noted that in one error path data is *not* set
>>>>> (and
>>>>> we then discard the error and use data anyway). As an experiment, you
>>>>> could try:
>>>>
>>>> The code that the compiler sees is not a simple loop other enum 'port'
>>>> but "for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {", which
>>>> is expanded [1] to:
>>>>
>>>> for ((port) = PORT_A; (port) < I915_MAX_PORTS; (port)++)
>>>>   if (!((intel_dsi->dcs_backlight_ports) & (1 << (port {}
>>>> else {
>>>>
>>>> This is why I spoke of intel_dsi->dcs_backlight_ports in my
>>>> description:
>>>> if it is zero, the body of the loop is never run.
>>>>
>>>> As for the analyses of calls using , clang does not warn about the
>>>> variable being maybe uninitialized following a call. This is quite
>>>> expected as this would lead to too many false positives, even though it
>>>> may miss some bugs.
>>>>
&g

[Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value

2016-09-08 Thread Nicolas Iooss 
On 07/09/16 18:03, Dave Gordon wrote:
> On 06/09/16 21:36, Nicolas Iooss wrote:
>> On 06/09/16 12:21, Dave Gordon wrote:
>>> On 04/09/16 19:58, Nicolas Iooss wrote:
>>>> When building the kernel with clang and some warning flags, the
>>>> compiler
>>>> reports that the return value of dcs_get_backlight() may be
>>>> uninitialized:
>>>>
>>>> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:53:2: error:
>>>> variable
>>>> 'data' is used uninitialized whenever 'for' loop exits because its
>>>> condition is false [-Werror,-Wsometimes-uninitialized]
>>>> for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
>>>> ^~~
>>>> drivers/gpu/drm/i915/intel_dsi.h:126:49: note: expanded from macro
>>>> 'for_each_dsi_port'
>>>> #define for_each_dsi_port(__port, __ports_mask)
>>>> for_each_port_masked(__port,
>>>> __ports_mask)
>>>>
>>>> ^~
>>>> drivers/gpu/drm/i915/i915_drv.h:322:26: note: expanded from macro
>>>> 'for_each_port_masked'
>>>> for ((__port) = PORT_A; (__port) < I915_MAX_PORTS;
>>>> (__port)++)  \
>>>> ^
>>>> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:60:9: note:
>>>> uninitialized use occurs here
>>>> return data;
>>>>^~~~
>>>>
>>>> As intel_dsi->dcs_backlight_ports seems to be always initialized to a
>>>> non-null value, the content of the for loop is always executed and
>>>> there
>>>> is no bug in the current code. Nevertheless the compiler has no way of
>>>> knowing that assumption, so initialize variable 'data' to silence the
>>>> warning here.
>>>>
>>>> Signed-off-by: Nicolas Iooss 
>>>
>>> Interesting ... there are two things that could lead to this (possibly)
>>> incorrect analysis. Either it thinks the loop could be executed zero
>>> times, which would be a deficiency in the compiler, as the initialiser
>>> and loop bound are both known (different) constants:
>>>
>>> enum port {
>>> PORT_A = 0,
>>> PORT_B,
>>> PORT_C,
>>> PORT_D,
>>> PORT_E,
>>> I915_MAX_PORTS
>>> };
>>>
>>> or, it doesn't understand that because we've passed  to another
>>> function, it can have been set by the callee. It may be extra confusing
>>> that the callee takes (void *); or it may be being ultra-sophisticated
>>> in its analysis and noted that in one error path data is *not* set (and
>>> we then discard the error and use data anyway). As an experiment, you
>>> could try:
>>
>> The code that the compiler sees is not a simple loop other enum 'port'
>> but "for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {", which
>> is expanded [1] to:
>>
>> for ((port) = PORT_A; (port) < I915_MAX_PORTS; (port)++)
>>   if (!((intel_dsi->dcs_backlight_ports) & (1 << (port {} else {
>>
>> This is why I spoke of intel_dsi->dcs_backlight_ports in my description:
>> if it is zero, the body of the loop is never run.
>>
>> As for the analyses of calls using , clang does not warn about the
>> variable being maybe uninitialized following a call. This is quite
>> expected as this would lead to too many false positives, even though it
>> may miss some bugs.
>>
>>> static u8 mipi_dsi_dcs_read1(struct mipi_dsi_device *dsi_device, u8 cmd)
>>> {
>>> u8 data = 0;
>>>
>>> mipi_dsi_dcs_read(dsi_device, cmd, , sizeof(data));
>>>
>>> return data;
>>> }
>>>
>>> static u32 dcs_get_backlight(struct intel_connector *connector)
>>> {
>>> struct intel_encoder *encoder = connector->encoder;
>>> struct intel_dsi *intel_dsi = enc_to_intel_dsi(>base);
>>> struct mipi_dsi_device *dsi_device;
>>> enum port port;
>>> u8 data;
>>>
>>> /* FIXME: Need to take care of 16 bit brightness level */
>>> for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
>>> dsi_device = intel_d

[Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value

2016-09-06 Thread Nicolas Iooss 
On 06/09/16 12:21, Dave Gordon wrote:
> On 04/09/16 19:58, Nicolas Iooss wrote:
>> When building the kernel with clang and some warning flags, the compiler
>> reports that the return value of dcs_get_backlight() may be
>> uninitialized:
>>
>> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:53:2: error: variable
>> 'data' is used uninitialized whenever 'for' loop exits because its
>> condition is false [-Werror,-Wsometimes-uninitialized]
>> for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
>> ^~~
>> drivers/gpu/drm/i915/intel_dsi.h:126:49: note: expanded from macro
>> 'for_each_dsi_port'
>> #define for_each_dsi_port(__port, __ports_mask)
>> for_each_port_masked(__port,
>> __ports_mask)
>>
>> ^~
>> drivers/gpu/drm/i915/i915_drv.h:322:26: note: expanded from macro
>> 'for_each_port_masked'
>> for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++)  \
>> ^
>> drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:60:9: note:
>> uninitialized use occurs here
>> return data;
>>^~~~
>>
>> As intel_dsi->dcs_backlight_ports seems to be always initialized to a
>> non-null value, the content of the for loop is always executed and there
>> is no bug in the current code. Nevertheless the compiler has no way of
>> knowing that assumption, so initialize variable 'data' to silence the
>> warning here.
>>
>> Signed-off-by: Nicolas Iooss 
> 
> Interesting ... there are two things that could lead to this (possibly)
> incorrect analysis. Either it thinks the loop could be executed zero
> times, which would be a deficiency in the compiler, as the initialiser
> and loop bound are both known (different) constants:
> 
> enum port {
> PORT_A = 0,
> PORT_B,
> PORT_C,
> PORT_D,
> PORT_E,
> I915_MAX_PORTS
> };
> 
> or, it doesn't understand that because we've passed  to another
> function, it can have been set by the callee. It may be extra confusing
> that the callee takes (void *); or it may be being ultra-sophisticated
> in its analysis and noted that in one error path data is *not* set (and
> we then discard the error and use data anyway). As an experiment, you
> could try:

The code that the compiler sees is not a simple loop other enum 'port'
but "for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {", which
is expanded [1] to:

for ((port) = PORT_A; (port) < I915_MAX_PORTS; (port)++)
  if (!((intel_dsi->dcs_backlight_ports) & (1 << (port {} else {

This is why I spoke of intel_dsi->dcs_backlight_ports in my description:
if it is zero, the body of the loop is never run.

As for the analyses of calls using , clang does not warn about the
variable being maybe uninitialized following a call. This is quite
expected as this would lead to too many false positives, even though it
may miss some bugs.

> 
> static u8 mipi_dsi_dcs_read1(struct mipi_dsi_device *dsi_device, u8 cmd)
> {
> u8 data = 0;
> 
> mipi_dsi_dcs_read(dsi_device, cmd, , sizeof(data));
> 
> return data;
> }
> 
> static u32 dcs_get_backlight(struct intel_connector *connector)
> {
> struct intel_encoder *encoder = connector->encoder;
> struct intel_dsi *intel_dsi = enc_to_intel_dsi(>base);
> struct mipi_dsi_device *dsi_device;
> enum port port;
> u8 data;
> 
> /* FIXME: Need to take care of 16 bit brightness level */
> for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
> dsi_device = intel_dsi->dsi_hosts[port]->device;
> data = mipi_dsi_dcs_read1(dsi_device,
> MIPI_DCS_GET_DISPLAY_BRIGHTNESS);
> break;
> }
> 
> return data;
> }
> 
> If it complains about that then it's a shortcoming in the loop analysis.

It complains (in dcs_get_backlight), because for_each_dsi_port() still
hides an 'if' condition.

> If not you could try:
> 
> static u8 mipi_dsi_dcs_read1(struct mipi_dsi_device *dsi_device, u8 cmd)
> {
> u8 data;
> ssize_t nbytes = sizeof(data);
> 
> nbytes = mipi_dsi_dcs_read(dsi_device, cmd, , nbytes);
> return nbytes == sizeof(data) ? data : 0;
> }
> 
> and if complains about that then it doesn't understand that passing
>  allows it to be set. If it doesn't complain about this version,
> then the original error was actually correct, in the sense that data can
> indeed be used uninitialised if certain error paths can be taken.

clang did not complain with this last case.

> Here's an R-b for your fix anyway ...
> 
> Reviewed-by: Dave Gordon 

Thanks!

Nicolas


[1] I used "make drivers/gpu/drm/i915/intel_dsi_dcs_backlight.i" to see
the output of the preprocessor.

[PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value

2016-09-04 Thread Nicolas Iooss 
When building the kernel with clang and some warning flags, the compiler
reports that the return value of dcs_get_backlight() may be
uninitialized:

drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:53:2: error: variable
'data' is used uninitialized whenever 'for' loop exits because its
condition is false [-Werror,-Wsometimes-uninitialized]
for_each_dsi_port(port, intel_dsi->dcs_backlight_ports) {
^~~
drivers/gpu/drm/i915/intel_dsi.h:126:49: note: expanded from macro
'for_each_dsi_port'
#define for_each_dsi_port(__port, __ports_mask)
for_each_port_masked(__port, __ports_mask)
^~
drivers/gpu/drm/i915/i915_drv.h:322:26: note: expanded from macro
'for_each_port_masked'
for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++)  \
^
drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c:60:9: note:
uninitialized use occurs here
return data;
   ^~~~

As intel_dsi->dcs_backlight_ports seems to be always initialized to a
non-null value, the content of the for loop is always executed and there
is no bug in the current code. Nevertheless the compiler has no way of
knowing that assumption, so initialize variable 'data' to silence the
warning here.

Signed-off-by: Nicolas Iooss 
---
 drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c 
b/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c
index ac7c6020c443..eec45856f910 100644
--- a/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dsi_dcs_backlight.c
@@ -46,7 +46,7 @@ static u32 dcs_get_backlight(struct intel_connector 
*connector)
struct intel_encoder *encoder = connector->encoder;
struct intel_dsi *intel_dsi = enc_to_intel_dsi(>base);
struct mipi_dsi_device *dsi_device;
-   u8 data;
+   u8 data = 0;
enum port port;

/* FIXME: Need to take care of 16 bit brightness level */
-- 
2.9.3

[PATCH 1/1] drm/amdgpu: initialize amdgpu_cgs_acpi_eval_object result value

2016-06-18 Thread Nicolas Iooss 
amdgpu_cgs_acpi_eval_object() returned the value of variable "result"
without initializing it first.

This bug has been found by compiling the kernel with clang.  The
compiler complained:

drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:972:14: error: variable
'result' is used uninitialized whenever 'for' loop exits because its
condition is false [-Werror,-Wsometimes-uninitialized]
for (i = 0; i < count; i++) {
^
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:1011:9: note: uninitialized
use occurs here
return result;
   ^~
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:972:14: note: remove the
condition if it is always true
for (i = 0; i < count; i++) {
^
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:864:12: note: initialize the
variable 'result' to silence this warning
int result;
  ^
   = 0

Fixes: 3f1d35a03b3c ("drm/amdgpu: implement new cgs interface for acpi
function")
Signed-off-by: Nicolas Iooss 
Cc: stable at vger.kernel.org
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
index 8943099eb135..cf6f49fc1c75 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c
@@ -909,7 +909,7 @@ static int amdgpu_cgs_acpi_eval_object(struct cgs_device 
*cgs_device,
struct cgs_acpi_method_argument *argument = NULL;
uint32_t i, count;
acpi_status status;
-   int result;
+   int result = 0;
uint32_t func_no = 0x;

handle = ACPI_HANDLE(>pdev->dev);
-- 
2.8.3

[PATCH v2 2/2] drm: use dev_name as default unique name in drm_dev_alloc()

2015-12-11 Thread Nicolas Iooss 
The following code pattern exists in some DRM drivers:

ddev = drm_dev_alloc(, parent_dev);
drm_dev_set_unique(ddev, dev_name(parent_dev));

(Sometimes dev_name(ddev->dev) is used, which is the same.)

As suggested in
http://lists.freedesktop.org/archives/dri-devel/2015-December/096441.html,
the unique name of a new DRM device can be set as dev_name(parent_dev)
when parent_dev is not NULL (vgem is a special case).

Signed-off-by: Nicolas Iooss 
Acked-by: Boris Brezillon 
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 4 
 drivers/gpu/drm/drm_drv.c| 9 +
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c| 1 -
 drivers/gpu/drm/nouveau/nouveau_drm.c| 4 
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c  | 4 
 drivers/gpu/drm/tegra/drm.c  | 1 -
 drivers/gpu/drm/vc4/vc4_drv.c| 2 --
 7 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c 
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 244df0a440b7..fba4f72e7ae1 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -733,10 +733,6 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device 
*pdev)
if (!ddev)
return -ENOMEM;

-   ret = drm_dev_set_unique(ddev, dev_name(ddev->dev));
-   if (ret)
-   goto err_unref;
-
ret = atmel_hlcdc_dc_load(ddev);
if (ret)
goto err_unref;
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index eaa4316f3c45..bf934cdea21c 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -633,8 +633,17 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
}
}

+   if (parent) {
+   ret = drm_dev_set_unique(dev, dev_name(parent));
+   if (ret)
+   goto err_setunique;
+   }
+
return dev;

+err_setunique:
+   if (drm_core_check_feature(dev, DRIVER_GEM))
+   drm_gem_destroy(dev);
 err_ctxbitmap:
drm_legacy_ctxbitmap_cleanup(dev);
drm_ht_remove(>map_hash);
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c 
b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index 1930234ba5f1..fca97d3fc846 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -363,7 +363,6 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
fsl_dev->np = dev->of_node;
drm->dev_private = fsl_dev;
dev_set_drvdata(dev, fsl_dev);
-   drm_dev_set_unique(drm, dev_name(dev));

ret = drm_dev_register(drm, 0);
if (ret < 0)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 2d23f95f17ce..b3a563c44bcd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1046,10 +1046,6 @@ nouveau_platform_device_create(const struct 
nvkm_device_tegra_func *func,
goto err_free;
}

-   err = drm_dev_set_unique(drm, dev_name(>dev));
-   if (err < 0)
-   goto err_free;
-
drm->platformdev = pdev;
platform_set_drvdata(pdev, drm);

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 215d6c44af55..afbb7407c44f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -450,10 +450,6 @@ static int rockchip_drm_bind(struct device *dev)
if (!drm)
return -ENOMEM;

-   ret = drm_dev_set_unique(drm, dev_name(dev));
-   if (ret)
-   goto err_free;
-
ret = drm_dev_register(drm, 0);
if (ret)
goto err_free;
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 159ef515cab1..12e2d3ccbc9d 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -991,7 +991,6 @@ static int host1x_drm_probe(struct host1x_device *dev)
if (!drm)
return -ENOMEM;

-   drm_dev_set_unique(drm, dev_name(>dev));
dev_set_drvdata(>dev, drm);

err = drm_dev_register(drm, 0);
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index d5db9e0f3b73..647772305e8f 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -168,8 +168,6 @@ static int vc4_drm_bind(struct device *dev)
vc4->dev = drm;
drm->dev_private = vc4;

-   drm_dev_set_unique(drm, dev_name(dev));
-
drm_mode_config_init(drm);
if (ret)
goto unref;
-- 
2.6.3

[PATCH v2 1/2] drm: make drm_dev_set_unique() not use a format string

2015-12-11 Thread Nicolas Iooss 
drm_dev_set_unique() uses a format string to define the unique name of a
device.  This feature is not used as currently all the calls to this
function either use "%s" as a format string or directly use
dev_name().

Even though this second kind of call does not introduce security
problems, because there cannot be "%" characters in dev_name() results,
gcc issues a warning when building with -Wformat-security flag
("warning: format string is not a string literal (potentially
insecure)").  This warning is useful to find real bugs like the one
fixed by commit 3958b79266b1 ("configfs: fix kernel infoleak through
user-controlled format string").  False positives which do not bring
an extra value make the work of finding real bugs harder.

Therefore remove the format-string feature from drm_dev_set_unique().

Signed-off-by: Nicolas Iooss 
---
v2: update drm_dev_set_unique() comment

 drivers/gpu/drm/drm_drv.c   | 17 ++---
 drivers/gpu/drm/nouveau/nouveau_drm.c   |  2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  2 +-
 include/drm/drmP.h  |  2 +-
 4 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7dd6728dd092..eaa4316f3c45 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -797,23 +797,18 @@ EXPORT_SYMBOL(drm_dev_unregister);
 /**
  * drm_dev_set_unique - Set the unique name of a DRM device
  * @dev: device of which to set the unique name
- * @fmt: format string for unique name
+ * @name: unique name
  *
- * Sets the unique name of a DRM device using the specified format string and
- * a variable list of arguments. Drivers can use this at driver probe time if
- * the unique name of the devices they drive is static.
+ * Sets the unique name of a DRM device using the specified string. Drivers
+ * can use this at driver probe time if the unique name of the devices they
+ * drive is static.
  *
  * Return: 0 on success or a negative error code on failure.
  */
-int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...)
+int drm_dev_set_unique(struct drm_device *dev, const char *name)
 {
-   va_list ap;
-
kfree(dev->unique);
-
-   va_start(ap, fmt);
-   dev->unique = kvasprintf(GFP_KERNEL, fmt, ap);
-   va_end(ap);
+   dev->unique = kstrdup(name, GFP_KERNEL);

return dev->unique ? 0 : -ENOMEM;
 }
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 1d3ee5179ab8..2d23f95f17ce 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1046,7 +1046,7 @@ nouveau_platform_device_create(const struct 
nvkm_device_tegra_func *func,
goto err_free;
}

-   err = drm_dev_set_unique(drm, "%s", dev_name(>dev));
+   err = drm_dev_set_unique(drm, dev_name(>dev));
if (err < 0)
goto err_free;

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f22e1e1ee64a..215d6c44af55 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -450,7 +450,7 @@ static int rockchip_drm_bind(struct device *dev)
if (!drm)
return -ENOMEM;

-   ret = drm_dev_set_unique(drm, "%s", dev_name(dev));
+   ret = drm_dev_set_unique(drm, dev_name(dev));
if (ret)
goto err_free;

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 0a271ca1f7c7..f14c25a6bbf2 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1059,7 +1059,7 @@ void drm_dev_ref(struct drm_device *dev);
 void drm_dev_unref(struct drm_device *dev);
 int drm_dev_register(struct drm_device *dev, unsigned long flags);
 void drm_dev_unregister(struct drm_device *dev);
-int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...);
+int drm_dev_set_unique(struct drm_device *dev, const char *name);

 struct drm_minor *drm_minor_acquire(unsigned int minor_id);
 void drm_minor_release(struct drm_minor *minor);
-- 
2.6.3

[PATCH 1/2] drm: make drm_dev_set_unique() not use a format string

2015-12-09 Thread Nicolas Iooss 
On 12/09/2015 12:28 AM, Emil Velikov wrote:
> On 8 December 2015 at 22:12, Nicolas Iooss  
> wrote:
>> drm_dev_set_unique() uses a format string to define the unique name of a
>> device.  This feature is not used as currently all the calls to this
>> function either use "%s" as a format string or directly use
>> dev_name().
>>
>> Even though this second kind of call does not introduce security
>> problems, because there cannot be "%" characters in dev_name() results,
>> gcc issues a warning when building with -Wformat-security flag
>> ("warning: format string is not a string literal (potentially
>> insecure)").  This warning is useful to find real bugs like the one
>> fixed by commit 3958b79266b1 ("configfs: fix kernel infoleak through
>> user-controlled format string").  False positives which do not bring
>> an extra value make the work of finding real bugs harder.
>>
>> Therefore remove the format-string feature from drm_dev_set_unique().
>>
>> Signed-off-by: Nicolas Iooss 
>> ---
>>  drivers/gpu/drm/drm_drv.c   | 11 +++
>>  drivers/gpu/drm/nouveau/nouveau_drm.c   |  2 +-
>>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  2 +-
>>  include/drm/drmP.h  |  2 +-
>>  4 files changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 7dd6728dd092..20eaa0aae205 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -797,7 +797,7 @@ EXPORT_SYMBOL(drm_dev_unregister);
>>  /**
>>   * drm_dev_set_unique - Set the unique name of a DRM device
>>   * @dev: device of which to set the unique name
>> - * @fmt: format string for unique name
>> + * @name: unique name
>>   *
>>   * Sets the unique name of a DRM device using the specified format string 
>> and
>>   * a variable list of arguments. Drivers can use this at driver probe time 
>> if
> You might want to also update the above hunk :-)

Indeed, thanks! I will wait a little bit for other feedbacks, read all
the comments/documentation to see if anything else needs an update and
submit a v2.

Nicolas

[PATCH 2/2] drm: use dev_name as default unique name in drm_dev_alloc()

2015-12-08 Thread Nicolas Iooss 
The following code pattern exists in some DRM drivers:

ddev = drm_dev_alloc(, parent_dev);
drm_dev_set_unique(ddev, dev_name(parent_dev));

(Sometimes dev_name(ddev->dev) is used, which is the same.)

As suggested in
http://lists.freedesktop.org/archives/dri-devel/2015-December/096441.html,
the unique name of a new DRM device can be set as dev_name(parent_dev)
when parent_dev is not NULL (vgem is a special case).

Signed-off-by: Nicolas Iooss 
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 4 
 drivers/gpu/drm/drm_drv.c| 9 +
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c| 1 -
 drivers/gpu/drm/nouveau/nouveau_drm.c| 4 
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c  | 4 
 drivers/gpu/drm/tegra/drm.c  | 1 -
 drivers/gpu/drm/vc4/vc4_drv.c| 2 --
 7 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c 
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 244df0a440b7..fba4f72e7ae1 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -733,10 +733,6 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device 
*pdev)
if (!ddev)
return -ENOMEM;

-   ret = drm_dev_set_unique(ddev, dev_name(ddev->dev));
-   if (ret)
-   goto err_unref;
-
ret = atmel_hlcdc_dc_load(ddev);
if (ret)
goto err_unref;
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 20eaa0aae205..df749a6156e0 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -633,8 +633,17 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
}
}

+   if (parent) {
+   ret = drm_dev_set_unique(dev, dev_name(parent));
+   if (ret)
+   goto err_setunique;
+   }
+
return dev;

+err_setunique:
+   if (drm_core_check_feature(dev, DRIVER_GEM))
+   drm_gem_destroy(dev);
 err_ctxbitmap:
drm_legacy_ctxbitmap_cleanup(dev);
drm_ht_remove(>map_hash);
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c 
b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index 1930234ba5f1..fca97d3fc846 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -363,7 +363,6 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
fsl_dev->np = dev->of_node;
drm->dev_private = fsl_dev;
dev_set_drvdata(dev, fsl_dev);
-   drm_dev_set_unique(drm, dev_name(dev));

ret = drm_dev_register(drm, 0);
if (ret < 0)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 2d23f95f17ce..b3a563c44bcd 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1046,10 +1046,6 @@ nouveau_platform_device_create(const struct 
nvkm_device_tegra_func *func,
goto err_free;
}

-   err = drm_dev_set_unique(drm, dev_name(>dev));
-   if (err < 0)
-   goto err_free;
-
drm->platformdev = pdev;
platform_set_drvdata(pdev, drm);

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 215d6c44af55..afbb7407c44f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -450,10 +450,6 @@ static int rockchip_drm_bind(struct device *dev)
if (!drm)
return -ENOMEM;

-   ret = drm_dev_set_unique(drm, dev_name(dev));
-   if (ret)
-   goto err_free;
-
ret = drm_dev_register(drm, 0);
if (ret)
goto err_free;
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 159ef515cab1..12e2d3ccbc9d 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -991,7 +991,6 @@ static int host1x_drm_probe(struct host1x_device *dev)
if (!drm)
return -ENOMEM;

-   drm_dev_set_unique(drm, dev_name(>dev));
dev_set_drvdata(>dev, drm);

err = drm_dev_register(drm, 0);
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index d5db9e0f3b73..647772305e8f 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -168,8 +168,6 @@ static int vc4_drm_bind(struct device *dev)
vc4->dev = drm;
drm->dev_private = vc4;

-   drm_dev_set_unique(drm, dev_name(dev));
-
drm_mode_config_init(drm);
if (ret)
goto unref;
-- 
2.6.3

[PATCH 1/2] drm: make drm_dev_set_unique() not use a format string

2015-12-08 Thread Nicolas Iooss 
drm_dev_set_unique() uses a format string to define the unique name of a
device.  This feature is not used as currently all the calls to this
function either use "%s" as a format string or directly use
dev_name().

Even though this second kind of call does not introduce security
problems, because there cannot be "%" characters in dev_name() results,
gcc issues a warning when building with -Wformat-security flag
("warning: format string is not a string literal (potentially
insecure)").  This warning is useful to find real bugs like the one
fixed by commit 3958b79266b1 ("configfs: fix kernel infoleak through
user-controlled format string").  False positives which do not bring
an extra value make the work of finding real bugs harder.

Therefore remove the format-string feature from drm_dev_set_unique().

Signed-off-by: Nicolas Iooss 
---
 drivers/gpu/drm/drm_drv.c   | 11 +++
 drivers/gpu/drm/nouveau/nouveau_drm.c   |  2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  2 +-
 include/drm/drmP.h  |  2 +-
 4 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7dd6728dd092..20eaa0aae205 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -797,7 +797,7 @@ EXPORT_SYMBOL(drm_dev_unregister);
 /**
  * drm_dev_set_unique - Set the unique name of a DRM device
  * @dev: device of which to set the unique name
- * @fmt: format string for unique name
+ * @name: unique name
  *
  * Sets the unique name of a DRM device using the specified format string and
  * a variable list of arguments. Drivers can use this at driver probe time if
@@ -805,15 +805,10 @@ EXPORT_SYMBOL(drm_dev_unregister);
  *
  * Return: 0 on success or a negative error code on failure.
  */
-int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...)
+int drm_dev_set_unique(struct drm_device *dev, const char *name)
 {
-   va_list ap;
-
kfree(dev->unique);
-
-   va_start(ap, fmt);
-   dev->unique = kvasprintf(GFP_KERNEL, fmt, ap);
-   va_end(ap);
+   dev->unique = kstrdup(name, GFP_KERNEL);

return dev->unique ? 0 : -ENOMEM;
 }
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 1d3ee5179ab8..2d23f95f17ce 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1046,7 +1046,7 @@ nouveau_platform_device_create(const struct 
nvkm_device_tegra_func *func,
goto err_free;
}

-   err = drm_dev_set_unique(drm, "%s", dev_name(>dev));
+   err = drm_dev_set_unique(drm, dev_name(>dev));
if (err < 0)
goto err_free;

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c 
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f22e1e1ee64a..215d6c44af55 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -450,7 +450,7 @@ static int rockchip_drm_bind(struct device *dev)
if (!drm)
return -ENOMEM;

-   ret = drm_dev_set_unique(drm, "%s", dev_name(dev));
+   ret = drm_dev_set_unique(drm, dev_name(dev));
if (ret)
goto err_free;

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 0a271ca1f7c7..f14c25a6bbf2 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1059,7 +1059,7 @@ void drm_dev_ref(struct drm_device *dev);
 void drm_dev_unref(struct drm_device *dev);
 int drm_dev_register(struct drm_device *dev, unsigned long flags);
 void drm_dev_unregister(struct drm_device *dev);
-int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...);
+int drm_dev_set_unique(struct drm_device *dev, const char *name);

 struct drm_minor *drm_minor_acquire(unsigned int minor_id);
 void drm_minor_release(struct drm_minor *minor);
-- 
2.6.3

[PATCH] drm: do not use device name as a format string

2015-12-07 Thread Nicolas Iooss 
On 12/07/2015 01:31 PM, Thierry Reding wrote:
> On Mon, Dec 07, 2015 at 12:46:52PM +0100, Daniel Vetter wrote:
>> On Mon, Dec 07, 2015 at 11:53:01AM +0200, Jani Nikula wrote:
>>> On Mon, 07 Dec 2015, Daniel Vetter  wrote:
>>>> On Sun, Dec 06, 2015 at 11:16:32AM +0100, Nicolas Iooss wrote:
>>>>> On 12/06/2015 10:35 AM, Daniel Vetter wrote:
>>>>>>> On 11/18/2015 06:58 PM, Nicolas Iooss wrote:
>>>>>>>> drm_dev_set_unique() formats its parameter using kvasprintf() but many
>>>>>>>> of its callers directly pass dev_name(dev) as printf format string,
>>>>>>>> without any format parameter.  This can cause some issues when the
>>>>>>>> device name contains '%' characters.
>>>>>>>>
>>>>>>>> To avoid any potential issue, always use "%s" when using
>>>>>>>> drm_dev_set_unique() with dev_name().
>>>>>>
>>>>>> Not sure this is worth it really, normally people don't place % 
>>>>>> characters
>>>>>> into their device names, ever. And if they do it'll blow up. There's also
>>>>>> no security issue here since userspace can't set this name.
>>>>>>
>>>>>> If the maintainers of the affected drivers don't want this I won't merge
>>>>>> this patch.
>>>>>
>>>>> Actually I had the same opinion before I began to add __printf
>>>>> attributes and "%s" in several places in the kernel to make
>>>>> -Wformat-security useful.  This led me to discover some funny issues
>>>>> like the one fixed by commit 3958b79266b1 ("configfs: fix kernel
>>>>> infoleak through user-controlled format string",
>>>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3958b79266b14729edd61daf9dfb84de45f4ec6d
>>>>> ).  The patch I sent is in fact a very small step towards making
>>>>> -Wformat-security useful again to detect "real" issues.
>>>>>
>>>>> Of course, if you do not feel it is worth it and believe that dev_name
>>>>> is fully controlled by trusted sources which will never introduce any %
>>>>> character, I understand your will of not merging my patch.
>>>>
>>>> Ah, that's the context I was missing, that really should be in the commit
>>>> message. If this is part of an overall effort to enable something useful
>>>> it makes more sense to get it in.
>>>>
>>>> On the patch itself it feels rather funny to do a "%s", str); combo, maybe
>>>> we should have a drm_dev_set_unique_static instead? Including kerneldoc
>>>> explaining why there's too.
>>>
>>> No caller of drm_dev_set_unique() actually uses the formatting for
>>> anything... so you'd end up with drm_dev_set_unique_static() and an
>>> orphaned drm_dev_set_unique()...
>>
>> Ok, then I guess we can just ditch the printf stuff from set_unique.
>> Nicolas, you're up for that?
> 
> Looking at all the callsites of drm_dev_set_unique() it seems like all
> of the drivers (with the exception of vgem) use dev_name() on the same
> device that's already passed into drm_dev_alloc(), so perhaps another
> alternative would be to have drm_dev_alloc() set the unique name by
> default and keep the function for cases where it needs to be set
> explicitly (like for vgem). vgem passes drm_dev_alloc() a NULL device,
> so that could serve as condition.

I have written a patch which removes the printf format processing from
drm_dev_set_unique().  I will test it as soon as possible and depending
on the results, send it or explain what went wrong.  If no one does the
work before me, I'll also take a look at calling drm_dev_set_unique() in
drm_dev_alloc(), and this would be an other patch.

Thanks,
Nicolas

[PATCH] drm: do not use device name as a format string

2015-12-06 Thread Nicolas Iooss 
On 12/06/2015 10:35 AM, Daniel Vetter wrote:
>> On 11/18/2015 06:58 PM, Nicolas Iooss wrote:
>>> drm_dev_set_unique() formats its parameter using kvasprintf() but many
>>> of its callers directly pass dev_name(dev) as printf format string,
>>> without any format parameter.  This can cause some issues when the
>>> device name contains '%' characters.
>>>
>>> To avoid any potential issue, always use "%s" when using
>>> drm_dev_set_unique() with dev_name().
> 
> Not sure this is worth it really, normally people don't place % characters
> into their device names, ever. And if they do it'll blow up. There's also
> no security issue here since userspace can't set this name.
> 
> If the maintainers of the affected drivers don't want this I won't merge
> this patch.

Actually I had the same opinion before I began to add __printf
attributes and "%s" in several places in the kernel to make
-Wformat-security useful.  This led me to discover some funny issues
like the one fixed by commit 3958b79266b1 ("configfs: fix kernel
infoleak through user-controlled format string",
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3958b79266b14729edd61daf9dfb84de45f4ec6d
).  The patch I sent is in fact a very small step towards making
-Wformat-security useful again to detect "real" issues.

Of course, if you do not feel it is worth it and believe that dev_name
is fully controlled by trusted sources which will never introduce any %
character, I understand your will of not merging my patch.

Regards,
Nicolas

[PATCH] drm: do not use device name as a format string

2015-12-05 Thread Nicolas Iooss 
Hello,
I sent the path below a few weeks ago and did not have any feedback.
Is there any issue in it that I need to fix before submitting it again?

Thanks,
Nicolas Iooss

On 11/18/2015 06:58 PM, Nicolas Iooss wrote:
> drm_dev_set_unique() formats its parameter using kvasprintf() but many
> of its callers directly pass dev_name(dev) as printf format string,
> without any format parameter.  This can cause some issues when the
> device name contains '%' characters.
> 
> To avoid any potential issue, always use "%s" when using
> drm_dev_set_unique() with dev_name().
> 
> Signed-off-by: Nicolas Iooss 
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +-
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c| 2 +-
>  drivers/gpu/drm/tegra/drm.c  | 2 +-
>  drivers/gpu/drm/vc4/vc4_drv.c| 2 +-
>  include/drm/drmP.h   | 1 +
>  5 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c 
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 244df0a440b7..0d720d3a7ee0 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -733,7 +733,7 @@ static int atmel_hlcdc_dc_drm_probe(struct 
> platform_device *pdev)
>   if (!ddev)
>   return -ENOMEM;
>  
> - ret = drm_dev_set_unique(ddev, dev_name(ddev->dev));
> + ret = drm_dev_set_unique(ddev, "%s", dev_name(ddev->dev));
>   if (ret)
>   goto err_unref;
>  
> diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c 
> b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> index 1930234ba5f1..947d75f59881 100644
> --- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> +++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
> @@ -363,7 +363,7 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
>   fsl_dev->np = dev->of_node;
>   drm->dev_private = fsl_dev;
>   dev_set_drvdata(dev, fsl_dev);
> - drm_dev_set_unique(drm, dev_name(dev));
> + drm_dev_set_unique(drm, "%s", dev_name(dev));
>  
>   ret = drm_dev_register(drm, 0);
>   if (ret < 0)
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 159ef515cab1..b278f60f4376 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -991,7 +991,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
>   if (!drm)
>   return -ENOMEM;
>  
> - drm_dev_set_unique(drm, dev_name(>dev));
> + drm_dev_set_unique(drm, "%s", dev_name(>dev));
>   dev_set_drvdata(>dev, drm);
>  
>   err = drm_dev_register(drm, 0);
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index 6e730605edcc..c90a451aaa79 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -168,7 +168,7 @@ static int vc4_drm_bind(struct device *dev)
>   vc4->dev = drm;
>   drm->dev_private = vc4;
>  
> - drm_dev_set_unique(drm, dev_name(dev));
> + drm_dev_set_unique(drm, "%s", dev_name(dev));
>  
>   drm_mode_config_init(drm);
>   if (ret)
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 0b921ae06cd8..995fb96cb740 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1049,6 +1049,7 @@ void drm_dev_ref(struct drm_device *dev);
>  void drm_dev_unref(struct drm_device *dev);
>  int drm_dev_register(struct drm_device *dev, unsigned long flags);
>  void drm_dev_unregister(struct drm_device *dev);
> +__printf(2, 3)
>  int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...);
>  
>  struct drm_minor *drm_minor_acquire(unsigned int minor_id);
>

[PATCH] drm: do not use device name as a format string

2015-11-18 Thread Nicolas Iooss 
drm_dev_set_unique() formats its parameter using kvasprintf() but many
of its callers directly pass dev_name(dev) as printf format string,
without any format parameter.  This can cause some issues when the
device name contains '%' characters.

To avoid any potential issue, always use "%s" when using
drm_dev_set_unique() with dev_name().

Signed-off-by: Nicolas Iooss 
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c| 2 +-
 drivers/gpu/drm/tegra/drm.c  | 2 +-
 drivers/gpu/drm/vc4/vc4_drv.c| 2 +-
 include/drm/drmP.h   | 1 +
 5 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c 
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 244df0a440b7..0d720d3a7ee0 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -733,7 +733,7 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device 
*pdev)
if (!ddev)
return -ENOMEM;

-   ret = drm_dev_set_unique(ddev, dev_name(ddev->dev));
+   ret = drm_dev_set_unique(ddev, "%s", dev_name(ddev->dev));
if (ret)
goto err_unref;

diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c 
b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index 1930234ba5f1..947d75f59881 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -363,7 +363,7 @@ static int fsl_dcu_drm_probe(struct platform_device *pdev)
fsl_dev->np = dev->of_node;
drm->dev_private = fsl_dev;
dev_set_drvdata(dev, fsl_dev);
-   drm_dev_set_unique(drm, dev_name(dev));
+   drm_dev_set_unique(drm, "%s", dev_name(dev));

ret = drm_dev_register(drm, 0);
if (ret < 0)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 159ef515cab1..b278f60f4376 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -991,7 +991,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
if (!drm)
return -ENOMEM;

-   drm_dev_set_unique(drm, dev_name(>dev));
+   drm_dev_set_unique(drm, "%s", dev_name(>dev));
dev_set_drvdata(>dev, drm);

err = drm_dev_register(drm, 0);
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 6e730605edcc..c90a451aaa79 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -168,7 +168,7 @@ static int vc4_drm_bind(struct device *dev)
vc4->dev = drm;
drm->dev_private = vc4;

-   drm_dev_set_unique(drm, dev_name(dev));
+   drm_dev_set_unique(drm, "%s", dev_name(dev));

drm_mode_config_init(drm);
if (ret)
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 0b921ae06cd8..995fb96cb740 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1049,6 +1049,7 @@ void drm_dev_ref(struct drm_device *dev);
 void drm_dev_unref(struct drm_device *dev);
 int drm_dev_register(struct drm_device *dev, unsigned long flags);
 void drm_dev_unregister(struct drm_device *dev);
+__printf(2, 3)
 int drm_dev_set_unique(struct drm_device *dev, const char *fmt, ...);

 struct drm_minor *drm_minor_acquire(unsigned int minor_id);
-- 
2.6.2

[PATCH 2/2] drm/vmwgfx: make vmw_cotable_unbind return an initialized value

2015-09-20 Thread Nicolas Iooss 
In vmw_cotable_unbind(), local variable ret is never initialized before
being used in a return statement at the end of the function.  Fix this
by directly returning zero and removing the variable.

Signed-off-by: Nicolas Iooss 
---
 drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c
index ce659a125f2b..092ea81eeff7 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cotable.c
@@ -311,7 +311,6 @@ static int vmw_cotable_unbind(struct vmw_resource *res,
struct vmw_private *dev_priv = res->dev_priv;
struct ttm_buffer_object *bo = val_buf->bo;
struct vmw_fence_obj *fence;
-   int ret;

if (list_empty(>mob_head))
return 0;
@@ -328,7 +327,7 @@ static int vmw_cotable_unbind(struct vmw_resource *res,
if (likely(fence != NULL))
vmw_fence_obj_unreference();

-   return ret;
+   return 0;
 }

 /**
-- 
2.5.2

[PATCH 1/2] drm/vmwgfx: make vmw_kms_helper_dirty return an initialized value

2015-09-20 Thread Nicolas Iooss 
In vmw_kms_helper_dirty(), local variable ret is never initialized
before begin used in a return statement when vmw_fifo_reserve() fails.
Instead of returning an uninitialized value, return -ENOMEM here and
remove the useless variable.

Signed-off-by: Nicolas Iooss 
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 61fb7f3de311..15a6c01cd016 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -1685,7 +1685,6 @@ int vmw_kms_helper_dirty(struct vmw_private *dev_priv,
struct drm_crtc *crtc;
u32 num_units = 0;
u32 i, k;
-   int ret;

dirty->dev_priv = dev_priv;

@@ -1711,7 +1710,7 @@ int vmw_kms_helper_dirty(struct vmw_private *dev_priv,
if (!dirty->cmd) {
DRM_ERROR("Couldn't reserve fifo space "
  "for dirty blits.\n");
-   return ret;
+   return -ENOMEM;
}
memset(dirty->cmd, 0, dirty->fifo_reserve_size);
}
-- 
2.5.2

[PATCH] drm/amdgpu: increment queue when iterating on this variable.

2015-08-01 Thread Nicolas Iooss 
gfx_v7_0_print_status contains a for loop on variable queue which does
not update this variable between each iteration.  This is bug is
reported by clang while building allmodconfig LLVMLinux on x86_64:

drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c:5126:19: error: variable
'queue' used in loop condition not modified in loop body
[-Werror,-Wloop-analysis]
for (queue = 0; queue < 8; i++) {
^

Fix this by incrementing variable queue instead of i in this loop.

Signed-off-by: Nicolas Iooss 
---
 drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c 
b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
index 2db6ab0a543d..5c03420ca5dc 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
@@ -5122,7 +5122,7 @@ static void gfx_v7_0_print_status(void *handle)
dev_info(adev->dev, "  CP_HPD_EOP_CONTROL=0x%08X\n",
 RREG32(mmCP_HPD_EOP_CONTROL));

-   for (queue = 0; queue < 8; i++) {
+   for (queue = 0; queue < 8; queue++) {
cik_srbm_select(adev, me, pipe, queue, 0);
dev_info(adev->dev, "  queue: %d\n", queue);
dev_info(adev->dev, "  CP_PQ_WPTR_POLL_CNTL=0x%08X\n",
-- 
2.5.0