Messages in this thread | | | Date | Wed, 16 Sep 2015 22:18:29 -0700 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCHv2 03/16] staging: rtl8192u: r8192U_core: add temporary variables to keep lines under 80 characters |
| |
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 <gregkh@linuxfoundation.org>: > <SNIP> > >> @@ -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
| |