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. > > 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
