[PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.

[PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.

[Christophe JAILLET]

The 2nd parameter of 'find_first_zero_bit' is the number of bits to search.
In this case, we are passing 'sizeof(vt8500_ports_in_use)'.
'vt8500_ports_in_use' is an 'unsigned long'. So the sizeof is likely to
return 4.

A few lines below, we check if it is below VT8500_MAX_PORTS, which is 6.

It is likely that the number of bits in a long was expected here, so use
BITS_PER_LONG instead.


It has been spotted by the following coccinelle script:
@@
expression ret, x;

@@
*  ret = \(find_first_bit \| find_first_zero_bit\) (x, sizeof(...));

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Other options are possible:
  - 'vt8500_ports_in_use' being a 'unsigned long', use ffz to reduce
    code verbosity
  - VT8500_MAX_PORTS, in order to be consistent with the test below
---
 drivers/tty/serial/vt8500_serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/vt8500_serial.c b/drivers/tty/serial/vt8500_serial.c
index 23cfc5e16b45..935076c50cb1 100644
--- a/drivers/tty/serial/vt8500_serial.c
+++ b/drivers/tty/serial/vt8500_serial.c
@@ -664,7 +664,7 @@ static int vt8500_serial_probe(struct platform_device *pdev)
 	if (port < 0) {
 		/* calculate the port id */
 		port = find_first_zero_bit(&vt8500_ports_in_use,
-					sizeof(vt8500_ports_in_use));
+					   BITS_PER_LONG);
 	}
 
 	if (port >= VT8500_MAX_PORTS)
-- 
2.7.4


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

[PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.

[Arnd Bergmann]

On Sunday, August 21, 2016 11:20:25 PM CEST Christophe JAILLET wrote:
> The 2nd parameter of 'find_first_zero_bit' is the number of bits to search.
> In this case, we are passing 'sizeof(vt8500_ports_in_use)'.
> 'vt8500_ports_in_use' is an 'unsigned long'. So the sizeof is likely to
> return 4.
> 
> A few lines below, we check if it is below VT8500_MAX_PORTS, which is 6.
> 
> It is likely that the number of bits in a long was expected here, so use
> BITS_PER_LONG instead.
> 
> 
> It has been spotted by the following coccinelle script:
> @@
> expression ret, x;
> 
> @@
> *  ret = \(find_first_bit \| find_first_zero_bit\) (x, sizeof(...));
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Other options are possible:
>   - 'vt8500_ports_in_use' being a 'unsigned long', use ffz to reduce
>     code verbosity
>   - VT8500_MAX_PORTS, in order to be consistent with the test below

Sorry, but I'm not following the logic here.

> ---
>  drivers/tty/serial/vt8500_serial.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/vt8500_serial.c b/drivers/tty/serial/vt8500_serial.c
> index 23cfc5e16b45..935076c50cb1 100644
> --- a/drivers/tty/serial/vt8500_serial.c
> +++ b/drivers/tty/serial/vt8500_serial.c
> @@ -664,7 +664,7 @@ static int vt8500_serial_probe(struct platform_device *pdev)
>  	if (port < 0) {
>  		/* calculate the port id */
>  		port = find_first_zero_bit(&vt8500_ports_in_use,
> -					sizeof(vt8500_ports_in_use));
> +					   BITS_PER_LONG);
>  	}

You argue that the two have the same meaning, which I see, but
why is it better than the existing code?

	Arnd

[PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.

[Christophe JAILLET]

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.

In no way, 5 or above can be returned.

The code just after is:
	if (port >= VT8500_MAX_PORTS)
		return -ENODEV;
It turns into:
	if (port >= 6)
		return -ENODEV;


I see 2 problems there:
	- First, according to this test, "port = 5" seems a legal value, but can never trigger.
	- Second, if the first 3 bits are set, the find_first_zero_bit will return 4, whatever the value of the 4th bit.
           This 4 can either mean "4th bit is clear" or "no clear bit found, so return @size (i.e. 4)"

  
Using:
	port = find_first_zero_bit(&vt8500_ports_in_use, BITS_PER_LONG);
Would solve the 2 issues.
    - 4 would really mean, 4th bit is set.
    - 5 becomes a possible value.
    - 6 to 31 would mean: we found a clear bit "in the garbage after the VT8500_MAX_PORTS (i.e. 6) relevant bits".
    - 32 would mean, all bits set.

These answers look more in line with the "if (port >= VT8500_MAX_PORTS)" test.



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 :) )

Best regard,
CJ


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

[PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.

[Arnd Bergmann]

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);

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:

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

[PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.

[Christophe JAILLET]

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