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

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