netdev - Re: [PATCH net] net: ipa: disable ipa interrupt during suspend

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a0b67646-c30d-c697-d291-61af4c8ec473@ieee.org>
Date:   Tue, 17 Jan 2023 12:29:30 -0600
From:   Alex Elder <elder@...e.org>
To:     Caleb Connolly <caleb.connolly@...aro.org>,
        Alex Elder <elder@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Cc:     Alex Elder <elder@...aro.org>, netdev@...r.kernel.org
Subject: Re: [PATCH net] net: ipa: disable ipa interrupt during suspend

On 1/15/23 11:59 AM, Caleb Connolly wrote:
> The IPA interrupt can fire when pm_runtime is disabled due to it racing
> with the PM suspend/resume code. This causes a splat in the interrupt
> handler when it tries to call pm_runtime_get().
> 
> Explicitly disable the interrupt in our ->suspend callback, and
> re-enable it in ->resume to avoid this. If there is an interrupt pending
> it will be handled after resuming. The interrupt is a wake_irq, as a
> result even when disabled if it fires it will cause the system to wake
> from suspend as well as cancel any suspend transition that may be in
> progress. If there is an interrupt pending, the ipa_isr_thread handler
> will be called after resuming.

Looks good to me.  Thanks!

Reviewed-by: Alex Elder <elder@...aro.org>

> 
> Fixes: 1aac309d3207 ("net: ipa: use autosuspend")
> Signed-off-by: Caleb Connolly <caleb.connolly@...aro.org>
> ---
>   drivers/net/ipa/ipa_interrupt.c | 10 ++++++++++
>   drivers/net/ipa/ipa_interrupt.h | 16 ++++++++++++++++
>   drivers/net/ipa/ipa_power.c     | 17 +++++++++++++++++
>   3 files changed, 43 insertions(+)
> 
> diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
> index d458a35839cc..c1b3977e1ae4 100644
> --- a/drivers/net/ipa/ipa_interrupt.c
> +++ b/drivers/net/ipa/ipa_interrupt.c
> @@ -127,6 +127,16 @@ static irqreturn_t ipa_isr_thread(int irq, void *dev_id)
>   	return IRQ_HANDLED;
>   }
>   
> +void ipa_interrupt_irq_disable(struct ipa *ipa)
> +{
> +	disable_irq(ipa->interrupt->irq);
> +}
> +
> +void ipa_interrupt_irq_enable(struct ipa *ipa)
> +{
> +	enable_irq(ipa->interrupt->irq);
> +}
> +
>   /* Common function used to enable/disable TX_SUSPEND for an endpoint */
>   static void ipa_interrupt_suspend_control(struct ipa_interrupt *interrupt,
>   					  u32 endpoint_id, bool enable)
> diff --git a/drivers/net/ipa/ipa_interrupt.h b/drivers/net/ipa/ipa_interrupt.h
> index f31fd9965fdc..8a1bd5b89393 100644
> --- a/drivers/net/ipa/ipa_interrupt.h
> +++ b/drivers/net/ipa/ipa_interrupt.h
> @@ -85,6 +85,22 @@ void ipa_interrupt_suspend_clear_all(struct ipa_interrupt *interrupt);
>    */
>   void ipa_interrupt_simulate_suspend(struct ipa_interrupt *interrupt);
>   
> +/**
> + * ipa_interrupt_irq_enable() - Enable IPA interrupts
> + * @ipa:	IPA pointer
> + *
> + * This enables the IPA interrupt line
> + */
> +void ipa_interrupt_irq_enable(struct ipa *ipa);
> +
> +/**
> + * ipa_interrupt_irq_disable() - Disable IPA interrupts
> + * @ipa:	IPA pointer
> + *
> + * This disables the IPA interrupt line
> + */
> +void ipa_interrupt_irq_disable(struct ipa *ipa);
> +
>   /**
>    * ipa_interrupt_config() - Configure the IPA interrupt framework
>    * @ipa:	IPA pointer
> diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
> index 8420f93128a2..8057be8cda80 100644
> --- a/drivers/net/ipa/ipa_power.c
> +++ b/drivers/net/ipa/ipa_power.c
> @@ -181,6 +181,17 @@ static int ipa_suspend(struct device *dev)
>   
>   	__set_bit(IPA_POWER_FLAG_SYSTEM, ipa->power->flags);
>   
> +	/* Increment the disable depth to ensure that the IRQ won't
> +	 * be re-enabled until the matching _enable call in
> +	 * ipa_resume(). We do this to ensure that the interrupt
> +	 * handler won't run whilst PM runtime is disabled.
> +	 *
> +	 * Note that disabling the IRQ is NOT the same as disabling
> +	 * irq wake. If wakeup is enabled for the IPA then the IRQ
> +	 * will still cause the system to wake up, see irq_set_irq_wake().
> +	 */
> +	ipa_interrupt_irq_disable(ipa);
> +
>   	return pm_runtime_force_suspend(dev);
>   }
>   
> @@ -193,6 +204,12 @@ static int ipa_resume(struct device *dev)
>   
>   	__clear_bit(IPA_POWER_FLAG_SYSTEM, ipa->power->flags);
>   
> +	/* Now that PM runtime is enabled again it's safe
> +	 * to turn the IRQ back on and process any data
> +	 * that was received during suspend.
> +	 */
> +	ipa_interrupt_irq_enable(ipa);
> +
>   	return ret;
>   }
>