nr_cpu_ids is unsigned int, but find_first_bit returns unsigned long (at
least on arm). Use the larger type for min_t to avoid larger-to-smaller
type assignments. This address 141 MISRA C 10.3 violations.

Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---

diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h
index 9826707909..a6ed6a28e8 100644
--- a/xen/include/xen/cpumask.h
+++ b/xen/include/xen/cpumask.h
@@ -208,7 +208,7 @@ static inline void cpumask_copy(cpumask_t *dstp, const cpumask_t *srcp)
 
 static inline int cpumask_first(const cpumask_t *srcp)
 {
-	return min_t(int, nr_cpu_ids, find_first_bit(srcp->bits, nr_cpu_ids));
+	return min_t(unsigned long, nr_cpu_ids, find_first_bit(srcp->bits, nr_cpu_ids));
 }
 
 static inline int cpumask_next(int n, const cpumask_t *srcp)

Hi Stefano,

On 29/09/2023 00:32, Stefano Stabellini wrote:
> nr_cpu_ids is unsigned int, but find_first_bit returns unsigned long (at
> least on arm).

find_* are meant to be used by common code. So I think the first step is 
to correct the return type so it is consistent across all architectures.

I don't have a strong opinion on whether they should all return 
'unsigned long'.

Then we can discuss if the MISRA rule is still violated.

> Use the larger type for min_t to avoid larger-to-smaller
> type assignments. This address 141 MISRA C 10.3 violations.

I find interesting you are saying this given that...
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
> 
> diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h
> index 9826707909..a6ed6a28e8 100644
> --- a/xen/include/xen/cpumask.h
> +++ b/xen/include/xen/cpumask.h
> @@ -208,7 +208,7 @@ static inline void cpumask_copy(cpumask_t *dstp, const cpumask_t *srcp)
>   
>   static inline int cpumask_first(const cpumask_t *srcp)
>   {
> -	return min_t(int, nr_cpu_ids, find_first_bit(srcp->bits, nr_cpu_ids));
> +	return min_t(unsigned long, nr_cpu_ids, find_first_bit(srcp->bits, nr_cpu_ids));

... cpumask_first() is return 'int'. So rather than fixing it, you seem 
to have just moved the violation.

>   }
>   
>   static inline int cpumask_next(int n, const cpumask_t *srcp)

Cheers,

-- 
Julien Grall

> On 29 Sep 2023, at 08:31, Julien Grall <julien@xen.org> wrote:
> 
> Hi Stefano,
> 
> On 29/09/2023 00:32, Stefano Stabellini wrote:
>> nr_cpu_ids is unsigned int, but find_first_bit returns unsigned long (at
>> least on arm).
> 
> find_* are meant to be used by common code. So I think the first step is to correct the return type so it is consistent across all architectures.
> 
> I don't have a strong opinion on whether they should all return 'unsigned long'.
> 
> Then we can discuss if the MISRA rule is still violated.
> 
>> Use the larger type for min_t to avoid larger-to-smaller
>> type assignments. This address 141 MISRA C 10.3 violations.
> 
> I find interesting you are saying this given that...
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
>> ---
>> diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h
>> index 9826707909..a6ed6a28e8 100644
>> --- a/xen/include/xen/cpumask.h
>> +++ b/xen/include/xen/cpumask.h
>> @@ -208,7 +208,7 @@ static inline void cpumask_copy(cpumask_t *dstp, const cpumask_t *srcp)
>>    static inline int cpumask_first(const cpumask_t *srcp)
>>  {
>> - return min_t(int, nr_cpu_ids, find_first_bit(srcp->bits, nr_cpu_ids));
>> + return min_t(unsigned long, nr_cpu_ids, find_first_bit(srcp->bits, nr_cpu_ids));
> 
> ... cpumask_first() is return 'int'. So rather than fixing it, you seem to have just moved the violation.
> 
>>  }
>>    static inline int cpumask_next(int n, const cpumask_t *srcp)

I’ve also found that find_first_bit returns:

 - unsigned int on x86
 - unsigned long on ppc
 - unsigned long on arm64
 - int on arm32 (seems that value is always >= 0

So maybe they can be all unsigned int, and cpumask_first can be as well unsigned int?


> 
> Cheers,
> 
> -- 
> Julien Grall
> 

On Fri, 29 Sep 2023, Luca Fancellu wrote:
> > On 29 Sep 2023, at 08:31, Julien Grall <julien@xen.org> wrote:
> > 
> > Hi Stefano,
> > 
> > On 29/09/2023 00:32, Stefano Stabellini wrote:
> >> nr_cpu_ids is unsigned int, but find_first_bit returns unsigned long (at
> >> least on arm).
> > 
> > find_* are meant to be used by common code. So I think the first step is to correct the return type so it is consistent across all architectures.
> > 
> > I don't have a strong opinion on whether they should all return 'unsigned long'.
> > 
> > Then we can discuss if the MISRA rule is still violated.
> > 
> >> Use the larger type for min_t to avoid larger-to-smaller
> >> type assignments. This address 141 MISRA C 10.3 violations.
> > 
> > I find interesting you are saying this given that...
> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> >> ---
> >> diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h
> >> index 9826707909..a6ed6a28e8 100644
> >> --- a/xen/include/xen/cpumask.h
> >> +++ b/xen/include/xen/cpumask.h
> >> @@ -208,7 +208,7 @@ static inline void cpumask_copy(cpumask_t *dstp, const cpumask_t *srcp)
> >>    static inline int cpumask_first(const cpumask_t *srcp)
> >>  {
> >> - return min_t(int, nr_cpu_ids, find_first_bit(srcp->bits, nr_cpu_ids));
> >> + return min_t(unsigned long, nr_cpu_ids, find_first_bit(srcp->bits, nr_cpu_ids));
> > 
> > ... cpumask_first() is return 'int'. So rather than fixing it, you seem to have just moved the violation.
> > 
> >>  }
> >>    static inline int cpumask_next(int n, const cpumask_t *srcp)
> 
> I’ve also found that find_first_bit returns:
> 
>  - unsigned int on x86
>  - unsigned long on ppc
>  - unsigned long on arm64
>  - int on arm32 (seems that value is always >= 0
> 
> So maybe they can be all unsigned int, and cpumask_first can be as well unsigned int?

I am OK with that. Julien, Shawn do you agree? If so, I can make the
change to find_first_bit so that it returns unsigned int on all arches.