Re: [PATCH 1/1] drm/amd/powerplay: initialize a variable before using it
On Sun, Sep 3, 2017 at 2:00 PM, Nicolas Iooss 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 > --- > 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
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 --- 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
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 --- 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
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 --- 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()
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
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(&encoder->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
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 &data 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 &data, 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 positive
[Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value
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 &data 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 &data, 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, &data, 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(&encoder->base); >>> struct mipi_dsi_device *dsi_device; >>> enum port port; >>> u8 data; >>> >>> /* FIXME: Need to take care of 16 bit brightness level */ >>> for
[Intel-gfx] [PATCH 1/1] drm/i915/dsi: silence a warning about uninitialized return value
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 &data 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 &data, 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, &data, 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(&encoder->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, &data, nbytes); > return nbytes == sizeof(data) ? data : 0; > } > > and if complains about that then it doesn't understand that passing > &data 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
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(&encoder->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
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(&adev->pdev->dev); -- 2.8.3
[PATCH v2 2/2] drm: use dev_name as default unique name in drm_dev_alloc()
The following code pattern exists in some DRM drivers: ddev = drm_dev_alloc(&driver, 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(&dev->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(&pdev->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)); dev_set_drvdata(&dev->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
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(&pdev->dev)); + err = drm_dev_set_unique(drm, dev_name(&pdev->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
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()
The following code pattern exists in some DRM drivers: ddev = drm_dev_alloc(&driver, 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(&dev->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(&pdev->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)); dev_set_drvdata(&dev->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
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(&pdev->dev)); + err = drm_dev_set_unique(drm, dev_name(&pdev->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
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
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
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->dev)); > + drm_dev_set_unique(drm, "%s", dev_name(&dev->dev)); > dev_set_drvdata(&dev->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
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->dev)); + drm_dev_set_unique(drm, "%s", dev_name(&dev->dev)); dev_set_drvdata(&dev->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
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(&res->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(&fence); - return ret; + return 0; } /** -- 2.5.2
[PATCH 1/2] drm/vmwgfx: make vmw_kms_helper_dirty return an initialized value
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.
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