linux-kernel - Re: [PATCH v1] soc: rockchip: power-domain: Fix clang warning about negative shift count

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: <1715023.AuHs9buZxh@phil>
Date:   Sat, 11 Mar 2017 13:03:48 +0100
From:   Heiko Stuebner <heiko@...ech.de>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     Elaine Zhang <zhangqing@...k-chips.com>,
        Caesar Wang <wxt@...k-chips.com>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        Douglas Anderson <dianders@...omium.org>,
        Grant Grundler <grundler@...omium.org>,
        linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] soc: rockchip: power-domain: Fix clang warning about negative shift count

Hi Matthias,

Am Freitag, 10. März 2017, 18:21:53 CET schrieb Matthias Kaehlcke:
> The following warning is generated when building with clang:
> 
> drivers/soc/rockchip/pm_domains.c:726:22: error: shift count is negative
> [-Werror,-Wshift-count-negative] [RK3399_PD_TCPD0]       = DOMAIN_RK3399(8,
> 8, -1, false),
>                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/soc/rockchip/pm_domains.c:101:2: note: expanded from macro
> 'DOMAIN_RK3399' DOMAIN(pwr, status, req, req, req, wakeup)
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/soc/rockchip/pm_domains.c:88:27: note: expanded from macro 'DOMAIN'
>         .req_mask = (req >= 0) ? BIT(req) : 0,          \
>                                  ^~~~~~~~
> include/linux/bitops.h:6:24: note: expanded from macro 'BIT'
> 
> The BIT macro is evaluated with the negative value -1, even though the
> resulting value would not be assigned. To fix this we only pass values
> between 0 and 63 to BIT(). Unfortunately this means that we lose the
> benefit of the compiler checking for out of bounds errors.

I tend to disagree here. This looks more like a case of "fix your compiler".

That conditional seems perfectly valid as the BIT(req) will never be reached 
if req < 0 - your clang simply doesn't recognize the pattern somehow, while 
for example gcc does.

Catering to specific whims of specific compilers feels somehow wrong, as what 
will happen if some imaginary third compiler requires another different hack 
to be satisfied?


Heiko

> Signed-off-by: Matthias Kaehlcke <mka@...omium.org>
> ---
>  drivers/soc/rockchip/pm_domains.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/soc/rockchip/pm_domains.c
> b/drivers/soc/rockchip/pm_domains.c index 1c78c42416c6..6f2bb1222992 100644
> --- a/drivers/soc/rockchip/pm_domains.c
> +++ b/drivers/soc/rockchip/pm_domains.c
> @@ -77,13 +77,15 @@ struct rockchip_pmu {
> 
>  #define to_rockchip_pd(gpd) container_of(gpd, struct rockchip_pm_domain,
> genpd)
> 
> +#define RK_MASK(bit) ((bit >= 0) ? BIT(bit & 0x3f) : 0)
> +
>  #define DOMAIN(pwr, status, req, idle, ack, wakeup)	\
> -{						\
> -	.pwr_mask = (pwr >= 0) ? BIT(pwr) : 0,		\
> -	.status_mask = (status >= 0) ? BIT(status) : 0,	\
> -	.req_mask = (req >= 0) ? BIT(req) : 0,		\
> -	.idle_mask = (idle >= 0) ? BIT(idle) : 0,	\
> -	.ack_mask = (ack >= 0) ? BIT(ack) : 0,		\
> +{							\
> +	.pwr_mask = RK_MASK(pwr),			\
> +	.status_mask = RK_MASK(status),			\
> +	.req_mask = RK_MASK(req),			\
> +	.idle_mask = RK_MASK(idle),			\
> +	.ack_mask = RK_MASK(ack),			\
>  	.active_wakeup = wakeup,			\
>  }