lkml.org 
[lkml]   [2016]   [Aug]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
From
Date
Le 23/08/2016 à 11:23, Arnd Bergmann a écrit :
> On Tuesday, August 23, 2016 6:20:28 AM CEST Christophe JAILLET wrote:
>> Le 22/08/2016 à 10:42, Arnd Bergmann a écrit :
>>> [...]
>>> Sorry, but I'm not following the logic here.
>>>
>>> [...]
>>> You argue that the two have the same meaning, which I see, but
>>> why is it better than the existing code?
>>>
>>> Arnd
>> Hi,
>>
>> sorry if my explanation was unclear.
>>
>> What I mean is that if "sizeof(unsigned long) = 4" (i.e. 32 bits systems
>> ?) then:
>>
>> port = find_first_zero_bit(&vt8500_ports_in_use, sizeof(vt8500_ports_in_use));
>> turns into:
>> port = find_first_zero_bit(&vt8500_ports_in_use, 4);
>>
>> find_first_zero_bit "Returns the bit number of the first set bit. If no bits are set, returns @size."
>> So, in this case, it can return 1, 2, 3 or 4, if one of the 4 first bits is 0.
>> And will also return 4, if none of the 4 first bits is 0.
> Ah, got it.
>
>> Finally, what I meant by "Other options are possible:" is:
>> - 'vt8500_ports_in_use' being a 'unsigned long', use ffz to reduce code verbosity
>> port = ffz(&vt8500_ports_in_use);
>> would also work, because it is equivalent to:
>> port = find_first_zero_bit(&vt8500_ports_in_use, BITS_PER_LONG);
>>
>> - VT8500_MAX_PORTS, in order to be consistent with the test below
>> port = find_first_zero_bit(&vt8500_ports_in_use, VT8500_MAX_PORTS);
>> would also work and is maybe more logical in regard to the test "if (port >= VT8500_MAX_PORTS)"
>>
>>
>>
>> Now if "sizeof(unsigned long) = 8" (i.e. 64 bits systems ?), the actual code would work.
>> But using "sizeof(long)" to mean "more than VT8500_MAX_PORTS" is odd.
>> In other words, expressing a number of bits using something that gives a size in bytes is, IMHO, spurious.
>>
>> All this is pure speculation.
>>
>> Hoping that it is clearer now ( and that my analysis is right :) )
> I misread the code in the same way the original author wrote it wrong,
> I guess it was meant to say
>
> port = find_first_zero_bit(&vt8500_ports_in_use, sizeof(vt8500_ports_in_use) * 8);
I guess so.

> to convert number of bytes into number of bits. Your patch is absolutely
> correct, but being more specific about the kind of mistake that was made
> is a good idea.
>
> Regarding which of the four alternatives to use, I'd probably use
> your third one, checking against VT8500_MAX_PORTS. To make this code
> absolutely foolproof, we can add this hunk too then:
Agreed for VT8500_MAX_PORTS. This documents the code.

Using DECLARE_BITMAP is also nice (even if I doubt that it will be
useful one day in this particular case)
It would turn the vt8500_ports_in_use variable into a pointer. So some
more code modification would be required.

Thk for your feedback and comments.

I'll send a v2.


CJ


> diff --git a/drivers/tty/serial/vt8500_serial.c b/drivers/tty/serial/vt8500_serial.c
> index 23cfc5e16b45..a68be66d2770 100644
> --- a/drivers/tty/serial/vt8500_serial.c
> +++ b/drivers/tty/serial/vt8500_serial.c
> @@ -118,7 +118,7 @@ struct vt8500_port {
> * have been allocated as we can't use pdev->id in
> * devicetree
> */
> -static unsigned long vt8500_ports_in_use;
> +static DECLARE_BITMAP(vt8500_ports_in_use, VT8500_MAX_PORTS);
>
> static inline void vt8500_write(struct uart_port *port, unsigned int val,
> unsigned int off)
>
>
>
> Arnd


---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus

\
 
 \ /
  Last update: 2016-09-17 09:57    [W:0.048 / U:0.488 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site