linux-kernel - Re: [PATCH] drm/ssd130x: Silence a `dubious: x & !y` warning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c7a21f75-35c8-486b-1f10-7c536573cd2a@redhat.com>
Date:   Mon, 23 Jan 2023 10:33:15 +0100
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     David Laight <David.Laight@...LAB.COM>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc:     Thomas Zimmermann <tzimmermann@...e.de>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...il.com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH] drm/ssd130x: Silence a `dubious: x & !y` warning

Hello David,

Thanks for your feedback.

On 1/23/23 10:26, David Laight wrote:
> From: Javier Martinez Canillas
>> Sent: 21 January 2023 19:10
>>
>> The sparse tool complains with the following warning:
>>
>> $ make M=drivers/gpu/drm/solomon/ C=2
>>   CC [M]  drivers/gpu/drm/solomon/ssd130x.o
>>   CHECK   drivers/gpu/drm/solomon/ssd130x.c
>> drivers/gpu/drm/solomon/ssd130x.c:363:21: warning: dubious: x & !y
>>
>> This seems to be a false positive in my opinion but still we can silence
>> the tool while making the code easier to read. Let's also add a comment,
>> to explain why the "com_seq" logical not is used rather than its value.
>>
>> Reported-by: Thomas Zimmermann <tzimmermann@...e.de>
>> Signed-off-by: Javier Martinez Canillas <javierm@...hat.com>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@...e.de>
>> ---
>>
>>  drivers/gpu/drm/solomon/ssd130x.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
>> index c3bf3a18302e..b16330a8b624 100644
>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> @@ -81,7 +81,7 @@
>>  #define SSD130X_SET_PRECHARGE_PERIOD2_MASK	GENMASK(7, 4)
>>  #define SSD130X_SET_PRECHARGE_PERIOD2_SET(val)	FIELD_PREP(SSD130X_SET_PRECHARGE_PERIOD2_MASK, (val))
>>  #define SSD130X_SET_COM_PINS_CONFIG1_MASK	GENMASK(4, 4)
>> -#define SSD130X_SET_COM_PINS_CONFIG1_SET(val)	FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, !(val))
>> +#define SSD130X_SET_COM_PINS_CONFIG1_SET(val)	FIELD_PREP(SSD130X_SET_COM_PINS_CONFIG1_MASK, (val))
> 
> How about just changing !(val) to (val) ? 0 : 1
> It should shut the compiler up and is probably more descriptive.
> 

I'm not sure if is more descriptive than what I have with this patch. As
mentioned, I believe this really is a sparse false positive but given that
need to change, I preferred to make it quite explicit with a new variable
and a comment.

This is only executed once at driver probe time anyways.

> 	David

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat