[PATCH net 0/2] net: ipa: prevent shutdown during setup

[PATCH net 0/2] net: ipa: prevent shutdown during setup

[Alex Elder]

The setup phase of the IPA driver occurs in one of two ways.
Normally, it is done directly by the main driver probe function.
But some systems (those having a "modem-init" DTS property) don't
start setup until an SMP2P interrupt (sent by the modem) arrives.

Because it isn't performed by the probe function, setup on
"modem-init" systems could be underway at the time a driver
remove (or shutdown) request arrives (or vice-versa).  This
situation can lead to hardware state not being cleaned up
properly.

This series addresses this problem by having the driver remove
function disable the setup interrupt.  A consequence of this is
that setup will complete if it is underway when the remove function
is called.

So now, when removing the driver, setup:
  - will have already completed;
  - is underway, and will complete before proceeding; or
  - will not have begun (and will not occur).

					-Alex

PS  These patches might not back-port cleanly; I'll gladly provide
    equivalent code for older kernel releases if needed.

Alex Elder (2):
  net: ipa: directly disable ipa-setup-ready interrupt
  net: ipa: separate disabling setup from modem stop

 drivers/net/ipa/ipa_main.c  |  6 ++++++
 drivers/net/ipa/ipa_modem.c |  6 +++---
 drivers/net/ipa/ipa_smp2p.c | 21 ++++++++++-----------
 drivers/net/ipa/ipa_smp2p.h |  7 +++----
 4 files changed, 22 insertions(+), 18 deletions(-)

-- 
2.32.0

[PATCH net 1/2] net: ipa: directly disable ipa-setup-ready interrupt

[Alex Elder]

We currently maintain a "disabled" Boolean flag to determine whether
the "ipa-setup-ready" SMP2P IRQ handler does anything.  That flag
must be accessed under protection of a mutex.

Instead, disable the SMP2P interrupt when requested, which prevents
the interrupt handler from ever being called.  More importantly, it
synchronizes a thread disabling the interrupt with the completion of
the interrupt handler in case they run concurrently.

Use the IPA setup_complete flag rather than the disabled flag in the
handler to determine whether to ignore any interrupts arriving after
the first.

Rename the "disabled" flag to be "setup_disabled", to be specific
about its purpose.

Fixes: 530f9216a953 ("soc: qcom: ipa: AP/modem communications")
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_smp2p.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ipa/ipa_smp2p.c b/drivers/net/ipa/ipa_smp2p.c
index df7639c39d716..24bc112a072c6 100644
--- a/drivers/net/ipa/ipa_smp2p.c
+++ b/drivers/net/ipa/ipa_smp2p.c
@@ -53,7 +53,7 @@
  * @setup_ready_irq:	IPA interrupt triggered by modem to signal GSI ready
  * @power_on:		Whether IPA power is on
  * @notified:		Whether modem has been notified of power state
- * @disabled:		Whether setup ready interrupt handling is disabled
+ * @setup_disabled:	Whether setup ready interrupt handler is disabled
  * @mutex:		Mutex protecting ready-interrupt/shutdown interlock
  * @panic_notifier:	Panic notifier structure
 */
@@ -67,7 +67,7 @@ struct ipa_smp2p {
 	u32 setup_ready_irq;
 	bool power_on;
 	bool notified;
-	bool disabled;
+	bool setup_disabled;
 	struct mutex mutex;
 	struct notifier_block panic_notifier;
 };
@@ -155,11 +155,9 @@ static irqreturn_t ipa_smp2p_modem_setup_ready_isr(int irq, void *dev_id)
 	struct device *dev;
 	int ret;
 
-	mutex_lock(&smp2p->mutex);
-
-	if (smp2p->disabled)
-		goto out_mutex_unlock;
-	smp2p->disabled = true;		/* If any others arrive, ignore them */
+	/* Ignore any (spurious) interrupts received after the first */
+	if (smp2p->ipa->setup_complete)
+		return IRQ_HANDLED;
 
 	/* Power needs to be active for setup */
 	dev = &smp2p->ipa->pdev->dev;
@@ -176,8 +174,6 @@ static irqreturn_t ipa_smp2p_modem_setup_ready_isr(int irq, void *dev_id)
 out_power_put:
 	pm_runtime_mark_last_busy(dev);
 	(void)pm_runtime_put_autosuspend(dev);
-out_mutex_unlock:
-	mutex_unlock(&smp2p->mutex);
 
 	return IRQ_HANDLED;
 }
@@ -322,7 +318,10 @@ void ipa_smp2p_disable(struct ipa *ipa)
 
 	mutex_lock(&smp2p->mutex);
 
-	smp2p->disabled = true;
+	if (!smp2p->setup_disabled) {
+		disable_irq(smp2p->setup_ready_irq);
+		smp2p->setup_disabled = true;
+	}
 
 	mutex_unlock(&smp2p->mutex);
 }
-- 
2.32.0

[PATCH net 2/2] net: ipa: separate disabling setup from modem stop

[Alex Elder]

The IPA setup_complete flag is set at the end of ipa_setup(), when
the setup phase of initialization has completed successfully.  This
occurs as part of driver probe processing, or (if "modem-init" is
specified in the DTS file) it is triggered by the "ipa-setup-ready"
SMP2P interrupt generated by the modem.

In the latter case, it's possible for driver shutdown (or remove) to
begin while setup processing is underway, and this can't be allowed.
The problem is that the setup_complete flag is not adequate to signal
that setup is underway.

If setup_complete is set, it will never be un-set, so that case is
not a problem.  But if setup_complete is false, there's a chance
setup is underway.

Because setup is triggered by an interrupt on a "modem-init" system,
there is a simple way to ensure the value of setup_complete is safe
to read.  The threaded handler--if it is executing--will complete as
part of a request to disable the "ipa-modem-ready" interrupt.  This
means that ipa_setup() (which is called from the handler) will run
to completion if it was underway, or will never be called otherwise.

The request to disable the "ipa-setup-ready" interrupt is currently
made within ipa_modem_stop().  Instead, disable the interrupt
outside that function in the two places it's called.  In the case of
ipa_remove(), this ensures the setup_complete flag is safe to read
before we read it.

Rename ipa_smp2p_disable() to be ipa_smp2p_irq_disable_setup(), to be
more specific about its effect.

Fixes: 530f9216a953 ("soc: qcom: ipa: AP/modem communications")
Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_main.c  | 6 ++++++
 drivers/net/ipa/ipa_modem.c | 6 +++---
 drivers/net/ipa/ipa_smp2p.c | 2 +-
 drivers/net/ipa/ipa_smp2p.h | 7 +++----
 4 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index cdfa98a76e1f4..a448ec198bee1 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -28,6 +28,7 @@
 #include "ipa_reg.h"
 #include "ipa_mem.h"
 #include "ipa_table.h"
+#include "ipa_smp2p.h"
 #include "ipa_modem.h"
 #include "ipa_uc.h"
 #include "ipa_interrupt.h"
@@ -801,6 +802,11 @@ static int ipa_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int ret;
 
+	/* Prevent the modem from triggering a call to ipa_setup().  This
+	 * also ensures a modem-initiated setup that's underway completes.
+	 */
+	ipa_smp2p_irq_disable_setup(ipa);
+
 	ret = pm_runtime_get_sync(dev);
 	if (WARN_ON(ret < 0))
 		goto out_power_put;
diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c
index ad116bcc0580e..d0ab4d70c303b 100644
--- a/drivers/net/ipa/ipa_modem.c
+++ b/drivers/net/ipa/ipa_modem.c
@@ -339,9 +339,6 @@ int ipa_modem_stop(struct ipa *ipa)
 	if (state != IPA_MODEM_STATE_RUNNING)
 		return -EBUSY;
 
-	/* Prevent the modem from triggering a call to ipa_setup() */
-	ipa_smp2p_disable(ipa);
-
 	/* Clean up the netdev and endpoints if it was started */
 	if (netdev) {
 		struct ipa_priv *priv = netdev_priv(netdev);
@@ -369,6 +366,9 @@ static void ipa_modem_crashed(struct ipa *ipa)
 	struct device *dev = &ipa->pdev->dev;
 	int ret;
 
+	/* Prevent the modem from triggering a call to ipa_setup() */
+	ipa_smp2p_irq_disable_setup(ipa);
+
 	ret = pm_runtime_get_sync(dev);
 	if (ret < 0) {
 		dev_err(dev, "error %d getting power to handle crash\n", ret);
diff --git a/drivers/net/ipa/ipa_smp2p.c b/drivers/net/ipa/ipa_smp2p.c
index 24bc112a072c6..2112336120391 100644
--- a/drivers/net/ipa/ipa_smp2p.c
+++ b/drivers/net/ipa/ipa_smp2p.c
@@ -309,7 +309,7 @@ void ipa_smp2p_exit(struct ipa *ipa)
 	kfree(smp2p);
 }
 
-void ipa_smp2p_disable(struct ipa *ipa)
+void ipa_smp2p_irq_disable_setup(struct ipa *ipa)
 {
 	struct ipa_smp2p *smp2p = ipa->smp2p;
 
diff --git a/drivers/net/ipa/ipa_smp2p.h b/drivers/net/ipa/ipa_smp2p.h
index 99a9567896388..59cee31a73836 100644
--- a/drivers/net/ipa/ipa_smp2p.h
+++ b/drivers/net/ipa/ipa_smp2p.h
@@ -27,13 +27,12 @@ int ipa_smp2p_init(struct ipa *ipa, bool modem_init);
 void ipa_smp2p_exit(struct ipa *ipa);
 
 /**
- * ipa_smp2p_disable() - Prevent "ipa-setup-ready" interrupt handling
+ * ipa_smp2p_irq_disable_setup() - Disable the "setup ready" interrupt
  * @ipa:	IPA pointer
  *
- * Prevent handling of the "setup ready" interrupt from the modem.
- * This is used before initiating shutdown of the driver.
+ * Disable the "ipa-setup-ready" interrupt from the modem.
  */
-void ipa_smp2p_disable(struct ipa *ipa);
+void ipa_smp2p_irq_disable_setup(struct ipa *ipa);
 
 /**
  * ipa_smp2p_notify_reset() - Reset modem notification state
-- 
2.32.0

Re: [PATCH net 0/2] net: ipa: prevent shutdown during setup

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Mon, 22 Nov 2021 18:15:53 -0600 you wrote:
> The setup phase of the IPA driver occurs in one of two ways.
> Normally, it is done directly by the main driver probe function.
> But some systems (those having a "modem-init" DTS property) don't
> start setup until an SMP2P interrupt (sent by the modem) arrives.
> 
> Because it isn't performed by the probe function, setup on
> "modem-init" systems could be underway at the time a driver
> remove (or shutdown) request arrives (or vice-versa).  This
> situation can lead to hardware state not being cleaned up
> properly.
> 
> [...]

Here is the summary with links:
  - [net,1/2] net: ipa: directly disable ipa-setup-ready interrupt
    https://git.kernel.org/netdev/net/c/33a153100bb3
  - [net,2/2] net: ipa: separate disabling setup from modem stop
    https://git.kernel.org/netdev/net/c/8afc7e471ad3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net 1/2] net: ipa: directly disable ipa-setup-ready interrupt

[Matthias Kaehlcke]

On Mon, Nov 22, 2021 at 06:15:54PM -0600, Alex Elder wrote:
> We currently maintain a "disabled" Boolean flag to determine whether
> the "ipa-setup-ready" SMP2P IRQ handler does anything.  That flag
> must be accessed under protection of a mutex.
> 
> Instead, disable the SMP2P interrupt when requested, which prevents
> the interrupt handler from ever being called.  More importantly, it
> synchronizes a thread disabling the interrupt with the completion of
> the interrupt handler in case they run concurrently.
> 
> Use the IPA setup_complete flag rather than the disabled flag in the
> handler to determine whether to ignore any interrupts arriving after
> the first.
> 
> Rename the "disabled" flag to be "setup_disabled", to be specific
> about its purpose.
> 
> Fixes: 530f9216a953 ("soc: qcom: ipa: AP/modem communications")
> Signed-off-by: Alex Elder <elder@linaro.org>

I don't claim to know much about IPA, but this looks reasonable to me.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>