Re: [PATCH 2/2] xen: Change parameter of generic_fls() to unsigned int

[Michal Orzel <michal.orzel@xxxxxxx>]

On 04/09/2023 15:28, Jan Beulich wrote:
> 
> 
> On 04.09.2023 11:14, Michal Orzel wrote:
>> --- a/xen/include/xen/bitops.h
>> +++ b/xen/include/xen/bitops.h
>> @@ -51,7 +51,7 @@ static inline int generic_ffs(int x)
>>   * fls: find last bit set.
>>   */
>>
>> -static __inline__ int generic_fls(int x)
>> +static __inline__ int generic_fls(unsigned int x)
>>  {
>>      int r = 32;
>>
> 
> Even if perhaps not affected by UBSAN, generic_ffs() then wants taking care
> of as well, imo. If additionally you switch __inline__ to inline, things
> will become nicely symmetric with generic_f{f,l}sl().
If others agree, I can switch generic_ffs() to take unsigned as well (together 
with s/__inline__/inline/).
However, such change would no longer fall in fixes category (i.e. nothing wrong 
with ffs() taking int):
- is it ok at this stage of release? (not sure but no problem for me to do this)
- is it ok to mix a fix with non-fix change? (I always prefer fixes to be done 
as standalone changes)

> 
> Another aspect that may be nice to take care of at this occasion is their
> return values: None of them can return negative values, so return type
> would better be unsigned int.
Looking at all the variations (including arch asm specific) of helpers to find 
first set/clear in the codebase, returning int is the de-facto standard.
So, this would result in some inconsistency or require changing at least the 
direct callers to also return unsigned.
I'd prefer not to be required to add this extra churn at this release stage.

~Michal