[PATCH 3/3] coresight etm4x: Add 32-bit read/write option to split 64-bit words

[Steve Clevenger]

On 3/9/2023 5:35 PM, Leo Yan wrote:
> On Thu, Mar 09, 2023 at 12:46:55PM -0800, Steve Clevenger wrote:
> 
> [...]
> 
>>> #include <linux/io-64-nonatomic-hi-lo.h>
>>>
>>> static inline u64 etm4x_relaxed_read64(struct csdev_access *csa, unsigned int offset)
>>> {
>>> 	if (csa->io_mem) {
>>> 		if (csa->no_quad_mmio)
>>>                         return hi_lo_readq_relaxed(csa->base + offset);
>>> 		else
>>> 			return readq_relaxed(csa->base + offset);
>>> 	} else {
>>> 		return read_etm4x_sysreg_offset(offset, true);
>>>         }
>>> }
>>>
>>> I'm not saying to play magic on {writeq|readq}_relaxed(), would it be
>>> fine to directly call hi_lo_readq_relaxed()?
>>
>> I can do this, and it would work fine. Without compile protection, one
>> concern is the silent inclusion of io-64-nonatomic-hi-lo.h definitions
>> for readq_relaxed/writeq_relaxed if the include hierarchy gets changed.
>> The only (minor) risk I see is to performance.
> 
> Yeah, we could include io.h ahead io-64-nonatomic-hi-lo.h:
> 
>   #include <linux/io.h>
>   #include <linux/io-64-nonatomic-lo-hi.h>
> 
> Thus we can get the definitions for {readq|writeq}_relaxed() from
> "io.h".

It seems the actual read_relaxed/write_relaxed definitions come from the
asm-generic/io.h version already in the include hierarchy without adding
linux/io.h. A more foolproof protection is to check whether these are
defined before the io-64-nonatomic-hi-lo.h include and error out if not.

> 
>> Second, it seems to me the hi_lo and lo_hi macros were intended to deal
>> with endianness. IMO, the implementation I offered is agnostic without a
>> hint to endianness or atomicity.
> 
> Seems to me, kernel functions with explict endianness naming is not bad
> thing :)
> 
>> The CoreSight maintainer audience is
>> limited and already aware of these issues, so I'll make the change for you.
> 
> Thanks!  Suzuki, Mike, I think it's good to get your opinion before
> Steve can proceed for updating patch.
> 
> Leo