| Message ID | alpine.DEB.2.22.394.2309281629360.1996340@ubuntu-linux-20-04-desktop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cpumask: fix integer type in cpumask_first | expand |
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,
> 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.
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)
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> ---