[PATCH 09/11] fsmc/nand:FIX: replace change_bit routine
Vipin Kumar
vipin.kumar at st.com
Thu Oct 11 00:17:03 EDT 2012
On 10/11/2012 1:51 AM, Nicolas Pitre wrote:
> On Wed, 10 Oct 2012, Linus Walleij wrote:
>
>> On Tue, Oct 9, 2012 at 12:44 PM, Vipin Kumar<vipin.kumar at st.com> wrote:
>>
>>> change_bit routine accepts only ulong pointers as buffer, so an unaligned char
>>> pointer passed to change_bit may lead to a crash.
>>>
>>> Fix this bug by accessing the buffer as char pointer.
>>
>> Why not see if we can fix change_bit() instead?
>> Since I suspect this is on ARM I bet Russell and Nico
>> want to hear about this if there is a problem.
>>
>> Can the ARM change_bit() not be fixed, so that
>> long arguments are the only option?
>
Hello Nicolas
> No. It is this code which is totally broken.
>
Yes, I understand and accept the probelm. That's why the fix
> The change_bit() is defined to operate on long values, in the _native_
> endian. Now imagine what this is going to do to your data buffer if
> instead you are running on a big endian device.
>
> And I doubt there is anything requiring atomic bit manipulation here
> either.
>
No, there is no requirement for an atomic change_bit
>>> if (err_idx[i]< chip->ecc.size * 8) {
>>> - change_bit(err_idx[i], (unsigned long *)dat);
>>> + uint8_t *p = dat + err_idx[i] / 8;
>>> + *p = *p ^ (1<< (err_idx[i] % 8));
>>
>> I'm one of these people who would write>>3 and
>> &7 rather than /8 or %8 but I guess we are all
>> different. Atleast consider it if you stick with this...
>
> Better yet:
>
> dat[err_idx[i] / 8] ^= (1<< (err_idx[i] % 8));
>
> The /8 and %8 will be changed into>>3 and&7 by the compiler anyway,
> while the /8 and %8 form is possibly a bit less obscure.
>
Yes, that's exactly why I kept /8 and %8. I would change the code as
suggested by you
> Of course, this needs to be done over _all_ this driver, not only a few
> cases.
>
There is only one place which needs a change_bit. I would send a v2 with
the suggested change
Vipin
>
> Nicolas
> .
>
More information about the linux-arm-kernel
mailing list