Re: [PATCH 3/3] locking/atomic: openrisc: use generic_cmpxchg[64]_local for arch_cmpxchg[64]_local

[Vineet Gupta <vgupta@kernel.org>]

On 10/29/23 20:41, wuqiang.matt wrote:
>>>>
>>>> arch_cmpxchg_relaxed:
>>>> ...
>>>>           switch(sizeof((_p_))) {
>>>>           case 4:
>>>> ....
>>>>
>>>> arch_cmpxchg:
>>>> ...
>>>>     BUILD_BUG_ON(sizeof(_p_) != 4);
>>>> ...
>>>>
>>>> _p is the address pointer, so I'm thinking it's a typo but I couldn't
>>>> yet confirm. There is not much about arc processors in the web :(
>>> Hmm, indeed. This seems like a bug but it depends on the 'llock  %0, 
>>> [%1]'
>>> can take a 32bit address or 32bit data register. Usually it should
>>> check the size of data, but need to check with ISA manual.
>>>
>>> Vineet, can you check this suspicious bug?
>>
>> ARCv2 is a 32-bit ISA and LLOCK/SCOND work on 32-bit data.
>> So the pointers will be 32-bit anyways. Is the issue that 
>> pointer/cmpxchg operation could be on a smaller data type ?
>
> For ARCv2 with CONFIG_ARC_HAS_LLSC, better add the data size checking and
> only permit 32bit data size. Even for 32-bit system, data should can be
> 64bit 'long long'.

Right, makes sense. Can you send a patch ?

>
> And In the case that CONFIG_ARC_HAS_LLSC is undefined, in 
> arch_cmpxchg: the
> pointer size checking is unnecessary, since it's using spinlock 
> internally:

Correct, I had the same thought.

>
> https://elixir.bootlin.com/linux/v6.6-rc7/source/arch/arc/include/asm/cmpxchg.h#L60: 
>
>     BUILD_BUG_ON(sizeof(_p_) != 4);                    \
>                                     \
>     /*                                \
>      * spin lock/unlock provide the needed smp_mb() before/after    \
>      */                                \
>     atomic_ops_lock(__flags);                    \
>     _prev_ = *_p_;                            \
>     if (_prev_ == _o_)                        \
>         *_p_ = _n_;                        \
>     atomic_ops_unlock(__flags);

Can you do that fix in same patch as well ?

> Another question about the naming: arch_cmpxchg_relaxed() implemented if
> CONFIG_ARC_HAS_LLSC is configured and arch_cmpxchg() defined for the 
> rest.
> Are there any reasons for difference names ?

Yes the atomics API consists of _relaxed, _acquire, _release and unadorned.
_relaxed is the most efficient, with rest having some extra barriers.
If arch provides _relaxed, generic code can create the rest - assuming 
arch hardware can support the relaxed ones.
That is true for LLSC.

However for !LLSC, spinlock versions already have full barriers due to 
spinlock, so relaxed variant doesn't make sense.

>
> As I checked, Synopsys has released 64bit ARC processors (HS66/HS68), but
> I don't know the status of Linux kernel support.

Yes there's an internal ARC64 port running but it is not ready for 
upstreaming yet.

-Vineet