On Mon, Apr 20, 2015 at 11:49:04AM -0700, H Hartley Sweeten wrote: > 'b_chans' may be a valud up to 32. 'b_mask' is an unsigned int and a left > shift of > more than 31 bits has undefined behavior. Fix the calc so it works correctly > with > a 'b_chans' of 32.. > > Reported-by: coverity (CID 1192244) > Signed-off-by: H Hartley Sweeten <hswee...@visionengravers.com> > Cc: Ian Abbott <abbo...@mev.co.uk> > Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org> > --- > drivers/staging/comedi/drivers/comedi_bond.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/comedi/drivers/comedi_bond.c > b/drivers/staging/comedi/drivers/comedi_bond.c > index 96db0c2..50b76ec 100644 > --- a/drivers/staging/comedi/drivers/comedi_bond.c > +++ b/drivers/staging/comedi/drivers/comedi_bond.c > @@ -101,7 +101,8 @@ static int bonding_dio_insn_bits(struct comedi_device > *dev, > b_chans = bdev->nchans - base_chan; > if (b_chans > n_left) > b_chans = n_left; > - b_mask = (1U << b_chans) - 1; > + b_mask = (b_chans < 32) ? ((1 << b_chans) - 1) > + : 0xffffffff;
Is that really an improvement? The original code was actually defined. 1U << 32 is actually defined. It is zero. Which works for us. 1 << 32 is undefined. It could be anything. 1 << 31 is INT_MIN. I think INT_MIN - 1 might be undefined. But I'm not sure. Although, in my testing "INT_MIN - 1" and "(uint)INT_MIN - 1" are the same which is what we intended so maybe the new code is fine. regards, dan carpenter > b_write_mask = (write_mask >> n_done) & b_mask; > b_data_bits = (data_bits >> n_done) & b_mask; > /* Read/Write the new digital lines. */ > -- > 2.3.0 > > _______________________________________________ > devel mailing list > de...@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel _______________________________________________ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
