Re: [PATCH v3 02/25] bus: mhi: Fix MHI DMA structure endianness

16 Feb 2022

On 2/16/22 1:04 AM, Manivannan Sadhasivam wrote:
...
On Tue, Feb 15, 2022 at 02:02:01PM -0600, Alex Elder wrote:
...
On 2/12/22 12:20 PM, Manivannan Sadhasivam wrote:
...
From: Paul Davey paul.davey@alliedtelesis.co.nz
The MHI driver does not work on big endian architectures.  The
controller never transitions into mission mode.  This appears to be due
to the modem device expecting the various contexts and transfer rings to
have fields in little endian order in memory, but the driver constructs
them in native endianness.
Yes, this is true.
...
Fix MHI event, channel and command contexts and TRE handling macros to
use explicit conversion to little endian.  Mark fields in relevant
structures as little endian to document this requirement.
Basically every field in the external interface whose size
is greater than one byte must have its endianness noted.
 From what I can tell, you did that for all of the exposed
structures defined in "drivers/bus/mhi/core/internal.h",
which is good.
*However* some of the *constants* were defined the wrong way.
Basically, all of the constant values should be expressed
in host byte order.  And any needed byte swapping should be
done at the time the value is read from memory--immediately.
That way, we isolate that activity to the one place we
interface with the possibly "foreign" format, and from then
on, everything may be assumed to be in natural (CPU) byte order.
Well, I did think about it but I convinced myself that doing the
conversion in code rather in defines make the code look messy.
Also in some places it just makes it look complicated. More below:
I thought this might the case.
...
...
I will point out what I mean, below.
...
Fixes: a6e2e3522f29 ("bus: mhi: core: Add support for PM state transitions")
Fixes: 6cd330ae76ff ("bus: mhi: core: Add support for ringing channel/event ring doorbells")
Signed-off-by: Paul Davey paul.davey@alliedtelesis.co.nz
Cc: stable@vger.kernel.org
Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org

drivers/bus/mhi/core/debugfs.c  |  26 +++----
   drivers/bus/mhi/core/init.c     |  36 +++++-----
   drivers/bus/mhi/core/internal.h | 119 ++++++++++++++++----------------
   drivers/bus/mhi/core/main.c     |  22 +++---
   drivers/bus/mhi/core/pm.c       |   4 +-
   5 files changed, 104 insertions(+), 103 deletions(-)
[...]
...
...
@@ -277,57 +277,58 @@ enum mhi_cmd_type {
   /* No operation command */
   #define MHI_TRE_CMD_NOOP_PTR (0)
   #define MHI_TRE_CMD_NOOP_DWORD0 (0)
-#define MHI_TRE_CMD_NOOP_DWORD1 (MHI_CMD_NOP << 16)
+#define MHI_TRE_CMD_NOOP_DWORD1 (cpu_to_le32(MHI_CMD_NOP << 16))
This just looks wrong to me.  The original definition
should be fine, but then where it's *used* it should
be passed to cpu_to_le32().  I realize this might be
a special case, where these "DWORD" values are getting
written out to command ring elements, but even so, the
byte swapping that's happening is important and should
be made obvious in the code using these symbols.
This comment applies to many more similar definitions
below.  I don't know; maybe it looks cumbersome if
it's done in the code, but I still think it's better to
consistenly define symbols like this in CPU byte order
and do the conversions explicitly only when the values
are read/written to "foreign" (external interface)
memory.
Defines like MHI_TRE_GET_CMD_CHID are making the conversion look messy
to me. In this we first extract the DWORD from TRE and then doing
shifting + masking to get the CHID.
I didn't say so, but I don't really like those defines either.
I personally would rather see the field values get extracted
in open code rather than this, because they're actually pretty
simple operations.  But I understand, sometimes things just
"look complicated" if you do them certain ways (even if simple).
I did it in a certain way in the IPA code and I find that
preferable to the use of the "DWORD" definitions you're
using.  I also stand by my belief that it's preferable to
not hide the byte swaps in macro definitions.
You use this for reading/writing the command/transfer/event
ring elements (only) though, and you do that consistently.
...
So without splitting the DWORD extraction and GET_CHID macros
separately, we can't just do the conversion in code. And we may end up
doing the conversion in defines just for these special cases but that
will break the uniformity.
So IMO it looks better if we trust the defines to do the conversion itself.
Please let me know if you think the other way.
I'm OK with it.  I'm not convinced, but I won't object...
-Alex
...
Thanks,
Mani
...
Outside of this issue, the remainder of the patch looks
OK to me.
			-Alex


...
/* Channel reset command */
   #define MHI_TRE_CMD_RESET_PTR (0)
   #define MHI_TRE_CMD_RESET_DWORD0 (0)
-#define MHI_TRE_CMD_RESET_DWORD1(chid) ((chid << 24) | \

			(MHI_CMD_RESET_CHAN << 16))



+#define MHI_TRE_CMD_RESET_DWORD1(chid) (cpu_to_le32((chid << 24) | \

			(MHI_CMD_RESET_CHAN << 16)))

/* Channel stop command */
#define MHI_TRE_CMD_STOP_PTR (0)
#define MHI_TRE_CMD_STOP_DWORD0 (0)

-#define MHI_TRE_CMD_STOP_DWORD1(chid) ((chid << 24) | \

		       (MHI_CMD_STOP_CHAN << 16))



+#define MHI_TRE_CMD_STOP_DWORD1(chid) (cpu_to_le32((chid << 24) | \

		       (MHI_CMD_STOP_CHAN << 16)))

/* Channel start command */
#define MHI_TRE_CMD_START_PTR (0)
#define MHI_TRE_CMD_START_DWORD0 (0)

-#define MHI_TRE_CMD_START_DWORD1(chid) ((chid << 24) | \

			(MHI_CMD_START_CHAN << 16))



+#define MHI_TRE_CMD_START_DWORD1(chid) (cpu_to_le32((chid << 24) | \

			(MHI_CMD_START_CHAN << 16)))



-#define MHI_TRE_GET_CMD_CHID(tre) (((tre)->dword[1] >> 24) & 0xFF)
-#define MHI_TRE_GET_CMD_TYPE(tre) (((tre)->dword[1] >> 16) & 0xFF)
+#define MHI_TRE_GET_DWORD(tre, word) (le32_to_cpu((tre)->dword[(word)]))
+#define MHI_TRE_GET_CMD_CHID(tre) ((MHI_TRE_GET_DWORD(tre, 1) >> 24) & 0xFF)
+#define MHI_TRE_GET_CMD_TYPE(tre) ((MHI_TRE_GET_DWORD(tre, 1) >> 16) & 0xFF)
   /* Event descriptor macros */
-#define MHI_TRE_EV_PTR(ptr) (ptr)
-#define MHI_TRE_EV_DWORD0(code, len) ((code << 24) | len)
-#define MHI_TRE_EV_DWORD1(chid, type) ((chid << 24) | (type << 16))
-#define MHI_TRE_GET_EV_PTR(tre) ((tre)->ptr)
-#define MHI_TRE_GET_EV_CODE(tre) (((tre)->dword[0] >> 24) & 0xFF)
-#define MHI_TRE_GET_EV_LEN(tre) ((tre)->dword[0] & 0xFFFF)
-#define MHI_TRE_GET_EV_CHID(tre) (((tre)->dword[1] >> 24) & 0xFF)
-#define MHI_TRE_GET_EV_TYPE(tre) (((tre)->dword[1] >> 16) & 0xFF)
-#define MHI_TRE_GET_EV_STATE(tre) (((tre)->dword[0] >> 24) & 0xFF)
-#define MHI_TRE_GET_EV_EXECENV(tre) (((tre)->dword[0] >> 24) & 0xFF)
-#define MHI_TRE_GET_EV_SEQ(tre) ((tre)->dword[0])
-#define MHI_TRE_GET_EV_TIME(tre) ((tre)->ptr)
-#define MHI_TRE_GET_EV_COOKIE(tre) lower_32_bits((tre)->ptr)
-#define MHI_TRE_GET_EV_VEID(tre) (((tre)->dword[0] >> 16) & 0xFF)
-#define MHI_TRE_GET_EV_LINKSPEED(tre) (((tre)->dword[1] >> 24) & 0xFF)
-#define MHI_TRE_GET_EV_LINKWIDTH(tre) ((tre)->dword[0] & 0xFF)
+#define MHI_TRE_EV_PTR(ptr) (cpu_to_le64(ptr))
+#define MHI_TRE_EV_DWORD0(code, len) (cpu_to_le32((code << 24) | len))
+#define MHI_TRE_EV_DWORD1(chid, type) (cpu_to_le32((chid << 24) | (type << 16)))
+#define MHI_TRE_GET_EV_PTR(tre) (le64_to_cpu((tre)->ptr))
+#define MHI_TRE_GET_EV_CODE(tre) ((MHI_TRE_GET_DWORD(tre, 0) >> 24) & 0xFF)
+#define MHI_TRE_GET_EV_LEN(tre) (MHI_TRE_GET_DWORD(tre, 0) & 0xFFFF)
+#define MHI_TRE_GET_EV_CHID(tre) ((MHI_TRE_GET_DWORD(tre, 1) >> 24) & 0xFF)
+#define MHI_TRE_GET_EV_TYPE(tre) ((MHI_TRE_GET_DWORD(tre, 1) >> 16) & 0xFF)
+#define MHI_TRE_GET_EV_STATE(tre) ((MHI_TRE_GET_DWORD(tre, 0) >> 24) & 0xFF)
+#define MHI_TRE_GET_EV_EXECENV(tre) ((MHI_TRE_GET_DWORD(tre, 0) >> 24) & 0xFF)
+#define MHI_TRE_GET_EV_SEQ(tre) MHI_TRE_GET_DWORD(tre, 0)
+#define MHI_TRE_GET_EV_TIME(tre) (MHI_TRE_GET_EV_PTR(tre))
+#define MHI_TRE_GET_EV_COOKIE(tre) lower_32_bits(MHI_TRE_GET_EV_PTR(tre))
+#define MHI_TRE_GET_EV_VEID(tre) ((MHI_TRE_GET_DWORD(tre, 0) >> 16) & 0xFF)
+#define MHI_TRE_GET_EV_LINKSPEED(tre) ((MHI_TRE_GET_DWORD(tre, 1) >> 24) & 0xFF)
+#define MHI_TRE_GET_EV_LINKWIDTH(tre) (MHI_TRE_GET_DWORD(tre, 0) & 0xFF)
   /* Transfer descriptor macros */
-#define MHI_TRE_DATA_PTR(ptr) (ptr)
-#define MHI_TRE_DATA_DWORD0(len) (len & MHI_MAX_MTU)
-#define MHI_TRE_DATA_DWORD1(bei, ieot, ieob, chain) ((2 << 16) | (bei << 10) \

| (ieot << 9) | (ieob << 8) | chain)

+#define MHI_TRE_DATA_PTR(ptr) (cpu_to_le64(ptr))
+#define MHI_TRE_DATA_DWORD0(len) (cpu_to_le32(len & MHI_MAX_MTU))
+#define MHI_TRE_DATA_DWORD1(bei, ieot, ieob, chain) (cpu_to_le32((2 << 16) | (bei << 10) \

| (ieot << 9) | (ieob << 8) | chain))
/* RSC transfer descriptor macros */

-#define MHI_RSCTRE_DATA_PTR(ptr, len) (((u64)len << 48) | ptr)
-#define MHI_RSCTRE_DATA_DWORD0(cookie) (cookie)
-#define MHI_RSCTRE_DATA_DWORD1 (MHI_PKT_TYPE_COALESCING << 16)
+#define MHI_RSCTRE_DATA_PTR(ptr, len) (cpu_to_le64(((u64)len << 48) | ptr))
+#define MHI_RSCTRE_DATA_DWORD0(cookie) (cpu_to_le32(cookie))
+#define MHI_RSCTRE_DATA_DWORD1 (cpu_to_le32(MHI_PKT_TYPE_COALESCING << 16))
   enum mhi_pkt_type {
   	MHI_PKT_TYPE_INVALID = 0x0,
@@ -500,7 +501,7 @@ struct state_transition {
   struct mhi_ring {
   	dma_addr_t dma_handle;
   	dma_addr_t iommu_base;

u64 *ctxt_wp; /* point to ctxt wp */


__le64 *ctxt_wp; /* point to ctxt wp */
void *pre_aligned;
void *base;
void *rp;

diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index ffde617f93a3..85f4f7c8d7c6 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -114,7 +114,7 @@ void mhi_ring_er_db(struct mhi_event *mhi_event)
   	struct mhi_ring *ring = &mhi_event->ring;
   	mhi_event->db_cfg.process_db(mhi_event->mhi_cntrl, &mhi_event->db_cfg,

		     ring->db_addr, *ring->ctxt_wp);




		     ring->db_addr, le64_to_cpu(*ring->ctxt_wp));

}
void mhi_ring_cmd_db(struct mhi_controller *mhi_cntrl, struct mhi_cmd *mhi_cmd)

@@ -123,7 +123,7 @@ void mhi_ring_cmd_db(struct mhi_controller *mhi_cntrl, struct mhi_cmd *mhi_cmd)
   	struct mhi_ring *ring = &mhi_cmd->ring;
   	db = ring->iommu_base + (ring->wp - ring->base);

*ring->ctxt_wp = db;


*ring->ctxt_wp = cpu_to_le64(db);
mhi_write_db(mhi_cntrl, ring->db_addr, db);
}

@@ -140,7 +140,7 @@ void mhi_ring_chan_db(struct mhi_controller *mhi_cntrl,
   	 * before letting h/w know there is new element to fetch.
   	 */
   	dma_wmb();

*ring->ctxt_wp = db;


*ring->ctxt_wp = cpu_to_le64(db);
mhi_chan->db_cfg.process_db(mhi_cntrl, &mhi_chan->db_cfg,
			    ring->db_addr, db);

@@ -432,7 +432,7 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev)
   	struct mhi_event_ctxt *er_ctxt =
   		&mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index];
   	struct mhi_ring *ev_ring = &mhi_event->ring;

dma_addr_t ptr = er_ctxt->rp;


dma_addr_t ptr = le64_to_cpu(er_ctxt->rp);
void *dev_rp;
if (!is_valid_ring_ptr(ev_ring, ptr)) {

@@ -537,14 +537,14 @@ static void mhi_recycle_ev_ring_element(struct mhi_controller *mhi_cntrl,
   	/* Update the WP */
   	ring->wp += ring->el_size;

ctxt_wp = *ring->ctxt_wp + ring->el_size;


ctxt_wp = le64_to_cpu(*ring->ctxt_wp) + ring->el_size;
if (ring->wp >= (ring->base + ring->len)) {
	ring->wp = ring->base;
	ctxt_wp = ring->iommu_base;
}


*ring->ctxt_wp = ctxt_wp;


*ring->ctxt_wp = cpu_to_le64(ctxt_wp);
/* Update the RP */
ring->rp += ring->el_size;

@@ -801,7 +801,7 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
   	struct device *dev = &mhi_cntrl->mhi_dev->dev;
   	u32 chan;
   	int count = 0;

dma_addr_t ptr = er_ctxt->rp;


dma_addr_t ptr = le64_to_cpu(er_ctxt->rp);
/*
This is a quick check to avoid unnecessary event processing



@@ -940,7 +940,7 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl,
   		mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring);
   		local_rp = ev_ring->rp;

ptr = er_ctxt->rp;




ptr = le64_to_cpu(er_ctxt->rp);
if (!is_valid_ring_ptr(ev_ring, ptr)) {
	dev_err(&mhi_cntrl->mhi_dev->dev,
		"Event ring rp points outside of the event ring\n");



@@ -970,7 +970,7 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
   	int count = 0;
   	u32 chan;
   	struct mhi_chan *mhi_chan;

dma_addr_t ptr = er_ctxt->rp;


dma_addr_t ptr = le64_to_cpu(er_ctxt->rp);
if (unlikely(MHI_EVENT_ACCESS_INVALID(mhi_cntrl->pm_state)))
	return -EIO;

@@ -1011,7 +1011,7 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl,
   		mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring);
   		local_rp = ev_ring->rp;

ptr = er_ctxt->rp;




ptr = le64_to_cpu(er_ctxt->rp);
if (!is_valid_ring_ptr(ev_ring, ptr)) {
	dev_err(&mhi_cntrl->mhi_dev->dev,
		"Event ring rp points outside of the event ring\n");



@@ -1533,7 +1533,7 @@ static void mhi_mark_stale_events(struct mhi_controller *mhi_cntrl,
   	/* mark all stale events related to channel as STALE event */
   	spin_lock_irqsave(&mhi_event->lock, flags);

ptr = er_ctxt->rp;


ptr = le64_to_cpu(er_ctxt->rp);
if (!is_valid_ring_ptr(ev_ring, ptr)) {
	dev_err(&mhi_cntrl->mhi_dev->dev,
		"Event ring rp points outside of the event ring\n");

diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 4aae0baea008..c35c5ddc7220 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -218,7 +218,7 @@ int mhi_ready_state_transition(struct mhi_controller *mhi_cntrl)
   			continue;
   		ring->wp = ring->base + ring->len - ring->el_size;

*ring->ctxt_wp = ring->iommu_base + ring->len - ring->el_size;




*ring->ctxt_wp = cpu_to_le64(ring->iommu_base + ring->len - ring->el_size);
/* Update all cores */
smp_wmb();



@@ -420,7 +420,7 @@ static int mhi_pm_mission_mode_transition(struct mhi_controller *mhi_cntrl)
   			continue;
   		ring->wp = ring->base + ring->len - ring->el_size;

*ring->ctxt_wp = ring->iommu_base + ring->len - ring->el_size;




*ring->ctxt_wp = cpu_to_le64(ring->iommu_base + ring->len - ring->el_size);
/* Update to all cores */
smp_wmb();




    

2024

2023

2022

2021

2020

2019

2018

2017

Re: [PATCH v3 02/25] bus: mhi: Fix MHI DMA structure endianness