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.5 required=3.0 tests=DKIM_ADSP_CUSTOM_MED, FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 53B80C433E0 for ; Mon, 8 Jun 2020 13:00:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 3A652206C3 for ; Mon, 8 Jun 2020 13:00:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729543AbgFHNAL (ORCPT ); Mon, 8 Jun 2020 09:00:11 -0400 Received: from mga07.intel.com ([134.134.136.100]:24601 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729027AbgFHNAK (ORCPT ); Mon, 8 Jun 2020 09:00:10 -0400 IronPort-SDR: L9yHiPSY6L9003JD8yJ4tCbt4+dStbQHDGniGEIJvvuNSXqRBjVBXc6PZHD4NyM5f+9y93iRNg HQYVDNftdOCA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Jun 2020 06:00:08 -0700 IronPort-SDR: PE7HufXDg7f9+jqjyOSDQxo3+G5SfFBHCYIwahilTfZJSDu5i1xv5XRhvFNk0pOmcejo6UVxmT YgYUB1xC1Ltw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,487,1583222400"; d="scan'208";a="274212404" Received: from smile.fi.intel.com (HELO smile) ([10.237.68.40]) by orsmga006.jf.intel.com with ESMTP; 08 Jun 2020 06:00:04 -0700 Received: from andy by smile with local (Exim 4.93) (envelope-from ) id 1jiHNm-00BgOT-Bj; Mon, 08 Jun 2020 16:00:06 +0300 Date: Mon, 8 Jun 2020 16:00:06 +0300 From: Andy Shevchenko To: Alexander Gordeev Cc: Linux Kernel Mailing List , linux-s390@vger.kernel.org, Stable , Andrew Morton , Yury Norov , Amritha Nambiar , Arnaldo Carvalho de Melo , Chris Wilson , Kees Cook , Matthew Wilcox , Miklos Szeredi , Rasmus Villemoes , Steffen Klassert , "Tobin C . Harding" , Vineet Gupta , Will Deacon , Willem de Bruijn Subject: Re: [PATCH RESEND2] lib: fix bitmap_parse() on 64-bit big endian archs Message-ID: <20200608130006.GN2428291@smile.fi.intel.com> References: <1591611829-23071-1-git-send-email-agordeev@linux.ibm.com> <20200608124433.GA28369@oc3871087118.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200608124433.GA28369@oc3871087118.ibm.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jun 08, 2020 at 02:44:34PM +0200, Alexander Gordeev wrote: > On Mon, Jun 08, 2020 at 03:03:05PM +0300, Andy Shevchenko wrote: > > On Mon, Jun 8, 2020 at 1:26 PM Alexander Gordeev wrote: > > > > > > Commit 2d6261583be0 ("lib: rework bitmap_parse()") does not > > > take into account order of halfwords on 64-bit big endian > > > architectures. As result (at least) Receive Packet Steering, > > > IRQ affinity masks and runtime kernel test "test_bitmap" get > > > broken on s390. > > > > ... > > > > > +#if defined(__BIG_ENDIAN) && defined(CONFIG_64BIT) > > > > I think it's better to re-use existing patterns. > > > > ipc/sem.c:1682:#if defined(CONFIG_64BIT) && defined(__BIG_ENDIAN) > > > > > +static void save_x32_chunk(unsigned long *maskp, u32 chunk, int chunk_idx) > > > +{ > > > + maskp += (chunk_idx / 2); > > > + ((u32 *)maskp)[(chunk_idx & 1) ^ 1] = chunk; > > > +} > > > +#else > > > +static void save_x32_chunk(unsigned long *maskp, u32 chunk, int chunk_idx) > > > +{ > > > + ((u32 *)maskp)[chunk_idx] = chunk; > > > +} > > > +#endif > > > > See below. > > > > ... > > > > > - end = bitmap_get_x32_reverse(start, end, bitmap++); > > > + end = bitmap_get_x32_reverse(start, end, &chunk); > > > if (IS_ERR(end)) > > > return PTR_ERR(end); > > > + > > > + save_x32_chunk(maskp, chunk, chunk_idx++); > > > > Can't we simple do > > > > int chunk_index = 0; > > ... > > do { > > #if defined(CONFIG_64BIT) && defined(__BIG_ENDIAN) > > end = bitmap_get_x32_reverse(start, end, > > bitmap[chunk_index ^ 1]); > > #else > > end = bitmap_get_x32_reverse(start, end, bitmap[chunk_index]); > > #endif > > ... > > } while (++chunk_index); > > > > ? > > Well, unless we ignore coding style 21) Conditional Compilation > we could. Do you still insist it would be better? I think it's okay to do here - it's not a big function - it has no stub versions (you always do something) - the result pretty much readable (5 lines any editor can keep on screen) - and it's not ignoring, see "Wherever possible...", compare readability of two versions, for yours reader needs to go somewhere to read, calculate and return, when everything already being forgotten - last but not least, I bet it makes code shorter (at least in C) -- With Best Regards, Andy Shevchenko