Re: [PATCH v3.1 2/3] iio: chemical: Add Senseair Sunrise 006-0-007 driver

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 23 Aug 2021 14:09:21 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Aug 23, 2021 at 1:20 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > On Mon, Aug 23, 2021 at 12:40:00PM +0300, Andy Shevchenko wrote:  
> > > On Mon, Aug 23, 2021 at 11:06:10AM +0200, Jacopo Mondi wrote:  
> > > > On Mon, Aug 23, 2021 at 11:35:23AM +0300, Andy Shevchenko wrote:  
> > > > > On Mon, Aug 23, 2021 at 09:36:39AM +0200, Jacopo Mondi wrote:  
> 
> ...
> 
> > > > > > +       errors = (const unsigned long *)&value;  
> > > > >
> > > > > Yes, it takes a pointer, but in your case it will oops the kernel quite likely.  
> > > >
> > > > The usage of a pointer kind of puzzled me in first place, and I found
> > > > no pattern to copy in the existing code base. I have probbaly not
> > > > looked hard enough ?
> > > >  
> > > > >   unsigned long errors;
> > > > >
> > > > >   ...
> > > > >
> > > > >   errors = value;
> > > > >   for_each_set_bit(..., &errors, ...)  
> > > >
> > > > I can do so but, for my education mostly, why do you think it would
> > > > oops ? '*errors' is a pointer to a variable allocated on the stack as
> > > > much as 'errors' you suggested is a local stack variable. I might be a
> > > > bit slow today, but I'm missing the difference. FWIW I tested the
> > > > function, even if there were no error bits set at the moment I tested, but
> > > > the for_each_set_bit() macro was indeed run.  
> > >
> > > Because you theoretically access bytes behind 16-bit. That castings just ugly
> > > and compiler should warn you about if it is clever enough.  
> >
> > I don't think there's such a risk as I limit the search space to
> > ARRAY_SIZE(error_codes), but I agree the cast is ugly and I'll change
> > to what you suggested  
> 
> More for the sake of education. Actually it's not theoretical. Some
> architectures may not access longs on unaligned (16-bit) addresses.
> So, yes, oops is probable.
> Another example is reading the long to the register. This can cross
> the page boundary and produce fault (again) oops.

For extra giggles, (and I'm half asleep today so may have this backwards)
what it the platform is big endian and you do this?  I'm fairly sure it will
walk the bits from low to high and so the first access will be off the end
of your shorter type being as it will be at addr + sizeof (long) - 1 bit 7.

> 
> > > If you found it in the existing code, please, fix it there, so quite bad
> > > pattern won't spread.  
> >
> > My point was that I was not able to find any pattern to copy from :)
> >  
> > > > > > +       for_each_set_bit(i, errors, ARRAY_SIZE(error_codes))
> > > > > > +               len += sysfs_emit_at(buf, len, "%s ", sunrise_error_statuses[i]);  
>