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

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

[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 <nicolas.iooss_linux@m4x.org>
---
 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

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

[Dave Gordon]

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 <nicolas.iooss_linux@m4x.org>

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:

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. 
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.

Here's an R-b for your fix anyway ...

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

.Dave.

>  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 */
>

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

[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 <nicolas.iooss_linux@m4x.org>
> 
> 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 <david.s.gordon@intel.com>

Thanks!

Nicolas


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

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

[Dave Gordon]

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 <nicolas.iooss_linux@m4x.org>
>>
>> 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.

So it does, In that case the complaint is really quite reasonable.

>> 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.

It probably should have, since the (hidden) if() could still result in 
this function never being called. Oh well ...

.Dave.

>> Here's an R-b for your fix anyway ...
>>
>> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
>
> Thanks!
> Nicolas
>
> [1] I used "make drivers/gpu/drm/i915/intel_dsi_dcs_backlight.i" to see
> the output of the preprocessor.

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

[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 <nicolas.iooss_linux@m4x.org>
>>>
>>> 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.
> 
> So it does, In that case the complaint is really quite reasonable.
> 
>>> 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.
> 
> It probably should have, since the (hidden) if() could still result in
> this function never being called. Oh well ...

Sorry, my message was not clear enough. The compiler did not complain in
mipi_dsi_dcs_read1() in the last case, but the -Wsometimes-uninitialized
warning was still there for variable 'data' in dcs_get_backlight(), as
expected because of the "hidden if".

Nicolas

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

[Dave Gordon]

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 <nicolas.iooss_linux@m4x.org>
>>>>
>>>> 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.
>>
>> So it does, In that case the complaint is really quite reasonable.
>>
>>>> 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.
>>
>> It probably should have, since the (hidden) if() could still result in
>> this function never being called. Oh well ...
>
> Sorry, my message was not clear enough. The compiler did not complain in
> mipi_dsi_dcs_read1() in the last case, but the -Wsometimes-uninitialized
> warning was still there for variable 'data' in dcs_get_backlight(), as
> expected because of the "hidden if".
>
> Nicolas

OK, thanks.

BTW do you see any "may be used uninitialised" warnings in
gen{6,8}_ggtt_insert_entries()? In particular

     for_each_sgt_dma(addr, sgt_iter, st) {
         gtt_entry = gen8_pte_encode(addr, level, true);
         gen8_set_pte(&gtt_entries[i++], gtt_entry);
     }

[snip]

     if (i != 0)
         WARN_ON(readq(&gtt_entries[i-1]) != gtt_entry);

Or maybe clang is smart enough to realise that the WARN_ON() is 
reachable only if the gen8_set_pte() has already been executed at least 
once?

.Dave.

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

[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 <nicolas.iooss_linux@m4x.org>
>>>>>
>>>>> 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.
>>>
>>> So it does, In that case the complaint is really quite reasonable.
>>>
>>>>> 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.
>>>
>>> It probably should have, since the (hidden) if() could still result in
>>> this function never being called. Oh well ...
>>
>> Sorry, my message was not clear enough. The compiler did not complain in
>> mipi_dsi_dcs_read1() in the last case, but the -Wsometimes-uninitialized
>> warning was still there for variable 'data' in dcs_get_backlight(), as
>> expected because of the "hidden if".
>>
>> Nicolas
> 
> OK, thanks.
> 
> BTW do you see any "may be used uninitialised" warnings in
> gen{6,8}_ggtt_insert_entries()? In particular
> 
>     for_each_sgt_dma(addr, sgt_iter, st) {
>         gtt_entry = gen8_pte_encode(addr, level, true);
>         gen8_set_pte(&gtt_entries[i++], gtt_entry);
>     }
> 
> [snip]
> 
>     if (i != 0)
>         WARN_ON(readq(&gtt_entries[i-1]) != gtt_entry);
> 
> Or maybe clang is smart enough to realise that the WARN_ON() is
> reachable only if the gen8_set_pte() has already been executed at least
> once?
> 
> .Dave.

The clang I am using (version 3.8.1) does not report any
-Wsometimes-uninitialized warning in
drivers/gpu/drm/i915/i915_gem_gtt.c. Even when I introduce a statement
which uses gtt_entry right after the for_each_sgt_dma loop (like
printk("%llu", gtt_entry);) no warning is produced.
Therefore the lack of the warning does not mean the compiler is finding
that i != 0 only when the loop got executed once, I guess the loop
condition is too complex for the compiler to decide whether the loop
body is always executed.

Nicolas

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

[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 <nicolas.iooss_linux@m4x.org>
> ---
>  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 */
> 

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

[Chris Wilson]

On Sun, Oct 23, 2016 at 06:55:58PM +0200, Nicolas Iooss wrote:
> 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?

It basically boils down to that you said it doesn't fix a bug, but shuts
up a non-default compiler using custom warning flags.

But there is a bug in that code, as mipi_dsi_dcs_read() may return an
error value, which may just be ENOMEM, but if it did we would return the
garbage. Not only should we fix the error handling here, but you also
need to fix the error handling in drivers/video/backlight/backlight.c as
despite many callees returning an error, it assumes that
bd->ops->get_brightness() never returns an error...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre