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=-0.7 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 ECCAFC433B4 for ; Mon, 17 May 2021 13:51:48 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (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 786FB60FF0 for ; Mon, 17 May 2021 13:51:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 786FB60FF0 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=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=pPthCT5Shliy+kak8apheixcVnMm28x5o8onhXEp/Gw=; b=Y3VmgkHWZpS/JJIkrqbm6c9Eh eZrsFqsywDdtKbTJa6GQSB407aA6UaVx2tFwiNtiio7Cyk2FJu6oToyFpfrUhqtw7tf9D/37KVFDc kFa1M2qOac1VWGqHSPcdlETZ1jNv8nilwTrHAmolSpTe7dfU6DvGOKMtW4Dc26qD2be6zuqlz19WV O/DdjcTR4x0a+ypQKlmxo96+lImMKmAAdvkgHW8ppqkMR+lSWpS7lsCbvMG7v4NHEIOJQs4RBqpT5 X7Mn7mPc/xqwsfHn4Vg3qYt08EFtWbCcFvQqMilRYOlXdepQ+ZLQ02gMBgYKDziXMuCe0R6fC9DmT YFc/3g1Mw==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1liddE-00F8Oq-27; Mon, 17 May 2021 13:50:04 +0000 Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1liddB-00F8Ik-IK for linux-arm-kernel@desiato.infradead.org; Mon, 17 May 2021 13:50:01 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20210309; h=In-Reply-To:Content-Type:MIME-Version :References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=OGVoYlvb6VsK74BAg0fwXDgO1KtsG8g0pSj7ZCQLsbQ=; b=rCY7ZIk/iVuB2pz0/1oElHoCSx icE4hlNnR62sHVr7D2zaDVAZuQ8aq0NWtdJ1E63erJGquo8vSZoSVSsbYKUYVn3TQs8VDnQlAC/wc 7MNZlR8cOH4LRG/Qgl/APAqWAdeujeCGQWp09HzE4bph8EwKG+Jsl6t17ZlguxVxfl/Q/Z3/KIuHh Z6HOJmbILf9A0BwxQ29pq0+H/9zymR4O6XZzLt+za+fb9hdNAuNPaNWFBrpj9FX9Hxt+JpVCgHoGc OcOhHWThokX7C5iIfN4bMp5Hh2oMc6K8qLD7GStqkJN4MKu69neg1nUx0rpuSGFpziTgbL9bIOqnA l/nFmQvQ==; Received: from mga07.intel.com ([134.134.136.100]) by bombadil.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lidd2-00DoRk-PQ for linux-arm-kernel@lists.infradead.org; Mon, 17 May 2021 13:50:00 +0000 IronPort-SDR: Ykfdbj5LlTUeNmzVNAzm/h4ModhHCmMMg/07AiNQGNpDpVK2YMGq9W1yJl9gCOSlnRVzFTqp8B YdmzBTLXJHmw== X-IronPort-AV: E=McAfee;i="6200,9189,9986"; a="264383652" X-IronPort-AV: E=Sophos;i="5.82,307,1613462400"; d="scan'208";a="264383652" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 May 2021 06:49:51 -0700 IronPort-SDR: qPrqaoZIsX7KFEzvrWT847vxuU/vQuXuNBSGdJJhpOobzAlB1E5ZRLycDimZlVlmDAekIzThdb mahwX5TRbyRg== X-IronPort-AV: E=Sophos;i="5.82,307,1613462400"; d="scan'208";a="393523740" Received: from smile.fi.intel.com (HELO smile) ([10.237.68.40]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 May 2021 06:49:48 -0700 Received: from andy by smile with local (Exim 4.94) (envelope-from ) id 1lidcw-00CkNa-2G; Mon, 17 May 2021 16:49:46 +0300 Date: Mon, 17 May 2021 16:49:46 +0300 From: Andy Shevchenko To: Dan Carpenter Cc: Colin King , Shubhrajyoti Datta , Srinivas Neeli , Michal Simek , Linus Walleij , Bartosz Golaszewski , "open list:GPIO SUBSYSTEM" , linux-arm Mailing List , kernel-janitors , Linux Kernel Mailing List Subject: Re: [PATCH][next] gpio: xilinx: Fix potential integer overflow on shift of a u32 int Message-ID: References: <20210513085227.54392-1-colin.king@canonical.com> <20210514053754.GZ1955@kadam> <20210517133643.GI1955@kadam> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210517133643.GI1955@kadam> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210517_064952_890716_6EEF36D6 X-CRM114-Status: GOOD ( 29.71 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, May 17, 2021 at 04:36:43PM +0300, Dan Carpenter wrote: > On Mon, May 17, 2021 at 10:07:20AM +0300, Andy Shevchenko wrote: > > On Fri, May 14, 2021 at 12:26 PM Dan Carpenter wrote: > > > On Thu, May 13, 2021 at 09:52:27AM +0100, Colin King wrote: > > > > ... > > > > > > const unsigned long offset = (bit % BITS_PER_LONG) & BIT(5); > > > > > > > > map[index] &= ~(0xFFFFFFFFul << offset); > > > > - map[index] |= v << offset; > > > > + map[index] |= (unsigned long)v << offset; > > > > > > Doing a shift by BIT(5) is super weird. > > > > Not the first place in the kernel with such a trick. > > > > > It looks like a double shift > > > bug and should probably trigger a static checker warning. It's like > > > when people do BIT(BIT(5)). > > > > > > It would be more readable to write it as: > > > > > > int shift = (bit % BITS_PER_LONG) ? 32 : 0; > > > > Usually this code is in a kinda fast path. Have you checked if the > > compiler generates the same or better code when you are using ternary? > > I wrote a little benchmark to see which was faster and they're the same > as far as I can see. Thanks for checking. Besides the fact that offset should be 0 for 32-bit always and if compiler can proof that... The test below doesn't take into account the exact trick is used for offset (i.e. implicit dependency between BITS_PER_LONG, size of unsigned long, and using 5th bit out of value). I don't know if compiler can properly optimize the ternary in this case (but it looks like it should generate the same code). That said, I would rather to see the diff between assembly of the exact function before and after your proposal. > static inline __attribute__((__gnu_inline__)) unsigned long xgpio_set_value_orig(unsigned long *map, int bit, u32 v) > { > int shift = (bit % 64) & ((((1UL))) << (5)); > return v << shift; > } > > static inline __attribute__((__gnu_inline__)) unsigned long xgpio_set_value_new(unsigned long *map, int bit, u32 v) > { > int shift = (bit % 64) ? 32 : 0; > return v << shift; > } > > int main(void) > { > int i; > > for (i = 0; i < INT_MAX; i++) > xgpio_set_value_orig(NULL, i, 0); > > // for (i = 0; i < INT_MAX; i++) > // xgpio_set_value_new(NULL, i, 0); > > return 0; > } > -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel