From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753145AbbIQFSe (ORCPT ); Thu, 17 Sep 2015 01:18:34 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:45040 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751709AbbIQFSd (ORCPT ); Thu, 17 Sep 2015 01:18:33 -0400 Date: Wed, 16 Sep 2015 22:18:29 -0700 From: Greg Kroah-Hartman To: =?iso-8859-1?Q?Rapha=EBl?= Beamonte Cc: Cristina Opriceana , devel@driverdev.osuosl.org, lkml , Dan Carpenter Subject: Re: [PATCHv2 03/16] staging: rtl8192u: r8192U_core: add temporary variables to keep lines under 80 characters Message-ID: <20150917051829.GA12481@kroah.com> References: <20150917045702.GB29493@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 17, 2015 at 01:06:33AM -0400, Raphaël Beamonte wrote: > 2015-09-17 0:57 GMT-04:00 Greg Kroah-Hartman : > > >> @@ -1748,8 +1755,9 @@ static short rtl8192_usb_initendpoints(struct net_device *dev) > >> oldaddr = priv->oldaddr; > >> align = ((long)oldaddr) & 3; > >> if (align) { > >> - newaddr = oldaddr + 4 - align; > >> - priv->rx_urb[16]->transfer_buffer_length = 16 - 4 + align; > >> + align = 4 - align; > >> + newaddr = oldaddr + align; > >> + priv->rx_urb[16]->transfer_buffer_length = 16 - align; > > > > At a quick glance, this conversion looks wrong... > > What is wrong with it? > > oldaddr + 4 - align; > can also be read: > oldaddr + (4 - align); > > as well as > 16 - 4 + align; > can also be read > 16 - (4 - align); > as when we remove the parenthesis, the - sign invert the parenthesis > elements signs. > > Calculating (4 - align) previously thus preserve the logic here! Ugh, it's been a long day, yeah, ok, this is the same, you are right. But step back please, what exactly is this trying to do? I think it's a round_up type function, perhaps that should be what we do instead (the kernel has such functions already.) > > And it's not what your changelog text said you were doing :( > > It's true that I didn't add a new temporary variable here but instead > re-used one that is not used after, but I thought it went in the same > idea as the rest of this patch. Should I separate that as another > patch? It might be the same "idea", but it's not what you did, so don't try to sneak it in. greg k-h