* [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-21 21:20 ` Christophe JAILLET
0 siblings, 0 replies; 18+ messages in thread
From: Christophe JAILLET @ 2016-08-21 21:20 UTC (permalink / raw)
To: linux, gregkh, jslaby
Cc: linux-arm-kernel, linux-serial, linux-kernel, kernel-janitors,
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-21 21:20 ` Christophe JAILLET
0 siblings, 0 replies; 18+ messages in thread
From: Christophe JAILLET @ 2016-08-21 21:20 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-21 21:20 ` Christophe JAILLET
0 siblings, 0 replies; 18+ messages in thread
From: Christophe JAILLET @ 2016-08-21 21:20 UTC (permalink / raw)
To: linux, gregkh, jslaby
Cc: Christophe JAILLET, kernel-janitors, linux-serial,
linux-arm-kernel, linux-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-21 21:20 ` Christophe JAILLET
0 siblings, 0 replies; 18+ messages in thread
From: Christophe JAILLET @ 2016-08-21 21:20 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
2016-08-21 21:20 ` Christophe JAILLET
(?)
(?)
@ 2016-08-22 8:42 ` Arnd Bergmann
-1 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2016-08-22 8:42 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Christophe JAILLET, linux, gregkh, jslaby, kernel-janitors,
linux-serial, linux-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-22 8:42 ` Arnd Bergmann
0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2016-08-22 8:42 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-22 8:42 ` Arnd Bergmann
0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2016-08-22 8:42 UTC (permalink / raw)
To: linux-arm-kernel
Cc: gregkh, kernel-janitors, linux-kernel, linux, Christophe JAILLET,
linux-serial, jslaby
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-22 8:42 ` Arnd Bergmann
0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2016-08-22 8:42 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
2016-08-22 8:42 ` Arnd Bergmann
(?)
@ 2016-08-23 4:20 ` Christophe JAILLET
-1 siblings, 0 replies; 18+ messages in thread
From: Christophe JAILLET @ 2016-08-23 4:20 UTC (permalink / raw)
To: Arnd Bergmann, linux-arm-kernel
Cc: gregkh, kernel-janitors, linux-kernel, linux, linux-serial, jslaby
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-23 4:20 ` Christophe JAILLET
0 siblings, 0 replies; 18+ messages in thread
From: Christophe JAILLET @ 2016-08-23 4:20 UTC (permalink / raw)
To: linux-arm-kernel
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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-23 4:20 ` Christophe JAILLET
0 siblings, 0 replies; 18+ messages in thread
From: Christophe JAILLET @ 2016-08-23 4:20 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
2016-08-23 4:20 ` Christophe JAILLET
(?)
@ 2016-08-23 9:23 ` Arnd Bergmann
-1 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2016-08-23 9:23 UTC (permalink / raw)
To: Christophe JAILLET
Cc: linux-arm-kernel, gregkh, kernel-janitors, linux-kernel, linux,
linux-serial, jslaby
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-23 9:23 ` Arnd Bergmann
0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2016-08-23 9:23 UTC (permalink / raw)
To: linux-arm-kernel
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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-23 9:23 ` Arnd Bergmann
0 siblings, 0 replies; 18+ messages in thread
From: Arnd Bergmann @ 2016-08-23 9:23 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
2016-08-23 9:23 ` Arnd Bergmann
(?)
(?)
@ 2016-08-23 20:24 ` Christophe JAILLET
-1 siblings, 0 replies; 18+ messages in thread
From: Christophe JAILLET @ 2016-08-23 20:24 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-arm-kernel, gregkh, kernel-janitors, linux-kernel, linux,
linux-serial, jslaby
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-23 20:24 ` Christophe JAILLET
0 siblings, 0 replies; 18+ messages in thread
From: Christophe JAILLET @ 2016-08-23 20:24 UTC (permalink / raw)
To: linux-arm-kernel
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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-23 20:24 ` Christophe JAILLET
0 siblings, 0 replies; 18+ messages in thread
From: Christophe JAILLET @ 2016-08-23 20:24 UTC (permalink / raw)
To: kernel-janitors; +Cc: linux-arm-kernel, linux-kernel, linux-serial
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit.
@ 2016-08-23 20:24 ` Christophe JAILLET
0 siblings, 0 replies; 18+ messages in thread
From: Christophe JAILLET @ 2016-08-23 20:24 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-08-23 20:24 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-21 21:20 [PATCH] serial: vt8500_serial: Fix a parameter of find_first_zero_bit Christophe JAILLET
2016-08-21 21:20 ` Christophe JAILLET
2016-08-21 21:20 ` Christophe JAILLET
2016-08-21 21:20 ` Christophe JAILLET
2016-08-22 8:42 ` Arnd Bergmann
2016-08-22 8:42 ` Arnd Bergmann
2016-08-22 8:42 ` Arnd Bergmann
2016-08-22 8:42 ` Arnd Bergmann
2016-08-23 4:20 ` Christophe JAILLET
2016-08-23 4:20 ` Christophe JAILLET
2016-08-23 4:20 ` Christophe JAILLET
2016-08-23 9:23 ` Arnd Bergmann
2016-08-23 9:23 ` Arnd Bergmann
2016-08-23 9:23 ` Arnd Bergmann
2016-08-23 20:24 ` Christophe JAILLET
2016-08-23 20:24 ` Christophe JAILLET
2016-08-23 20:24 ` Christophe JAILLET
2016-08-23 20:24 ` Christophe JAILLET
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.