From f8d3bdd561a7c95269a8f908d82249e41badc2aa Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Thu, 19 Nov 2020 16:49:24 -0600 Subject: [PATCH 1/6] net: ipa: print channel/event ring number on error When a GSI command is used to change the state of a channel or event ring we check the state before and after the command to ensure it is as expected. If not, we print an error message, but it does not include the channel or event ring id, and it easily can. Add the channel or event ring id to these error messages. Signed-off-by: Alex Elder Signed-off-by: Jakub Kicinski --- drivers/net/ipa/gsi.c | 54 +++++++++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index 55151960a6985..2bc513c663396 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -365,15 +365,15 @@ static int gsi_evt_ring_alloc_command(struct gsi *gsi, u32 evt_ring_id) /* Get initial event ring state */ evt_ring->state = gsi_evt_ring_state(gsi, evt_ring_id); if (evt_ring->state != GSI_EVT_RING_STATE_NOT_ALLOCATED) { - dev_err(gsi->dev, "bad event ring state %u before alloc\n", - evt_ring->state); + dev_err(gsi->dev, "event ring %u bad state %u before alloc\n", + evt_ring_id, evt_ring->state); return -EINVAL; } ret = evt_ring_command(gsi, evt_ring_id, GSI_EVT_ALLOCATE); if (!ret && evt_ring->state != GSI_EVT_RING_STATE_ALLOCATED) { - dev_err(gsi->dev, "bad event ring state %u after alloc\n", - evt_ring->state); + dev_err(gsi->dev, "event ring %u bad state %u after alloc\n", + evt_ring_id, evt_ring->state); ret = -EIO; } @@ -389,15 +389,15 @@ static void gsi_evt_ring_reset_command(struct gsi *gsi, u32 evt_ring_id) if (state != GSI_EVT_RING_STATE_ALLOCATED && state != GSI_EVT_RING_STATE_ERROR) { - dev_err(gsi->dev, "bad event ring state %u before reset\n", - evt_ring->state); + dev_err(gsi->dev, "event ring %u bad state %u before reset\n", + evt_ring_id, evt_ring->state); return; } ret = evt_ring_command(gsi, evt_ring_id, GSI_EVT_RESET); if (!ret && evt_ring->state != GSI_EVT_RING_STATE_ALLOCATED) - dev_err(gsi->dev, "bad event ring state %u after reset\n", - evt_ring->state); + dev_err(gsi->dev, "event ring %u bad state %u after reset\n", + evt_ring_id, evt_ring->state); } /* Issue a hardware de-allocation request for an allocated event ring */ @@ -407,15 +407,15 @@ static void gsi_evt_ring_de_alloc_command(struct gsi *gsi, u32 evt_ring_id) int ret; if (evt_ring->state != GSI_EVT_RING_STATE_ALLOCATED) { - dev_err(gsi->dev, "bad event ring state %u before dealloc\n", - evt_ring->state); + dev_err(gsi->dev, "event ring %u state %u before dealloc\n", + evt_ring_id, evt_ring->state); return; } ret = evt_ring_command(gsi, evt_ring_id, GSI_EVT_DE_ALLOC); if (!ret && evt_ring->state != GSI_EVT_RING_STATE_NOT_ALLOCATED) - dev_err(gsi->dev, "bad event ring state %u after dealloc\n", - evt_ring->state); + dev_err(gsi->dev, "event ring %u bad state %u after dealloc\n", + evt_ring_id, evt_ring->state); } /* Fetch the current state of a channel from hardware */ @@ -479,7 +479,8 @@ static int gsi_channel_alloc_command(struct gsi *gsi, u32 channel_id) /* Get initial channel state */ state = gsi_channel_state(channel); if (state != GSI_CHANNEL_STATE_NOT_ALLOCATED) { - dev_err(dev, "bad channel state %u before alloc\n", state); + dev_err(dev, "channel %u bad state %u before alloc\n", + channel_id, state); return -EINVAL; } @@ -488,7 +489,8 @@ static int gsi_channel_alloc_command(struct gsi *gsi, u32 channel_id) /* Channel state will normally have been updated */ state = gsi_channel_state(channel); if (!ret && state != GSI_CHANNEL_STATE_ALLOCATED) { - dev_err(dev, "bad channel state %u after alloc\n", state); + dev_err(dev, "channel %u bad state %u after alloc\n", + channel_id, state); ret = -EIO; } @@ -505,7 +507,8 @@ static int gsi_channel_start_command(struct gsi_channel *channel) state = gsi_channel_state(channel); if (state != GSI_CHANNEL_STATE_ALLOCATED && state != GSI_CHANNEL_STATE_STOPPED) { - dev_err(dev, "bad channel state %u before start\n", state); + dev_err(dev, "channel %u bad state %u before start\n", + gsi_channel_id(channel), state); return -EINVAL; } @@ -514,7 +517,8 @@ static int gsi_channel_start_command(struct gsi_channel *channel) /* Channel state will normally have been updated */ state = gsi_channel_state(channel); if (!ret && state != GSI_CHANNEL_STATE_STARTED) { - dev_err(dev, "bad channel state %u after start\n", state); + dev_err(dev, "channel %u bad state %u after start\n", + gsi_channel_id(channel), state); ret = -EIO; } @@ -538,7 +542,8 @@ static int gsi_channel_stop_command(struct gsi_channel *channel) if (state != GSI_CHANNEL_STATE_STARTED && state != GSI_CHANNEL_STATE_STOP_IN_PROC) { - dev_err(dev, "bad channel state %u before stop\n", state); + dev_err(dev, "channel %u bad state %u before stop\n", + gsi_channel_id(channel), state); return -EINVAL; } @@ -553,7 +558,8 @@ static int gsi_channel_stop_command(struct gsi_channel *channel) if (state == GSI_CHANNEL_STATE_STOP_IN_PROC) return -EAGAIN; - dev_err(dev, "bad channel state %u after stop\n", state); + dev_err(dev, "channel %u bad state %u after stop\n", + gsi_channel_id(channel), state); return -EIO; } @@ -570,7 +576,8 @@ static void gsi_channel_reset_command(struct gsi_channel *channel) state = gsi_channel_state(channel); if (state != GSI_CHANNEL_STATE_STOPPED && state != GSI_CHANNEL_STATE_ERROR) { - dev_err(dev, "bad channel state %u before reset\n", state); + dev_err(dev, "channel %u bad state %u before reset\n", + gsi_channel_id(channel), state); return; } @@ -579,7 +586,8 @@ static void gsi_channel_reset_command(struct gsi_channel *channel) /* Channel state will normally have been updated */ state = gsi_channel_state(channel); if (!ret && state != GSI_CHANNEL_STATE_ALLOCATED) - dev_err(dev, "bad channel state %u after reset\n", state); + dev_err(dev, "channel %u bad state %u after reset\n", + gsi_channel_id(channel), state); } /* Deallocate an ALLOCATED GSI channel */ @@ -592,7 +600,8 @@ static void gsi_channel_de_alloc_command(struct gsi *gsi, u32 channel_id) state = gsi_channel_state(channel); if (state != GSI_CHANNEL_STATE_ALLOCATED) { - dev_err(dev, "bad channel state %u before dealloc\n", state); + dev_err(dev, "channel %u bad state %u before dealloc\n", + channel_id, state); return; } @@ -601,7 +610,8 @@ static void gsi_channel_de_alloc_command(struct gsi *gsi, u32 channel_id) /* Channel state will normally have been updated */ state = gsi_channel_state(channel); if (!ret && state != GSI_CHANNEL_STATE_NOT_ALLOCATED) - dev_err(dev, "bad channel state %u after dealloc\n", state); + dev_err(dev, "channel %u bad state %u after dealloc\n", + channel_id, state); } /* Ring an event ring doorbell, reporting the last entry processed by the AP. From 5d28913d4ee655cf15756ba67d4075cac9204a90 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Thu, 19 Nov 2020 16:49:25 -0600 Subject: [PATCH 2/6] net: ipa: don't reset an ALLOCATED channel If the rmnet_ipa0 network device has not been opened at the time we remove or shut down the IPA driver, its underlying TX and RX GSI channels will not have been started, and they will still be in ALLOCATED state. The RESET command on a channel is meant to return a channel to ALLOCATED state after it's been stopped. But if it was never started, its state will still be ALLOCATED, the RESET command is not required. Quietly skip doing the reset without printing an error message if a channel is already in ALLOCATED state when we request it be reset. Signed-off-by: Alex Elder Signed-off-by: Jakub Kicinski --- drivers/net/ipa/gsi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index 2bc513c663396..58bec70db5ab4 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -576,8 +576,10 @@ static void gsi_channel_reset_command(struct gsi_channel *channel) state = gsi_channel_state(channel); if (state != GSI_CHANNEL_STATE_STOPPED && state != GSI_CHANNEL_STATE_ERROR) { - dev_err(dev, "channel %u bad state %u before reset\n", - gsi_channel_id(channel), state); + /* No need to reset a channel already in ALLOCATED state */ + if (state != GSI_CHANNEL_STATE_ALLOCATED) + dev_err(dev, "channel %u bad state %u before reset\n", + gsi_channel_id(channel), state); return; } From f849afcc8c3b27d7b50827e95b60557f24184df0 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Thu, 19 Nov 2020 16:49:26 -0600 Subject: [PATCH 3/6] net: ipa: ignore CHANNEL_NOT_RUNNING errors IPA v4.2 has a hardware quirk that requires the AP to allocate GSI channels for the modem to use. It is recommended that these modem channels get stopped (with a HALT generic command) by the AP when its IPA driver gets removed. The AP has no way of knowing the current state of a modem channel. So when the IPA driver issues a HALT command it's possible the channel is not running, and in that case we get an error indication. This error simply means we didn't need to stop the channel, so we can ignore it. This patch adds an explanation for this situation, and arranges for this condition to *not* report an error message. Signed-off-by: Alex Elder Signed-off-by: Jakub Kicinski --- drivers/net/ipa/gsi.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index 58bec70db5ab4..7c2e820625590 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -1087,10 +1087,32 @@ static void gsi_isr_gp_int1(struct gsi *gsi) u32 result; u32 val; + /* This interrupt is used to handle completions of the two GENERIC + * GSI commands. We use these to allocate and halt channels on + * the modem's behalf due to a hardware quirk on IPA v4.2. Once + * allocated, the modem "owns" these channels, and as a result we + * have no way of knowing the channel's state at any given time. + * + * It is recommended that we halt the modem channels we allocated + * when shutting down, but it's possible the channel isn't running + * at the time we issue the HALT command. We'll get an error in + * that case, but it's harmless (the channel is already halted). + * + * For this reason, we silently ignore a CHANNEL_NOT_RUNNING error + * if we receive it. + */ val = ioread32(gsi->virt + GSI_CNTXT_SCRATCH_0_OFFSET); result = u32_get_bits(val, GENERIC_EE_RESULT_FMASK); - if (result != GENERIC_EE_SUCCESS) + + switch (result) { + case GENERIC_EE_SUCCESS: + case GENERIC_EE_CHANNEL_NOT_RUNNING: + break; + + default: dev_err(gsi->dev, "global INT1 generic result %u\n", result); + break; + } complete(&gsi->completion); } From 1136145660f3116cb92794c1a7571bf49e4a1938 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Thu, 19 Nov 2020 16:49:27 -0600 Subject: [PATCH 4/6] net: ipa: support retries on generic GSI commands When stopping an AP RX channel, there can be a transient period while the channel enters STOP_IN_PROC state before reaching the final STOPPED state. In that case we make another attempt to stop the channel. Similarly, when stopping a modem channel (using a GSI generic command issued from the AP), it's possible that multiple attempts will be required before the channel reaches STOPPED state. Add a field to the GSI structure to record an errno representing the result code provided when a generic command completes. If the result learned in gsi_isr_gp_int1() is RETRY, record -EAGAIN in the result code, otherwise record 0 for success, or -EIO for any other result. If we time out nf gsi_generic_command() waiting for the command to complete, return -ETIMEDOUT (as before). Otherwise return the result stashed by gsi_isr_gp_int1(). Add a loop in gsi_modem_channel_halt() to reissue the HALT command if the result code indicates -EAGAIN. Limit this to 10 retries (after the initial attempt). Signed-off-by: Alex Elder Signed-off-by: Jakub Kicinski --- drivers/net/ipa/gsi.c | 21 +++++++++++++++++++-- drivers/net/ipa/gsi.h | 1 + 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index 7c2e820625590..eb4c5d408a835 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -92,6 +92,7 @@ #define GSI_CMD_TIMEOUT 5 /* seconds */ #define GSI_CHANNEL_STOP_RX_RETRIES 10 +#define GSI_CHANNEL_MODEM_HALT_RETRIES 10 #define GSI_MHI_EVENT_ID_START 10 /* 1st reserved event id */ #define GSI_MHI_EVENT_ID_END 16 /* Last reserved event id */ @@ -1107,10 +1108,16 @@ static void gsi_isr_gp_int1(struct gsi *gsi) switch (result) { case GENERIC_EE_SUCCESS: case GENERIC_EE_CHANNEL_NOT_RUNNING: + gsi->result = 0; + break; + + case GENERIC_EE_RETRY: + gsi->result = -EAGAIN; break; default: dev_err(gsi->dev, "global INT1 generic result %u\n", result); + gsi->result = -EIO; break; } @@ -1624,7 +1631,7 @@ static int gsi_generic_command(struct gsi *gsi, u32 channel_id, iowrite32(BIT(ERROR_INT), gsi->virt + GSI_CNTXT_GLOB_IRQ_EN_OFFSET); if (success) - return 0; + return gsi->result; dev_err(gsi->dev, "GSI generic command %u to channel %u timed out\n", opcode, channel_id); @@ -1640,7 +1647,17 @@ static int gsi_modem_channel_alloc(struct gsi *gsi, u32 channel_id) static void gsi_modem_channel_halt(struct gsi *gsi, u32 channel_id) { - (void)gsi_generic_command(gsi, channel_id, GSI_GENERIC_HALT_CHANNEL); + u32 retries = GSI_CHANNEL_MODEM_HALT_RETRIES; + int ret; + + do + ret = gsi_generic_command(gsi, channel_id, + GSI_GENERIC_HALT_CHANNEL); + while (ret == -EAGAIN && retries--); + + if (ret) + dev_err(gsi->dev, "error %d halting modem channel %u\n", + ret, channel_id); } /* Setup function for channels */ diff --git a/drivers/net/ipa/gsi.h b/drivers/net/ipa/gsi.h index ecc784e3a8127..96c9aed397aad 100644 --- a/drivers/net/ipa/gsi.h +++ b/drivers/net/ipa/gsi.h @@ -161,6 +161,7 @@ struct gsi { u32 type_enabled_bitmap; /* GSI IRQ types enabled */ u32 ieob_enabled_bitmap; /* IEOB IRQ enabled (event rings) */ struct completion completion; /* for global EE commands */ + int result; /* Negative errno (generic commands) */ struct mutex mutex; /* protects commands, programming */ }; From 7c80e83829dbc86c1e564017867a48805e459de0 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Thu, 19 Nov 2020 16:49:28 -0600 Subject: [PATCH 5/6] net: ipa: retry modem stop if busy The IPA driver remove callback, ipa_remove(), calls ipa_modem_stop() if the setup stage of initialization is complete. If a concurrent call to ipa_modem_start() or ipa_modem_stop() has begin but not completed, ipa_modem_stop() can return an error (-EBUSY). The next patch adds a driver shutdown callback, which will simply call ipa_remove(). We really want our shutdown callback to clean things up. So add a single retry to the ipa_modem_stop() call in ipa_remove() after a short (millisecond) delay. This offers no guarantee the shutdown will complete successfully, but we'll at least try a little harder before giving up. Signed-off-by: Alex Elder Signed-off-by: Jakub Kicinski --- drivers/net/ipa/ipa_main.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c index 468ab1acc20e0..4f4a7ca01d450 100644 --- a/drivers/net/ipa/ipa_main.c +++ b/drivers/net/ipa/ipa_main.c @@ -863,6 +863,11 @@ static int ipa_remove(struct platform_device *pdev) if (ipa->setup_complete) { ret = ipa_modem_stop(ipa); + /* If starting or stopping is in progress, try once more */ + if (ret == -EBUSY) { + usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC); + ret = ipa_modem_stop(ipa); + } if (ret) return ret; From ae1d72f9779fbd05808517c030a2dae327cb038d Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Thu, 19 Nov 2020 16:49:29 -0600 Subject: [PATCH 6/6] net: ipa: add driver shutdown callback A system shutdown can happen at essentially any time, and it's possible that the IPA driver is busy when a shutdown is underway. IPA hardware accesses IMEM and SMEM memory regions using an IOMMU, and at some point during shutdown, needed I/O mappings could become invalid. This could be disastrous for any "in flight" IPA activity. Avoid this by defining a new driver shutdown callback that stops all IPA activity and cleanly shuts down the driver. It merely calls the driver's existing remove callback, reporting the error if it returns one. Signed-off-by: Alex Elder Signed-off-by: Jakub Kicinski --- drivers/net/ipa/ipa_main.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c index 4f4a7ca01d450..e9bd0d72f2db1 100644 --- a/drivers/net/ipa/ipa_main.c +++ b/drivers/net/ipa/ipa_main.c @@ -888,6 +888,15 @@ static int ipa_remove(struct platform_device *pdev) return 0; } +static void ipa_shutdown(struct platform_device *pdev) +{ + int ret; + + ret = ipa_remove(pdev); + if (ret) + dev_err(&pdev->dev, "shutdown: remove returned %d\n", ret); +} + /** * ipa_suspend() - Power management system suspend callback * @dev: IPA device structure @@ -945,8 +954,9 @@ static const struct dev_pm_ops ipa_pm_ops = { }; static struct platform_driver ipa_driver = { - .probe = ipa_probe, - .remove = ipa_remove, + .probe = ipa_probe, + .remove = ipa_remove, + .shutdown = ipa_shutdown, .driver = { .name = "ipa", .pm = &ipa_pm_ops,