Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210223142908.23374-1-youghand@codeaurora.org>
In-Reply-To: <20210223142908.23374-1-youghand@codeaurora.org>
From:   Abhishek Kumar <kuabhs@chromium.org>
Date:   Mon, 1 Mar 2021 17:51:05 -0800
Message-ID: <CACTWRwti94_VXpFZZhZKJhhJ_YUb1mCWjR2AtD_Rqwe6-GOGHQ@mail.gmail.com>
Subject: Re: [PATCH v3] ath10k: skip the wait for completion to recovery in
 shutdown path
To:     Youghandhar Chintala <youghand@codeaurora.org>
Cc:     ath10k <ath10k@lists.infradead.org>,
        ath10k-review.external@qti.qualcomm.com,
        Kalle Valo <kvalo@codeaurora.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        netdev <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Douglas Anderson <dianders@chromium.org>,
        Brian Norris <briannorris@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

This patch seems to address the comments on v2. Overall this patch LGTM.

Reviewed-by: Abhishek Kumar <kuabhs@chromium.org>

On Tue, Feb 23, 2021 at 6:29 AM Youghandhar Chintala
<youghand@codeaurora.org> wrote:
>
> Currently in the shutdown callback we wait for recovery to complete
> before freeing up the resources. This results in additional two seconds
> delay during the shutdown and thereby increase the shutdown time.
>
> As an attempt to take less time during shutdown, remove the wait for
> recovery completion in the shutdown callback and added an API to freeing
> the reosurces in which they were common for shutdown and removing
> the module.
>
> Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1
>
> Signed-off-by: Youghandhar Chintala <youghand@codeaurora.org>
> Change-Id: I65bc27b5adae1fedc7f7b367ef13aafbd01f8c0c
> ---
> Changes from v2:
> -Corrected commit text and added common API for freeing the
>  resources for shutdown and unloading the module
> ---
>  drivers/net/wireless/ath/ath10k/snoc.c | 29 ++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
> index 84666f72bdfa..70b3f2bd1c81 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -1781,17 +1781,11 @@ static int ath10k_snoc_probe(struct platform_device *pdev)
>         return ret;
>  }
>
> -static int ath10k_snoc_remove(struct platform_device *pdev)
> +static int ath10k_snoc_free_resources(struct ath10k *ar)
>  {
> -       struct ath10k *ar = platform_get_drvdata(pdev);
>         struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
>
> -       ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc remove\n");
> -
> -       reinit_completion(&ar->driver_recovery);
> -
> -       if (test_bit(ATH10K_SNOC_FLAG_RECOVERY, &ar_snoc->flags))
> -               wait_for_completion_timeout(&ar->driver_recovery, 3 * HZ);
> +       ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc free resources\n");
>
>         set_bit(ATH10K_SNOC_FLAG_UNREGISTERING, &ar_snoc->flags);
>
> @@ -1805,12 +1799,29 @@ static int ath10k_snoc_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +static int ath10k_snoc_remove(struct platform_device *pdev)
> +{
> +       struct ath10k *ar = platform_get_drvdata(pdev);
> +       struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
> +
> +       ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc remove\n");
> +
> +       reinit_completion(&ar->driver_recovery);
> +
> +       if (test_bit(ATH10K_SNOC_FLAG_RECOVERY, &ar_snoc->flags))
> +               wait_for_completion_timeout(&ar->driver_recovery, 3 * HZ);
> +
> +       ath10k_snoc_free_resources(ar);
> +
> +       return 0;
> +}
> +
>  static void ath10k_snoc_shutdown(struct platform_device *pdev)
>  {
>         struct ath10k *ar = platform_get_drvdata(pdev);
>
>         ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc shutdown\n");
> -       ath10k_snoc_remove(pdev);
> +       ath10k_snoc_free_resources(ar);
>  }
>
>  static struct platform_driver ath10k_snoc_driver = {
> --
> 2.29.0
>