This series fixes several issues on the MHI EP stack and also improves
the suspend and resume functionality.
Thanks,
Mani
Manivannan Sadhasivam (6):
bus: mhi: ep: Power up/down MHI stack during MHI RESET
bus: mhi: ep: Check if the channel is supported by the controller
bus: mhi: ep: Only send -ENOTCONN status if client driver is available
bus: mhi: ep: Fix the debug message for MHI_PKT_TYPE_RESET_CHAN_CMD
cmd
bus: mhi: ep: Move chan->lock to the start of processing queued ch
ring
bus: mhi: ep: Save channel state locally during suspend and resume
drivers/bus/mhi/ep/main.c | 79 +++++++++++++++++++++------------------
1 file changed, 42 insertions(+), 37 deletions(-)
--
2.25.1
There is a good chance that while the channel ring gets processed, the STOP
or RESET command for the channel might be received from the MHI host. In
those cases, the entire channel ring processing needs to be protected by
chan->lock to prevent the race where the corresponding channel ring might
be reset.
While at it, let's also add a sanity check to make sure that the ring is
started before processing it. Because, if the STOP/RESET command gets
processed while mhi_ep_ch_ring_worker() waited for chan->lock, the ring
would've been reset.
Cc: <[email protected]> # 5.19
Fixes: 03c0bb8ec983 ("bus: mhi: ep: Add support for processing channel rings")
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/bus/mhi/ep/main.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
index 0bce6610ebf1..2362fcc8b32c 100644
--- a/drivers/bus/mhi/ep/main.c
+++ b/drivers/bus/mhi/ep/main.c
@@ -730,24 +730,37 @@ static void mhi_ep_ch_ring_worker(struct work_struct *work)
list_del(&itr->node);
ring = itr->ring;
+ chan = &mhi_cntrl->mhi_chan[ring->ch_id];
+ mutex_lock(&chan->lock);
+
+ /*
+ * The ring could've stopped while we waited to grab the (chan->lock), so do
+ * a sanity check before going further.
+ */
+ if (!ring->started) {
+ mutex_unlock(&chan->lock);
+ kfree(itr);
+ continue;
+ }
+
/* Update the write offset for the ring */
ret = mhi_ep_update_wr_offset(ring);
if (ret) {
dev_err(dev, "Error updating write offset for ring\n");
+ mutex_unlock(&chan->lock);
kfree(itr);
continue;
}
/* Sanity check to make sure there are elements in the ring */
if (ring->rd_offset == ring->wr_offset) {
+ mutex_unlock(&chan->lock);
kfree(itr);
continue;
}
el = &ring->ring_cache[ring->rd_offset];
- chan = &mhi_cntrl->mhi_chan[ring->ch_id];
- mutex_lock(&chan->lock);
dev_dbg(dev, "Processing the ring for channel (%u)\n", ring->ch_id);
ret = mhi_ep_process_ch_ring(ring, el);
if (ret) {
--
2.25.1
The debug log incorrectly mentions that STOP command is received instead of
RESET command. Fix that.
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/bus/mhi/ep/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
index 7d68b00bdbcf..0bce6610ebf1 100644
--- a/drivers/bus/mhi/ep/main.c
+++ b/drivers/bus/mhi/ep/main.c
@@ -226,7 +226,7 @@ static int mhi_ep_process_cmd_ring(struct mhi_ep_ring *ring, struct mhi_ring_ele
mutex_unlock(&mhi_chan->lock);
break;
case MHI_PKT_TYPE_RESET_CHAN_CMD:
- dev_dbg(dev, "Received STOP command for channel (%u)\n", ch_id);
+ dev_dbg(dev, "Received RESET command for channel (%u)\n", ch_id);
if (!ch_ring->started) {
dev_err(dev, "Channel (%u) not opened\n", ch_id);
return -ENODEV;
--
2.25.1
For the STOP and RESET commands, only send the channel disconnect status
-ENOTCONN if client driver is available. Otherwise, it will result in
null pointer dereference.
Cc: <[email protected]> # 5.19
Fixes: e827569062a8 ("bus: mhi: ep: Add support for processing command rings")
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/bus/mhi/ep/main.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
index 8b065a3cc848..7d68b00bdbcf 100644
--- a/drivers/bus/mhi/ep/main.c
+++ b/drivers/bus/mhi/ep/main.c
@@ -203,9 +203,11 @@ static int mhi_ep_process_cmd_ring(struct mhi_ep_ring *ring, struct mhi_ring_ele
mhi_ep_mmio_disable_chdb(mhi_cntrl, ch_id);
/* Send channel disconnect status to client drivers */
- result.transaction_status = -ENOTCONN;
- result.bytes_xferd = 0;
- mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result);
+ if (mhi_chan->xfer_cb) {
+ result.transaction_status = -ENOTCONN;
+ result.bytes_xferd = 0;
+ mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result);
+ }
/* Set channel state to STOP */
mhi_chan->state = MHI_CH_STATE_STOP;
@@ -235,9 +237,11 @@ static int mhi_ep_process_cmd_ring(struct mhi_ep_ring *ring, struct mhi_ring_ele
mhi_ep_ring_reset(mhi_cntrl, ch_ring);
/* Send channel disconnect status to client driver */
- result.transaction_status = -ENOTCONN;
- result.bytes_xferd = 0;
- mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result);
+ if (mhi_chan->xfer_cb) {
+ result.transaction_status = -ENOTCONN;
+ result.bytes_xferd = 0;
+ mhi_chan->xfer_cb(mhi_chan->mhi_dev, &result);
+ }
/* Set channel state to DISABLED */
mhi_chan->state = MHI_CH_STATE_DISABLED;
--
2.25.1
During graceful shutdown scenario, host will issue MHI RESET to the
endpoint device before initiating shutdown. In that case, it makes sense
to completely power down the MHI stack as sooner or later the access to
MMIO registers will be prohibited. Also, the stack needs to be powered
up in the case of SYS_ERR to recover the device.
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/bus/mhi/ep/main.c | 35 +++++++----------------------------
1 file changed, 7 insertions(+), 28 deletions(-)
diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
index 1dc8a3557a46..55209d42a995 100644
--- a/drivers/bus/mhi/ep/main.c
+++ b/drivers/bus/mhi/ep/main.c
@@ -973,11 +973,9 @@ static void mhi_ep_abort_transfer(struct mhi_ep_cntrl *mhi_cntrl)
static void mhi_ep_reset_worker(struct work_struct *work)
{
struct mhi_ep_cntrl *mhi_cntrl = container_of(work, struct mhi_ep_cntrl, reset_work);
- struct device *dev = &mhi_cntrl->mhi_dev->dev;
enum mhi_state cur_state;
- int ret;
- mhi_ep_abort_transfer(mhi_cntrl);
+ mhi_ep_power_down(mhi_cntrl);
spin_lock_bh(&mhi_cntrl->state_lock);
/* Reset MMIO to signal host that the MHI_RESET is completed in endpoint */
@@ -990,27 +988,8 @@ static void mhi_ep_reset_worker(struct work_struct *work)
* issue reset during shutdown also and we don't need to do re-init in
* that case.
*/
- if (cur_state == MHI_STATE_SYS_ERR) {
- mhi_ep_mmio_init(mhi_cntrl);
-
- /* Set AMSS EE before signaling ready state */
- mhi_ep_mmio_set_env(mhi_cntrl, MHI_EE_AMSS);
-
- /* All set, notify the host that we are ready */
- ret = mhi_ep_set_ready_state(mhi_cntrl);
- if (ret)
- return;
-
- dev_dbg(dev, "READY state notification sent to the host\n");
-
- ret = mhi_ep_enable(mhi_cntrl);
- if (ret) {
- dev_err(dev, "Failed to enable MHI endpoint: %d\n", ret);
- return;
- }
-
- enable_irq(mhi_cntrl->irq);
- }
+ if (cur_state == MHI_STATE_SYS_ERR)
+ mhi_ep_power_up(mhi_cntrl);
}
/*
@@ -1089,11 +1068,11 @@ EXPORT_SYMBOL_GPL(mhi_ep_power_up);
void mhi_ep_power_down(struct mhi_ep_cntrl *mhi_cntrl)
{
- if (mhi_cntrl->enabled)
+ if (mhi_cntrl->enabled) {
mhi_ep_abort_transfer(mhi_cntrl);
-
- kfree(mhi_cntrl->mhi_event);
- disable_irq(mhi_cntrl->irq);
+ kfree(mhi_cntrl->mhi_event);
+ disable_irq(mhi_cntrl->irq);
+ }
}
EXPORT_SYMBOL_GPL(mhi_ep_power_down);
--
2.25.1
On 12/28/2022 9:16 AM, Manivannan Sadhasivam wrote:
> During graceful shutdown scenario, host will issue MHI RESET to the
> endpoint device before initiating shutdown. In that case, it makes sense
> to completely power down the MHI stack as sooner or later the access to
> MMIO registers will be prohibited. Also, the stack needs to be powered
> up in the case of SYS_ERR to recover the device.
>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Jeffrey Hugo <[email protected]>
On 12/28/2022 9:17 AM, Manivannan Sadhasivam wrote:
> For the STOP and RESET commands, only send the channel disconnect status
> -ENOTCONN if client driver is available. Otherwise, it will result in
> null pointer dereference.
>
> Cc: <[email protected]> # 5.19
> Fixes: e827569062a8 ("bus: mhi: ep: Add support for processing command rings")
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Jeffrey Hugo <[email protected]>
On 12/28/2022 9:17 AM, Manivannan Sadhasivam wrote:
> The debug log incorrectly mentions that STOP command is received instead of
> RESET command. Fix that.
>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
Fixes?
Reviewed-by: Jeffrey Hugo <[email protected]>
On 12/28/2022 9:17 AM, Manivannan Sadhasivam wrote:
> There is a good chance that while the channel ring gets processed, the STOP
> or RESET command for the channel might be received from the MHI host. In
> those cases, the entire channel ring processing needs to be protected by
> chan->lock to prevent the race where the corresponding channel ring might
> be reset.
>
> While at it, let's also add a sanity check to make sure that the ring is
> started before processing it. Because, if the STOP/RESET command gets
> processed while mhi_ep_ch_ring_worker() waited for chan->lock, the ring
> would've been reset.
>
> Cc: <[email protected]> # 5.19
> Fixes: 03c0bb8ec983 ("bus: mhi: ep: Add support for processing channel rings")
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
Reviewed-by: Jeffrey Hugo <[email protected]>
On Wed, Dec 28, 2022 at 09:46:58PM +0530, Manivannan Sadhasivam wrote:
> This series fixes several issues on the MHI EP stack and also improves
> the suspend and resume functionality.
>
Applied to mhi-next!
Thanks,
Mani
> Thanks,
> Mani
>
> Manivannan Sadhasivam (6):
> bus: mhi: ep: Power up/down MHI stack during MHI RESET
> bus: mhi: ep: Check if the channel is supported by the controller
> bus: mhi: ep: Only send -ENOTCONN status if client driver is available
> bus: mhi: ep: Fix the debug message for MHI_PKT_TYPE_RESET_CHAN_CMD
> cmd
> bus: mhi: ep: Move chan->lock to the start of processing queued ch
> ring
> bus: mhi: ep: Save channel state locally during suspend and resume
>
> drivers/bus/mhi/ep/main.c | 79 +++++++++++++++++++++------------------
> 1 file changed, 42 insertions(+), 37 deletions(-)
>
> --
> 2.25.1
>
--
மணிவண்ணன் சதாசிவம்