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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.)

> +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.)

> +    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.

Jan