Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934417AbdCVMNB (ORCPT ); Wed, 22 Mar 2017 08:13:01 -0400 Received: from mga01.intel.com ([192.55.52.88]:64098 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934181AbdCVMMw (ORCPT ); Wed, 22 Mar 2017 08:12:52 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,205,1486454400"; d="scan'208";a="78158819" From: "Dilger, Andreas" To: Arushi Singhal CC: "Drokin, Oleg" , James Simmons , Greg Kroah-Hartman , "devel@driverdev.osuosl.org" , LKML , "outreachy-kernel@googlegroups.com" , Luuk Paulussen , Hamish Martin Subject: Re: [PATCH] staging: media: Replace a bit shift by a use of BIT. Thread-Topic: [PATCH] staging: media: Replace a bit shift by a use of BIT. Thread-Index: AQHSosOpSvSwBefRz0W5a8/MmGva46Gg5JKA Date: Wed, 22 Mar 2017 12:11:59 +0000 Message-ID: <900FF751-5BFD-4B17-855C-6B1C0665B712@intel.com> References: <20170322042028.GA22848@arushi-HP-Pavilion-Notebook> In-Reply-To: <20170322042028.GA22848@arushi-HP-Pavilion-Notebook> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.254.54.130] Content-Type: text/plain; charset="us-ascii" Content-ID: MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v2MCD9ci027504 Content-Length: 2730 Lines: 77 On Mar 22, 2017, at 00:20, Arushi Singhal wrote: > > This patch replaces bit shifting on 1 with the BIT(x) macro. > This was done with coccinelle: > @@ > constant c; > @@ > > -1 << c > +BIT(c) Hi Arushi, thanks for taking time to contribute to the kernel. There are a few problems with this patch. Firstly, I'm not sure why you sent this patch to lustre-devel, Oleg, James, and me, since it has nothing to do with Lustre. Conversely, the actual maintainer(s) of this code would normally be better suited to review the patch. This isn't totally clear in this case since get_maintainer.pl lists another person submitting minor cleanups, but "git log" does return some more likely candidates: - David Daney (original author) - Luuk Paulussen (recent non-trivial patcher) - Kyeong Yoo (recent non-trivial reviewer) - Richard Laing " " - Chris Packham - Tim Beale - Hamish Martin Secondly, as mentioned with the other patch you submitted, you need to stop and look at whether your changes actually make sense. In this case, I don't think they do make sense, and this patch should probably just be abandoned. > Signed-off-by: Arushi Singhal > --- > drivers/staging/octeon/ethernet-tx.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/octeon/ethernet-tx.c b/drivers/staging/octeon/ethernet-tx.c > index ff4119e8de42..f00186ac4e27 100644 > --- a/drivers/staging/octeon/ethernet-tx.c > +++ b/drivers/staging/octeon/ethernet-tx.c > @@ -381,7 +381,7 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev) > (ip_hdr(skb)->version == 4) && > (ip_hdr(skb)->ihl == 5) && > ((ip_hdr(skb)->frag_off == 0) || > - (ip_hdr(skb)->frag_off == htons(1 << 14))) && > + (ip_hdr(skb)->frag_off == htons(BIT(14)))) && In this case, it is checking if the fragment offset in bytes is 16384, rather than doing a bit comparison, so I don't think the use of BIT() is appropriate here. > ((ip_hdr(skb)->protocol == IPPROTO_TCP) || > (ip_hdr(skb)->protocol == IPPROTO_UDP))) { > /* Use hardware checksum calc */ > @@ -613,7 +613,7 @@ int cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev) > #endif > work->word2.s.is_frag = !((ip_hdr(skb)->frag_off == 0) || > (ip_hdr(skb)->frag_off == > - 1 << 14)); > + BIT(14))); Same. Cheers, Andreas -- Andreas Dilger Lustre Principal Architect Intel Corporation