Le 23/08/2016 à 11:23, Arnd Bergmann a écrit :

On Tuesday, August 23, 2016 6:20:28 AM CEST Christophe JAILLET wrote:

quoted

Le 22/08/2016 à 10:42, Arnd Bergmann a écrit :

quoted

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

quoted

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

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