Re: [PATCH v4 2/5] xen/ppc: Implement bitops.h

[Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>]

On 9/11/23 8:53 AM, Jan Beulich wrote:
> On 09.09.2023 00:50, Shawn Anastasio wrote:
>> +/**
>> + * test_bit - Determine whether a bit is set
>> + * @nr: bit number to test
>> + * @addr: Address to start counting from
>> + */
>> +static inline int test_bit(int nr, const volatile void *addr)
>> +{
>> +    const volatile unsigned long *p = (const volatile unsigned long *)addr;
>> +    return 1 & (p[BITOP_WORD(nr)] >> (nr & (BITOP_BITS_PER_WORD - 1)));
>> +}
> 
> Do you really mean unsigned long in here? (As an aside I'd also recommend
> to drop the cast; I don't think it's needed for anything.)
>

Good point -- this should definitely be int.

>> +static inline unsigned long test_and_clear_bits(
>> +    unsigned long mask,
> 
> While apparently benign here, perhaps also better to use unsigned int.
> (Looks like I even missed instances yet further up.)
> 
>> +    volatile unsigned int *p)
>> +{
>> +    unsigned long old, t;
> 
> I'm less certain here, because I don't know what exactly "r" permits on
> ppc. (Having said that I notice that mask also is used with an "r"
> constraint, so restrictions would then apply there as well. But if long
> is really needed in various placed, then I would say that a comment is
> warranted, perhaps next to the BITOP_* defines.)
>

I've tested it and it seems "r" works as expected with 32-bit operands,
so there should be no problem changing these longs to ints.

>> +    asm volatile ( PPC_ATOMIC_ENTRY_BARRIER
>> +                   "1: lwarx %0,0,%3,0\n"
>> +                   "andc %1,%0,%2\n"
>> +                   "stwcx. %1,0,%3\n"
>> +                   "bne- 1b\n"
>> +                   PPC_ATOMIC_EXIT_BARRIER
>> +                   : "=&r" (old), "=&r" (t)
>> +                   : "r" (mask), "r" (p)
>> +                   : "cc", "memory" );
>> +
>> +    return (old & mask);
>> +}
>> +
>> +static inline int test_and_clear_bit(unsigned int nr,
>> +                                     volatile void *addr)
>> +{
>> +    return test_and_clear_bits(BITOP_MASK(nr), (volatile unsigned int 
>> *)addr +
>> +                               BITOP_WORD(nr)) != 0;
> 
> I think this is misleading wrapping of arguments. Even if not strictly
> mandated, imo if an argument doesn't fit on the rest of a line it should
> start on a fresh one.
>

Will fix.

> Jan

Thanks,
Shawn