On Sun, Aug 29, 2021 at 7:18 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > On Mon, 23 Aug 2021 14:09:21 +0300 > Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > > On Mon, Aug 23, 2021 at 1:20 PM Jacopo Mondi <jacopo@xxxxxxxxxx> 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. Thanks, I forgot to think about this case. Maybe being half asleep is not a bad thing after all? :-) > > > > 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]); -- With Best Regards, Andy Shevchenko
