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

[Steve Clevenger]

It seems there's a bit of a lull in this patch discussion. Leo, I'll
resubmit the patch with the compile protection I suggested in my last
response.

Steve

On 3/10/2023 8:55 AM, Steve Clevenger wrote:
> 
> 
> 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