On 08.09.2023 01:12, Shawn Anastasio wrote:
> On 9/5/23 10:19 AM, Jan Beulich wrote:
>> On 01.09.2023 20:25, Shawn Anastasio wrote:
>>> +#define DEFINE_TESTOP(fn, op, eh)
>>> \
>>> +static inline unsigned long fn(unsigned long mask, volatile unsigned int
>>> *_p) \
>>> +{
>>> \
>>> + unsigned long old, t;
>>> \
>>> + unsigned int *p = (unsigned int *)_p;
>>> \
>>> + asm volatile ( PPC_ATOMIC_ENTRY_BARRIER
>>> \
>>> + "1: lwarx %0,0,%3,%4\n"
>>> \
>>> + #op "%I2 %1,%0,%2\n"
>>> \
>>> + "stwcx. %1,0,%3\n"
>>> \
>>> + "bne- 1b\n"
>>> \
>>> + PPC_ATOMIC_EXIT_BARRIER
>>> \
>>> + : "=&r" (old), "=&r" (t)
>>> \
>>> + : "rK" (mask), "r" (p), "n" (eh)
>>> \
>>> + : "cc", "memory" );
>>> \
>>> + return (old & mask);
>>> \
>>> +}
>>> +
>>> +DEFINE_TESTOP(test_and_set_bits, or, 0)
>>
>> Why can't test_and_clear_bits() not use this macro-ization? And if it
>> can't and hence there's only this single use, wouldn't it make sense
>> to avoid going through a macro here, too?
>>
>
> I've just tried this, but unfortunately the "rK" constraint on the mask
> operand only works when instructions have both a reg/reg/reg as well as
> a reg/reg/imm16 form. This is the case for `or` but not `andc`. I guess
> it would be better to keep the two separate implementations rather than
> try to accomodate both cases in the macro somehow (e.g, by making the
> constraint's type a macro parameter as well).
>
> I can also change the macro template into a standard function for just
> test_and_set_bits, given that it's the only user as you pointed out.
>
> As for your previous observation about the superfluous `p` variable, it
> would seem the same applies to the macro here. Other than casting away
> the volatile qualifier on `p_` it doesn't seem to be doing much, so I'm
> inclined to remove it.
Right. Comments like this are generally intended to apply to the entire
patch or even entire series.
Jan
