linux-kernel - Re: [PATCH net-next v2 4/7] net: ipa: manage endpoints separate from clock

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: <20200913023025.GN3715@yoga>
Date:   Sat, 12 Sep 2020 21:30:25 -0500
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Alex Elder <elder@...aro.org>
Cc:     davem@...emloft.net, kuba@...nel.org, evgreen@...omium.org,
        subashab@...eaurora.org, cpratapa@...eaurora.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 4/7] net: ipa: manage endpoints separate from
 clock

On Fri 11 Sep 19:45 CDT 2020, Alex Elder wrote:

> Currently, when (before) the last IPA clock reference is dropped,
> all endpoints are suspended.  And whenever the first IPA clock
> reference is taken, all endpoints are resumed (or started).
> 
> In most cases there's no need to start endpoints when the clock
> starts.  So move the calls to ipa_endpoint_suspend() and
> ipa_endpoint_resume() out of ipa_clock_put() and ipa_clock_get(),
> respectiely.  Instead, only suspend endpoints when handling a system
> suspend, and only resume endpoints when handling a system resume.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@...aro.org>

Regards,
Bjorn

> Signed-off-by: Alex Elder <elder@...aro.org>
> ---
>  drivers/net/ipa/ipa_clock.c | 14 ++++----------
>  drivers/net/ipa/ipa_main.c  |  8 ++++++++
>  2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
> index b703866f2e20b..a2c0fde058199 100644
> --- a/drivers/net/ipa/ipa_clock.c
> +++ b/drivers/net/ipa/ipa_clock.c
> @@ -200,9 +200,8 @@ bool ipa_clock_get_additional(struct ipa *ipa)
>  
>  /* Get an IPA clock reference.  If the reference count is non-zero, it is
>   * incremented and return is immediate.  Otherwise it is checked again
> - * under protection of the mutex, and if appropriate the clock (and
> - * interconnects) are enabled suspended endpoints (if any) are resumed
> - * before returning.
> + * under protection of the mutex, and if appropriate the IPA clock
> + * is enabled.
>   *
>   * Incrementing the reference count is intentionally deferred until
>   * after the clock is running and endpoints are resumed.
> @@ -229,17 +228,14 @@ void ipa_clock_get(struct ipa *ipa)
>  		goto out_mutex_unlock;
>  	}
>  
> -	ipa_endpoint_resume(ipa);
> -
>  	refcount_set(&clock->count, 1);
>  
>  out_mutex_unlock:
>  	mutex_unlock(&clock->mutex);
>  }
>  
> -/* Attempt to remove an IPA clock reference.  If this represents the last
> - * reference, suspend endpoints and disable the clock (and interconnects)
> - * under protection of a mutex.
> +/* Attempt to remove an IPA clock reference.  If this represents the
> + * last reference, disable the IPA clock under protection of the mutex.
>   */
>  void ipa_clock_put(struct ipa *ipa)
>  {
> @@ -249,8 +245,6 @@ void ipa_clock_put(struct ipa *ipa)
>  	if (!refcount_dec_and_mutex_lock(&clock->count, &clock->mutex))
>  		return;
>  
> -	ipa_endpoint_suspend(ipa);
> -
>  	ipa_clock_disable(ipa);
>  
>  	mutex_unlock(&clock->mutex);
> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index cfdf60ded86ca..3b68b53c99015 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
> @@ -913,11 +913,15 @@ static int ipa_remove(struct platform_device *pdev)
>   * Return:	Always returns zero
>   *
>   * Called by the PM framework when a system suspend operation is invoked.
> + * Suspends endpoints and releases the clock reference held to keep
> + * the IPA clock running until this point.
>   */
>  static int ipa_suspend(struct device *dev)
>  {
>  	struct ipa *ipa = dev_get_drvdata(dev);
>  
> +	ipa_endpoint_suspend(ipa);
> +
>  	ipa_clock_put(ipa);
>  	if (!test_and_clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
>  		dev_err(dev, "suspend: missing suspend clock reference\n");
> @@ -932,6 +936,8 @@ static int ipa_suspend(struct device *dev)
>   * Return:	Always returns 0
>   *
>   * Called by the PM framework when a system resume operation is invoked.
> + * Takes an IPA clock reference to keep the clock running until suspend,
> + * and resumes endpoints.
>   */
>  static int ipa_resume(struct device *dev)
>  {
> @@ -945,6 +951,8 @@ static int ipa_resume(struct device *dev)
>  	else
>  		dev_err(dev, "resume: duplicate suspend clock reference\n");
>  
> +	ipa_endpoint_resume(ipa);
> +
>  	return 0;
>  }
>  
> -- 
> 2.20.1
>