From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8C9D8C388F7 for ; Mon, 9 Nov 2020 14:39:42 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EE719206D8 for ; Mon, 9 Nov 2020 14:39:41 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="huI4s/la"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="K9SQgIU8" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EE719206D8 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type:Cc: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=UXZlxI6oY5SlyOy3TozjMI3wLgUUbzXyN8R9PX40yxQ=; b=huI4s/laiJ+EKzC/WwF02tsKQ Fvib4ZIrWazt1HYJljrnGcZrr81vGyZR744xgtyXG7Ge4EVtkZ6Ti2nVzCQG3Gch/NqumvQEADQPj a/D4QuYiIAs4A77ig+b1H2sqsWDFbIjRV8RLBtcHj/IWR1DK+6rk1+y9jryU/xSR5QeOhN4tidk5z 5f2GmXBZ0I9hxaqA9zRhExJKgNa2jndUF/QXhmckY8LTo/6GtofynMwvbJQR6QqBoDceRN0F9kNV6 1gM+HbXN39ZyZoKKBd0uIE4i0KMy1HFrywPQ03R8ClAE/GFFKiqYrieSPteESfQGKWQXBaIJXNExm 2VVdlP89A==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kc8K3-00060H-L8; Mon, 09 Nov 2020 14:39:08 +0000 Received: from mail-qv1-xf44.google.com ([2607:f8b0:4864:20::f44]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kc8Jz-0005zH-8f for linux-arm-kernel@lists.infradead.org; Mon, 09 Nov 2020 14:39:04 +0000 Received: by mail-qv1-xf44.google.com with SMTP id e5so2408831qvs.1 for ; Mon, 09 Nov 2020 06:39:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=31xujWu4MTA/dbqXe+y3ET/5R+dp3A0kcbn2zTL5Pxo=; b=K9SQgIU8CwVPfN/tbEIN/EI3GEuTrtbM4ZJonIeRM3riwz9jxsQhk1Hlvn8Aue4NOd q+Ze/oMa+rELKATFODOlT70gIPFiRTTi/EuY7YUCr5UU5Fw8csRfMLz54GoCuxtk17Qv T3ZZB3O7iSQcG75omFxXeo1r2/F8fdYVg04cLQoqbvb+dQ+Ws6/nV80c8a+3+bSM8i+w 7gV6SsqYvKEARofdaENuizQmhdjhYEtw3IrqcbeXmEqnzvvBRi0c/wa+9PbGi7Isomwv X10aisi16wBCST5193wmMb3l4Cu7/VzCs8AkZURqpE0E3xWQa4VdP7FqcaIxugY7/wB8 N57Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=31xujWu4MTA/dbqXe+y3ET/5R+dp3A0kcbn2zTL5Pxo=; b=ZcKR6uiAm7f9Y00RcIzJI/XWHwbhvE4Z4K1WcEUfwPHG2XZ6CgFTEpLmfY28CFzKMJ 2N9eZOJF9dVWD8XUBjeS/fQ7oSGsFuUMPMnLPBkYy0HiMN6HfIj/n3I7qbG08NC2C9ev gOFYytbuP5/zoQm+d1kEjIoAxo9IROZ7hiVmb6qWiyZLvxH7reFsWSccHvhEFPyoWexK e4OvfqgrLoRX3Qk5vkBOIoqdsMRcQ0Ys0PWXxFMNSsVeeXn3GemKtHMWn3Zzw2x1ssiY xE/jReaPMclL36iLgd+aepLkue1fC/sZCbI4jlk3EJbDqfElX10CJo69rvk4aBz1gMjU ZxFQ== X-Gm-Message-State: AOAM533VlZSl74yNVvgA7bHJfYZXcmMl8K0Ve87NGf5V7dOIHKeFLp9Z +oTeobxMpz13o7eE6sVttjQ= X-Google-Smtp-Source: ABdhPJx5mq8pmTpYL1RXJznOcjIqENswLfHOZSM9RQwuu0hNem/FBb93fCOj4QUJhtcFLVbwAP1CuA== X-Received: by 2002:a0c:fac6:: with SMTP id p6mr14767275qvo.5.1604932741507; Mon, 09 Nov 2020 06:39:01 -0800 (PST) Received: from shinobu (072-189-064-225.res.spectrum.com. [72.189.64.225]) by smtp.gmail.com with ESMTPSA id z16sm6498683qka.18.2020.11.09.06.38.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 09 Nov 2020 06:39:00 -0800 (PST) Date: Mon, 9 Nov 2020 09:38:45 -0500 From: William Breathitt Gray To: Arnd Bergmann , Syed Nayyar Waris Subject: Re: [PATCH v12 4/4] gpio: xilinx: Utilize generic bitmap_get_value and _set_value Message-ID: <20201109143845.GA8636@shinobu> References: <15a044d3ba23f00c31fd09437bdd3e5924bb91cd.1603055402.git.syednwaris@gmail.com> <20201101150033.GA68138@shinobu> <20201109123411.GA19869@syed> <20201109134128.GA5596@shinobu> MIME-Version: 1.0 In-Reply-To: <20201109134128.GA5596@shinobu> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201109_093903_628326_25C2983B X-CRM114-Status: GOOD ( 56.01 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Bartosz Golaszewski , Linus Walleij , Michal Simek , "linux-kernel@vger.kernel.org" , "open list:GPIO SUBSYSTEM" , Andrew Morton , Andy Shevchenko , Linux ARM Content-Type: multipart/mixed; boundary="===============3888285091699497266==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============3888285091699497266== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="3MwIy2ne0vdjdPXF" Content-Disposition: inline --3MwIy2ne0vdjdPXF Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Nov 09, 2020 at 08:41:28AM -0500, William Breathitt Gray wrote: > On Mon, Nov 09, 2020 at 06:04:11PM +0530, Syed Nayyar Waris wrote: > > On Sun, Nov 01, 2020 at 09:08:29PM +0100, Arnd Bergmann wrote: > > > On Sun, Nov 1, 2020 at 4:00 PM William Breathitt Gray > > > wrote: > > > > > > > > On Thu, Oct 29, 2020 at 11:44:47PM +0100, Arnd Bergmann wrote: > > > > > On Sun, Oct 18, 2020 at 11:44 PM Syed Nayyar Waris wrote: > > > > > > > > > > > > This patch reimplements the xgpio_set_multiple() function in > > > > > > drivers/gpio/gpio-xilinx.c to use the new generic functions: > > > > > > bitmap_get_value() and bitmap_set_value(). The code is now simp= ler > > > > > > to read and understand. Moreover, instead of looping for each b= it > > > > > > in xgpio_set_multiple() function, now we can check each channel= at > > > > > > a time and save cycles. > > > > > > > > > > This now causes -Wtype-limits warnings in linux-next with gcc-10: > > > > > > > > Hi Arnd, > > > > > > > > What version of gcc-10 are you running? I'm having trouble generati= ng > > > > these warnings so I suspect I'm using a different version than you. > > >=20 > > > I originally saw it with the binaries from > > > https://mirrors.edge.kernel.org/pub/tools/crosstool/, but I have > > > also been able to reproduce it with a minimal test case on the > > > binaries from godbolt.org, see https://godbolt.org/z/Wq8q4n > > >=20 > > > > Let me first verify that I understand the problem correctly. The is= sue > > > > is the possibility of a stack smash in bitmap_set_value() when the = value > > > > of start + nbits is larger than the length of the map bitmap memory > > > > region. This is because index (or index + 1) could be outside the r= ange > > > > of the bitmap memory region passed in as map. Is my understanding > > > > correct here? > > >=20 > > > Yes, that seems to be the case here. > > >=20 > > > > In xgpio_set_multiple(), the variables width[0] and width[1] serve = as > > > > possible start and nbits values for the bitmap_set_value() calls. > > > > Because width[0] and width[1] are unsigned int variables, GCC consi= ders > > > > the possibility that the value of width[0]/width[1] might exceed the > > > > length of the bitmap memory region named old and thus result in a s= tack > > > > smash. > > > > > > > > I don't know if invalid width values are actually possible for the > > > > Xilinx gpio device, but let's err on the side of safety and assume = this > > > > is actually a possibility. We should verify that the combined value= of > > > > gpio_width[0] + gpio_width[1] does not exceed 64 bits; we can add a > > > > check for this in xgpio_probe() when we grab the gpio_width values. > > > > > > > > However, we're still left with the GCC warnings because GCC is not = smart > > > > enough to know that we've already checked the boundary and width[0]= and > > > > width[1] are valid values. I suspect we can avoid this warning is we > > > > refactor bitmap_set_value() to increment map seperately and then se= t it: > > >=20 > > > As I understand it, part of the problem is that gcc sees the possible > > > range as being constrained by the operations on 'start' and 'nbits', > > > in particular the shift in BIT_WORD() that put an upper bound on > > > the index, but then it sees that the upper bound is higher than the > > > upper bound of the array, i.e. element zero. > > >=20 > > > I added a check > > >=20 > > > if (start >=3D 64 || start + size >=3D 64) return; > > >=20 > > > in the godbolt.org testcase, which does help limit the start > > > index appropriately, but it is not sufficient to let the compiler > > > see that the 'if (space >=3D nbits) ' condition is guaranteed to > > > be true for all values here. > > >=20 > > > > static inline void bitmap_set_value(unsigned long *map, > > > > unsigned long value, > > > > unsigned long start, unsigned l= ong nbits) > > > > { > > > > const unsigned long offset =3D start % BITS_PER_LONG; > > > > const unsigned long ceiling =3D round_up(start + 1, BITS_PE= R_LONG); > > > > const unsigned long space =3D ceiling - start; > > > > > > > > map +=3D BIT_WORD(start); > > > > value &=3D GENMASK(nbits - 1, 0); > > > > > > > > if (space >=3D nbits) { > > > > *map &=3D ~(GENMASK(nbits - 1, 0) << offset); > > > > *map |=3D value << offset; > > > > } else { > > > > *map &=3D ~BITMAP_FIRST_WORD_MASK(start); > > > > *map |=3D value << offset; > > > > map++; > > > > *map &=3D ~BITMAP_LAST_WORD_MASK(start + nbits); > > > > *map |=3D value >> space; > > > > } > > > > } > > > > > > > > This avoids adding a costly conditional check inside bitmap_set_val= ue() > > > > when almost all bitmap_set_value() calls will have static arguments= with > > > > well-defined and obvious boundaries. > > > > > > > > Do you think this would be an acceptable solution to resolve your G= CC > > > > warnings? > > >=20 > > > Unfortunately, it does not seem to make a difference, as gcc still > > > knows that this compiles to the same result, and it produces the same > > > warning as before (see https://godbolt.org/z/rjx34r) > > >=20 > > > Arnd > >=20 > > Hi Arnd, > >=20 > > Sharing a different version of bitmap_set_valuei() function. See below. > >=20 > > Let me know if the below solution looks good to you and if it resolves > > the above compiler warning. > >=20 > >=20 > > @@ -1,5 +1,5 @@ > > static inline void bitmap_set_value(unsigned long *map, > > - unsigned long value, > > + unsigned long value, const size_t = length, > > unsigned long start, unsigned long= nbits) > > { > > const size_t index =3D BIT_WORD(start); > > @@ -7,6 +7,9 @@ static inline void bitmap_set_value(unsigned long *map, > > const unsigned long ceiling =3D round_up(start + 1, BITS_PER_L= ONG); > > const unsigned long space =3D ceiling - start; > > =20 > > + if (index >=3D length) > > + return; > > + > > value &=3D GENMASK(nbits - 1, 0); > > =20 > > if (space >=3D nbits) { > > @@ -15,6 +18,10 @@ static inline void bitmap_set_value(unsigned long *m= ap, > > } else { > > map[index + 0] &=3D ~BITMAP_FIRST_WORD_MASK(start); > > map[index + 0] |=3D value << offset; > > + > > + if (index + 1 >=3D length) > > + return; > > + > > map[index + 1] &=3D ~BITMAP_LAST_WORD_MASK(start + nbi= ts); > > map[index + 1] |=3D value >> space; > > } >=20 > One of my concerns is that we're incurring the latency two additional > conditional checks just to suppress a compiler warning about a case that > wouldn't occur in the actual use of bitmap_set_value(). I'm hoping > there's a way for us to suppress these warnings without adding onto the > latency of this function; given that bitmap_set_value() is intended to > be used in loops, conditionals here could significantly increase latency > in drivers. >=20 > I wonder if array_index_nospec() might have the side effect of > suppressing these warnings for us. For example, would this work: >=20 > static inline void bitmap_set_value(unsigned long *map, > unsigned long value, > unsigned long start, unsigned long nbits) > { > const unsigned long offset =3D start % BITS_PER_LONG; > const unsigned long ceiling =3D round_up(start + 1, BITS_PER_LONG); > const unsigned long space =3D ceiling - start; > size_t index =3D BIT_WORD(start); >=20 > value &=3D GENMASK(nbits - 1, 0); >=20 > if (space >=3D nbits) { > index =3D array_index_nospec(index, index + 1); >=20 > map[index] &=3D ~(GENMASK(nbits - 1, 0) << offset); > map[index] |=3D value << offset; > } else { > index =3D array_index_nospec(index, index + 2); >=20 > map[index + 0] &=3D ~BITMAP_FIRST_WORD_MASK(start); > map[index + 0] |=3D value << offset; > map[index + 1] &=3D ~BITMAP_LAST_WORD_MASK(start + nbits); > map[index + 1] |=3D value >> space; > } > } >=20 > Or is this going to produce the same warning because we're not using an > explicit check against the map array size? >=20 > William Breathitt Gray After testing my suggestion, it looks like the warnings are still present. :-( Something else I've also considered is perhaps using the GCC built-in function __builtin_unreachable() instead of returning. So in Syed's code we would have the following instead: if (index + 1 >=3D length) __builtin_unreachable(); This might allow GCC to optimize better and avoid the conditional check all together, thus avoiding latency while also hinting enough context to the compiler to suppress the warnings. William Breathitt Gray --3MwIy2ne0vdjdPXF Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEk5I4PDJ2w1cDf/bghvpINdm7VJIFAl+pVFUACgkQhvpINdm7 VJJW1BAAk6xJJmtHS69OnMCwCLKFypsREz67ottZekyI0GVuxyVGYk9MhSlxi/P4 +GPwnuLiZU2QR7sg6oi24oOdbw19WjYM5Ogbj4B66fbiBRUVoptGopZ3tkIRSX4b wh1q2UEPS5fgDkqVpr/8b3QuSkO8QlGs1ypS94wqSGRfMiW+AvE2RRkj2WzyiRPq KkDGef+sdRD/djGNFOln7mnpDYl4Saa1UEY9OFo+YJkL4BTXSjU5MlNi97xL/ORT 4wFsdKK/hx27anhb3NT8bgwknMrKhtfQrOQESNouaEau9Ggjdv+KFnt8Ykw7tgeV 5bg3U4eKFRpCahF7RpGQJxeA9/bmg2kYDBiy0WPl1IEkwpw91q2PKRexVbzlwvHd piPYrl+prhzdr9bt00Felr4YstGS4YmgEYe9tzBZR0ubgNDGH5vokaSlBw1kcld+ 7k9SdABS2IujnjMy/UYRN2qzkb6QfnqQ9f4gY/Zu2CbEkZb2EuJRGfJabAZ6D2uZ Z3Hnapj/kmhYUPkJIMmxiYlBG1ZqX5t7o2SXlRRrlNS6LO122+9MASVHqygjiYLi MNz7nhgJmoMngyQShA9AkQ3xrjtNDEGW5TmvSFMQ2pwWNUyC7a16+IdrZz9Gxpzh Ts1p6jfHpWP/dUngIWcVxMpX/8AxsIDua5FgLeVSRe/uMvqlMTI= =1zr1 -----END PGP SIGNATURE----- --3MwIy2ne0vdjdPXF-- --===============3888285091699497266== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============3888285091699497266==--