| Message ID | 1471814425-18949-1-git-send-email-christophe.jaillet@wanadoo.fr |
|---|---|
| State | New |
| Headers | show |
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
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
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
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)
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(-)