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

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

[Christophe JAILLET]

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

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

[Christophe JAILLET]

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

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 3016 bytes --]

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-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
> ---
>  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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 2958 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 2958 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1246 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1246 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1443 bytes --]

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
>




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1443 bytes --]

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
>




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1726 bytes --]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1697 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1697 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Christophe JAILLET]

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

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

[Christophe JAILLET]

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

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

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]

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
>




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1852 bytes --]

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
>




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 2709 bytes --]

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 2680 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 2680 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1733 bytes --]

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-XMD5yJDbdMReXY1tMh2IBg@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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1681 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 1681 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

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

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2897 bytes --]

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
>




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2897 bytes --]

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
>




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]