Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1470542 > unrolled thread

[PATCH 1/2] IB/hfi1: Fix a parameter of find_first_bit.

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] IB/hfi1: Fix a parameter of find_first_bit. Christophe JAILLET <christophe.jaillet@wanadoo.fr> - 2016-08-26 07:00 +0200
    Re: [PATCH 1/2] IB/hfi1: Fix a parameter of find_first_bit. Doug Ledford <dledford@redhat.com> - 2016-08-26 15:40 +0200
      Re: [PATCH 1/2] IB/hfi1: Fix a parameter of find_first_bit. Doug Ledford <dledford@redhat.com> - 2016-08-26 20:10 +0200
        Re: [PATCH 1/2] IB/hfi1: Fix a parameter of find_first_bit. Doug Ledford <dledford@redhat.com> - 2016-08-26 21:40 +0200
          Re: [PATCH 1/2] IB/hfi1: Fix a parameter of find_first_bit. Leon Romanovsky <leon@kernel.org> - 2016-08-28 08:10 +0200
            Re: [PATCH 1/2] IB/hfi1: Fix a parameter of find_first_bit. Doug Ledford <dledford@redhat.com> - 2016-09-02 16:40 +0200
              Re: [PATCH 1/2] IB/hfi1: Fix a parameter of find_first_bit. Leon Romanovsky <leon@kernel.org> - 2016-09-04 11:10 +0200
        Re: [PATCH 1/2] IB/hfi1: Fix a parameter of find_first_bit. Leon Romanovsky <leon@kernel.org> - 2016-08-26 21:40 +0200
      Re: [PATCH 1/2] IB/hfi1: Fix a parameter of find_first_bit. Christophe JAILLET <christophe.jaillet@wanadoo.fr> - 2016-08-27 07:30 +0200
        Re: [PATCH 1/2] IB/hfi1: Fix a parameter of find_first_bit. Doug Ledford <dledford@redhat.com> - 2016-09-02 19:20 +0200

#1470542 — [PATCH 1/2] IB/hfi1: Fix a parameter of find_first_bit.

The 2nd parameter of 'find_first_bit' is the number of bits to search.
In this case, we are passing 'sizeof(unsigned long)' which is likely to
be 4 or 8.

It is likely that the number of bits of 'port_mask' was expected here. This
variable is a 'u64', so use 64 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>
---
Not sure that using 64 directly is the best option.
Maybe '8 * sizeof(port_mask)' as used in the same file for
'for_each_set_bit' would be better
---
 drivers/infiniband/hw/hfi1/mad.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/hw/hfi1/mad.c b/drivers/infiniband/hw/hfi1/mad.c
index 1263abe01999..2c6c138c41b2 100644
--- a/drivers/infiniband/hw/hfi1/mad.c
+++ b/drivers/infiniband/hw/hfi1/mad.c
@@ -2632,8 +2632,7 @@ static int pma_get_opa_datacounters(struct opa_pma_mad *pmp,
 	 * port the request came in on.
 	 */
 	port_mask = be64_to_cpu(req->port_select_mask[3]);
-	port_num = find_first_bit((unsigned long *)&port_mask,
-				  sizeof(port_mask));
+	port_num = find_first_bit((unsigned long *)&port_mask, 64);
 
 	if ((u8)port_num != port) {
 		pmp->mad_hdr.status |= IB_SMP_INVALID_FIELD;
@@ -2836,8 +2835,7 @@ static int pma_get_opa_porterrors(struct opa_pma_mad *pmp,
 	 * port the request came in on.
 	 */
 	port_mask = be64_to_cpu(req->port_select_mask[3]);
-	port_num = find_first_bit((unsigned long *)&port_mask,
-				  sizeof(port_mask));
+	port_num = find_first_bit((unsigned long *)&port_mask, 64);
 
 	if (port_num != port) {
 		pmp->mad_hdr.status |= IB_SMP_INVALID_FIELD;
@@ -3009,8 +3007,7 @@ static int pma_get_opa_errorinfo(struct opa_pma_mad *pmp,
 	 * the request came in on.
 	 */
 	port_mask = be64_to_cpu(req->port_select_mask[3]);
-	port_num = find_first_bit((unsigned long *)&port_mask,
-				  sizeof(port_mask));
+	port_num = find_first_bit((unsigned long *)&port_mask, 64);
 
 	if (port_num != port) {
 		pmp->mad_hdr.status |= IB_SMP_INVALID_FIELD;
@@ -3246,8 +3243,7 @@ static int pma_set_opa_errorinfo(struct opa_pma_mad *pmp,
 	 * the request came in on.
 	 */
 	port_mask = be64_to_cpu(req->port_select_mask[3]);
-	port_num = find_first_bit((unsigned long *)&port_mask,
-				  sizeof(port_mask));
+	port_num = find_first_bit((unsigned long *)&port_mask, 64);
 
 	if (port_num != port) {
 		pmp->mad_hdr.status |= IB_SMP_INVALID_FIELD;
-- 
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

[toc] | [next] | [standalone]

#1470728

[Multipart message — attachments visible in raw view] — view raw

On 8/26/2016 12:49 AM, Christophe JAILLET wrote:
> The 2nd parameter of 'find_first_bit' is the number of bits to search.
> In this case, we are passing 'sizeof(unsigned long)' which is likely to
> be 4 or 8.

If the size can be 4 or 8, then using 64 universally is not correct.
Why not use sizeof() * 8 (or << 3)?

> It is likely that the number of bits of 'port_mask' was expected here. This
> variable is a 'u64', so use 64 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>
> ---
> Not sure that using 64 directly is the best option.
> Maybe '8 * sizeof(port_mask)' as used in the same file for
> 'for_each_set_bit' would be better
> ---
>  drivers/infiniband/hw/hfi1/mad.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/hfi1/mad.c b/drivers/infiniband/hw/hfi1/mad.c
> index 1263abe01999..2c6c138c41b2 100644
> --- a/drivers/infiniband/hw/hfi1/mad.c
> +++ b/drivers/infiniband/hw/hfi1/mad.c
> @@ -2632,8 +2632,7 @@ static int pma_get_opa_datacounters(struct opa_pma_mad *pmp,
>  	 * port the request came in on.
>  	 */
>  	port_mask = be64_to_cpu(req->port_select_mask[3]);
> -	port_num = find_first_bit((unsigned long *)&port_mask,
> -				  sizeof(port_mask));
> +	port_num = find_first_bit((unsigned long *)&port_mask, 64);
>  
>  	if ((u8)port_num != port) {
>  		pmp->mad_hdr.status |= IB_SMP_INVALID_FIELD;
> @@ -2836,8 +2835,7 @@ static int pma_get_opa_porterrors(struct opa_pma_mad *pmp,
>  	 * port the request came in on.
>  	 */
>  	port_mask = be64_to_cpu(req->port_select_mask[3]);
> -	port_num = find_first_bit((unsigned long *)&port_mask,
> -				  sizeof(port_mask));
> +	port_num = find_first_bit((unsigned long *)&port_mask, 64);
>  
>  	if (port_num != port) {
>  		pmp->mad_hdr.status |= IB_SMP_INVALID_FIELD;
> @@ -3009,8 +3007,7 @@ static int pma_get_opa_errorinfo(struct opa_pma_mad *pmp,
>  	 * the request came in on.
>  	 */
>  	port_mask = be64_to_cpu(req->port_select_mask[3]);
> -	port_num = find_first_bit((unsigned long *)&port_mask,
> -				  sizeof(port_mask));
> +	port_num = find_first_bit((unsigned long *)&port_mask, 64);
>  
>  	if (port_num != port) {
>  		pmp->mad_hdr.status |= IB_SMP_INVALID_FIELD;
> @@ -3246,8 +3243,7 @@ static int pma_set_opa_errorinfo(struct opa_pma_mad *pmp,
>  	 * the request came in on.
>  	 */
>  	port_mask = be64_to_cpu(req->port_select_mask[3]);
> -	port_num = find_first_bit((unsigned long *)&port_mask,
> -				  sizeof(port_mask));
> +	port_num = find_first_bit((unsigned long *)&port_mask, 64);
>  
>  	if (port_num != port) {
>  		pmp->mad_hdr.status |= IB_SMP_INVALID_FIELD;
> 


-- 
Doug Ledford <dledford@redhat.com>
    GPG Key ID: 0E572FDD

[toc] | [prev] | [next] | [standalone]

#1470920

[Multipart message — attachments visible in raw view] — view raw

On 8/26/2016 9:35 AM, Doug Ledford wrote:
> On 8/26/2016 12:49 AM, Christophe JAILLET wrote:
>> The 2nd parameter of 'find_first_bit' is the number of bits to search.
>> In this case, we are passing 'sizeof(unsigned long)' which is likely to
>> be 4 or 8.
> 
> If the size can be 4 or 8, then using 64 universally is not correct.
> Why not use sizeof() * 8 (or << 3)?

Better yet, why not put this patch in the kernel first:

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index d96a6118d26a..a8838c87668e 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -52,6 +52,8 @@

 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))

+#define bitsizeof(x)   (sizeof((x)) << 3)
+
 #define u64_to_user_ptr(x) (           \
 {                                      \
        typecheck(u64, x);              \

then start going around replacing all these hard coded numbers with the
use of bitsizeof().  It can be applied not just to the find_first*bit()
routines, but to a bunch of other routines too.  Just look at
include/linux/bitmap.h and any that have nbits as an argument are
candidates.


-- 
Doug Ledford <dledford@redhat.com>
    GPG Key ID: 0E572FDD

[toc] | [prev] | [next] | [standalone]

#1470958

[Multipart message — attachments visible in raw view] — view raw

On 8/26/2016 3:29 PM, Leon Romanovsky wrote:
> On Fri, Aug 26, 2016 at 02:01:55PM -0400, Doug Ledford wrote:
>> On 8/26/2016 9:35 AM, Doug Ledford wrote:
>>> On 8/26/2016 12:49 AM, Christophe JAILLET wrote:
>>>> The 2nd parameter of 'find_first_bit' is the number of bits to search.
>>>> In this case, we are passing 'sizeof(unsigned long)' which is likely to
>>>> be 4 or 8.
>>>
>>> If the size can be 4 or 8, then using 64 universally is not correct.
>>> Why not use sizeof() * 8 (or << 3)?
>>
>> Better yet, why not put this patch in the kernel first:
>>
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index d96a6118d26a..a8838c87668e 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -52,6 +52,8 @@
>>
>>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
>> __must_be_array(arr))
>>
>> +#define bitsizeof(x)   (sizeof((x)) << 3)
>> +
>>  #define u64_to_user_ptr(x) (           \
>>  {                                      \
>>         typecheck(u64, x);              \
>>
>> then start going around replacing all these hard coded numbers with the
>> use of bitsizeof().  It can be applied not just to the find_first*bit()
>> routines, but to a bunch of other routines too.  Just look at
>> include/linux/bitmap.h and any that have nbits as an argument are
>> candidates.
> 
> There is BITS_PER_LONG define for that. There is actual use of it in mlx5 for
> the similar code pieces.

BITS_PER_LONG only works if your bitfield is a single long.  It doesn't
work for other bitfields.  What I posted above will work for anything.


-- 
Doug Ledford <dledford@redhat.com>
    GPG Key ID: 0E572FDD

[toc] | [prev] | [next] | [standalone]

#1471277

[Multipart message — attachments visible in raw view] — view raw

On Fri, Aug 26, 2016 at 03:34:48PM -0400, Doug Ledford wrote:
> On 8/26/2016 3:29 PM, Leon Romanovsky wrote:
> > On Fri, Aug 26, 2016 at 02:01:55PM -0400, Doug Ledford wrote:
> >> On 8/26/2016 9:35 AM, Doug Ledford wrote:
> >>> On 8/26/2016 12:49 AM, Christophe JAILLET wrote:
> >>>> The 2nd parameter of 'find_first_bit' is the number of bits to search.
> >>>> In this case, we are passing 'sizeof(unsigned long)' which is likely to
> >>>> be 4 or 8.
> >>>
> >>> If the size can be 4 or 8, then using 64 universally is not correct.
> >>> Why not use sizeof() * 8 (or << 3)?
> >>
> >> Better yet, why not put this patch in the kernel first:
> >>
> >> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> >> index d96a6118d26a..a8838c87668e 100644
> >> --- a/include/linux/kernel.h
> >> +++ b/include/linux/kernel.h
> >> @@ -52,6 +52,8 @@
> >>
> >>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> >> __must_be_array(arr))
> >>
> >> +#define bitsizeof(x)   (sizeof((x)) << 3)
> >> +
> >>  #define u64_to_user_ptr(x) (           \
> >>  {                                      \
> >>         typecheck(u64, x);              \
> >>
> >> then start going around replacing all these hard coded numbers with the
> >> use of bitsizeof().  It can be applied not just to the find_first*bit()
> >> routines, but to a bunch of other routines too.  Just look at
> >> include/linux/bitmap.h and any that have nbits as an argument are
> >> candidates.
> >
> > There is BITS_PER_LONG define for that. There is actual use of it in mlx5 for
> > the similar code pieces.
>
> BITS_PER_LONG only works if your bitfield is a single long.  It doesn't
> work for other bitfields.  What I posted above will work for anything.

Yes, the question to ask if it is really needed.

>
>
> --
> Doug Ledford <dledford@redhat.com>
>     GPG Key ID: 0E572FDD
>

[toc] | [prev] | [next] | [standalone]

#1475206

[Multipart message — attachments visible in raw view] — view raw

On 8/28/2016 2:06 AM, Leon Romanovsky wrote:
> On Fri, Aug 26, 2016 at 03:34:48PM -0400, Doug Ledford wrote:
>> On 8/26/2016 3:29 PM, Leon Romanovsky wrote:
>>> On Fri, Aug 26, 2016 at 02:01:55PM -0400, Doug Ledford wrote:
>>>> On 8/26/2016 9:35 AM, Doug Ledford wrote:
>>>>> On 8/26/2016 12:49 AM, Christophe JAILLET wrote:
>>>>>> The 2nd parameter of 'find_first_bit' is the number of bits to search.
>>>>>> In this case, we are passing 'sizeof(unsigned long)' which is likely to
>>>>>> be 4 or 8.
>>>>>
>>>>> If the size can be 4 or 8, then using 64 universally is not correct.
>>>>> Why not use sizeof() * 8 (or << 3)?
>>>>
>>>> Better yet, why not put this patch in the kernel first:
>>>>
>>>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>>>> index d96a6118d26a..a8838c87668e 100644
>>>> --- a/include/linux/kernel.h
>>>> +++ b/include/linux/kernel.h
>>>> @@ -52,6 +52,8 @@
>>>>
>>>>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
>>>> __must_be_array(arr))
>>>>
>>>> +#define bitsizeof(x)   (sizeof((x)) << 3)
>>>> +
>>>>  #define u64_to_user_ptr(x) (           \
>>>>  {                                      \
>>>>         typecheck(u64, x);              \
>>>>
>>>> then start going around replacing all these hard coded numbers with the
>>>> use of bitsizeof().  It can be applied not just to the find_first*bit()
>>>> routines, but to a bunch of other routines too.  Just look at
>>>> include/linux/bitmap.h and any that have nbits as an argument are
>>>> candidates.
>>>
>>> There is BITS_PER_LONG define for that. There is actual use of it in mlx5 for
>>> the similar code pieces.
>>
>> BITS_PER_LONG only works if your bitfield is a single long.  It doesn't
>> work for other bitfields.  What I posted above will work for anything.
> 
> Yes, the question to ask if it is really needed.

A quick review of the usage of find_first_bit in the kernel shows that
the majority of uses that use large, complex bit arrays (things larger
than a single long, where bitsizeof(complex_object) might be helpful),
also tend to have limits to their bitmap sizes that do not directly
equal the size of the bitmap.  For example, the bitmap for an md raid
array is allocated as a chunk of memory, where the chunk of memory is
larger than the bitmap size as a general rule, so
bitsizeof(chunk_of_memory) would run off the end of the real bitmap.
Likewise for a number of other complex bitmaps (block layer tag in use
bitmap for instance).

So, is it useful?  I think so.  But it's not needed for original patch
in this thread.


-- 
Doug Ledford <dledford@redhat.com>
    GPG Key ID: 0E572FDD

[toc] | [prev] | [next] | [standalone]

#1475896

[Multipart message — attachments visible in raw view] — view raw

On Fri, Sep 02, 2016 at 10:39:11AM -0400, Doug Ledford wrote:
> On 8/28/2016 2:06 AM, Leon Romanovsky wrote:
> > On Fri, Aug 26, 2016 at 03:34:48PM -0400, Doug Ledford wrote:
> >> On 8/26/2016 3:29 PM, Leon Romanovsky wrote:
> >>> On Fri, Aug 26, 2016 at 02:01:55PM -0400, Doug Ledford wrote:
> >>>> On 8/26/2016 9:35 AM, Doug Ledford wrote:
> >>>>> On 8/26/2016 12:49 AM, Christophe JAILLET wrote:
> >>>>>> The 2nd parameter of 'find_first_bit' is the number of bits to search.
> >>>>>> In this case, we are passing 'sizeof(unsigned long)' which is likely to
> >>>>>> be 4 or 8.
> >>>>>
> >>>>> If the size can be 4 or 8, then using 64 universally is not correct.
> >>>>> Why not use sizeof() * 8 (or << 3)?
> >>>>
> >>>> Better yet, why not put this patch in the kernel first:
> >>>>
> >>>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> >>>> index d96a6118d26a..a8838c87668e 100644
> >>>> --- a/include/linux/kernel.h
> >>>> +++ b/include/linux/kernel.h
> >>>> @@ -52,6 +52,8 @@
> >>>>
> >>>>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> >>>> __must_be_array(arr))
> >>>>
> >>>> +#define bitsizeof(x)   (sizeof((x)) << 3)
> >>>> +
> >>>>  #define u64_to_user_ptr(x) (           \
> >>>>  {                                      \
> >>>>         typecheck(u64, x);              \
> >>>>
> >>>> then start going around replacing all these hard coded numbers with the
> >>>> use of bitsizeof().  It can be applied not just to the find_first*bit()
> >>>> routines, but to a bunch of other routines too.  Just look at
> >>>> include/linux/bitmap.h and any that have nbits as an argument are
> >>>> candidates.
> >>>
> >>> There is BITS_PER_LONG define for that. There is actual use of it in mlx5 for
> >>> the similar code pieces.
> >>
> >> BITS_PER_LONG only works if your bitfield is a single long.  It doesn't
> >> work for other bitfields.  What I posted above will work for anything.
> >
> > Yes, the question to ask if it is really needed.
>
> A quick review of the usage of find_first_bit in the kernel shows that
> the majority of uses that use large, complex bit arrays (things larger
> than a single long, where bitsizeof(complex_object) might be helpful),
> also tend to have limits to their bitmap sizes that do not directly
> equal the size of the bitmap.  For example, the bitmap for an md raid
> array is allocated as a chunk of memory, where the chunk of memory is
> larger than the bitmap size as a general rule, so
> bitsizeof(chunk_of_memory) would run off the end of the real bitmap.
> Likewise for a number of other complex bitmaps (block layer tag in use
> bitmap for instance).
>
> So, is it useful?  I think so.  But it's not needed for original patch
> in this thread.

Honestly, I wasn't convinced at all, but you are right,
it is theoretical discussion.

Thanks

>
>
> --
> Doug Ledford <dledford@redhat.com>
>     GPG Key ID: 0E572FDD
>

[toc] | [prev] | [next] | [standalone]

#1470964

[Multipart message — attachments visible in raw view] — view raw

On Fri, Aug 26, 2016 at 02:01:55PM -0400, Doug Ledford wrote:
> On 8/26/2016 9:35 AM, Doug Ledford wrote:
> > On 8/26/2016 12:49 AM, Christophe JAILLET wrote:
> >> The 2nd parameter of 'find_first_bit' is the number of bits to search.
> >> In this case, we are passing 'sizeof(unsigned long)' which is likely to
> >> be 4 or 8.
> >
> > If the size can be 4 or 8, then using 64 universally is not correct.
> > Why not use sizeof() * 8 (or << 3)?
>
> Better yet, why not put this patch in the kernel first:
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index d96a6118d26a..a8838c87668e 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -52,6 +52,8 @@
>
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
>
> +#define bitsizeof(x)   (sizeof((x)) << 3)
> +
>  #define u64_to_user_ptr(x) (           \
>  {                                      \
>         typecheck(u64, x);              \
>
> then start going around replacing all these hard coded numbers with the
> use of bitsizeof().  It can be applied not just to the find_first*bit()
> routines, but to a bunch of other routines too.  Just look at
> include/linux/bitmap.h and any that have nbits as an argument are
> candidates.

There is BITS_PER_LONG define for that. There is actual use of it in mlx5 for
the similar code pieces.

>
>
> --
> Doug Ledford <dledford@redhat.com>
>     GPG Key ID: 0E572FDD
>

[toc] | [prev] | [next] | [standalone]

#1471102

Le 26/08/2016 à 15:35, Doug Ledford a écrit :
> On 8/26/2016 12:49 AM, Christophe JAILLET wrote:
>> The 2nd parameter of 'find_first_bit' is the number of bits to search.
>> In this case, we are passing 'sizeof(unsigned long)' which is likely to
>> be 4 or 8.
> If the size can be 4 or 8, then using 64 universally is not correct.
> Why not use sizeof() * 8 (or << 3)?
I agree with you...

BTW, the log message is wrong. 'port_mask' is a u64. (not an unsigned 
long). So the sizeof should always be 8.
(cut and paste error from another patch, sorry)

>
>> It is likely that the number of bits of 'port_mask' was expected 
>> here. This
>> variable is a 'u64', so use 64 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-39ZsbGIQGT5GWvitb5QawA@public.gmane.org>
>> ---
>> Not sure that using 64 directly is the best option.
>> Maybe '8 * sizeof(port_mask)' as used in the same file for
>> 'for_each_set_bit' would be better
>> ---
... as noted here

Would you like a v2 patch or, will you update it by yourself?

Best regards,
CJ

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

[toc] | [prev] | [next] | [standalone]

#1475361

[Multipart message — attachments visible in raw view] — view raw

On 8/27/2016 1:25 AM, Christophe JAILLET wrote:
> Le 26/08/2016 à 15:35, Doug Ledford a écrit :
>> On 8/26/2016 12:49 AM, Christophe JAILLET wrote:
>>> The 2nd parameter of 'find_first_bit' is the number of bits to search.
>>> In this case, we are passing 'sizeof(unsigned long)' which is likely to
>>> be 4 or 8.
>> If the size can be 4 or 8, then using 64 universally is not correct.
>> Why not use sizeof() * 8 (or << 3)?
> I agree with you...
> 
> BTW, the log message is wrong. 'port_mask' is a u64. (not an unsigned
> long). So the sizeof should always be 8.
> (cut and paste error from another patch, sorry)

Log message modified, then patch modified to use sizeof() * 8, result
applied.  Thanks.

>>
>>> It is likely that the number of bits of 'port_mask' was expected
>>> here. This
>>> variable is a 'u64', so use 64 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-39ZsbGIQGT5GWvitb5QawA@public.gmane.org>
>>> ---
>>> Not sure that using 64 directly is the best option.
>>> Maybe '8 * sizeof(port_mask)' as used in the same file for
>>> 'for_each_set_bit' would be better
>>> ---
> ... as noted here
> 
> Would you like a v2 patch or, will you update it by yourself?
> 
> Best regards,
> CJ
> 
> ---
> L'absence de virus dans ce courrier électronique a été vérifiée par le
> logiciel antivirus Avast.
> https://www.avast.com/antivirus
> 


-- 
Doug Ledford <dledford@redhat.com>
    GPG Key ID: 0E572FDD

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web