Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org E1752C53A37
From:   Kalle Valo <kvalo@codeaurora.org>
To:     Bo YU <tsu.yubo@gmail.com>
Cc:     kvalo@codeaurora.org, davem@davemloft.net, kuba@kernel.org,
        ath11k@lists.infradead.org, linux-wireless@vger.kernel.org,
        rmanohar@codeaurora.org
Subject: Re: [PATCH -next] ath11k: Add checked value for ath11k_ahb_remove
References: <20200621095136.7xdbzkthoxuw2qow@debian.debian-2>
Date:   Mon, 7 Sep 2020 10:27:58 +0000
In-Reply-To: <20200621095136.7xdbzkthoxuw2qow@debian.debian-2> (Bo YU's
        message of "Sun, 21 Jun 2020 17:51:40 +0800")
Message-ID: <01010174681bace8-5b83f615-4790-4722-8257-799205c9c96f-000000@us-west-2.amazonses.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Feedback-ID: 1.us-west-2.CZuq2qbDmUIuT3qdvXlRHZZCpfZqZ4GtG9v3VKgRyF0=:AmazonSES
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

+ rajkumar

Bo YU <tsu.yubo@gmail.com> writes:

> Return value form wait_for_completion_timeout should to be checked.
>
> This is detected by Coverity,#CID:1464479 (CHECKED_RETURN)
>
> FIXES: d5c65159f2895(ath11k: driver for Qualcomm IEEE 802.11ax devices)

This should be

Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")

But I can fix that.

> --- a/drivers/net/wireless/ath/ath11k/ahb.c
> +++ b/drivers/net/wireless/ath/ath11k/ahb.c
> @@ -981,12 +981,16 @@ static int ath11k_ahb_probe(struct platform_device *pdev)
> static int ath11k_ahb_remove(struct platform_device *pdev)
> {
> 	struct ath11k_base *ab = platform_get_drvdata(pdev);
> -
> +	int ret = 0;
> 	reinit_completion(&ab->driver_recovery);
>
> 	if (test_bit(ATH11K_FLAG_RECOVERY, &ab->dev_flags))
> -		wait_for_completion_timeout(&ab->driver_recovery,
> -					    ATH11K_AHB_RECOVERY_TIMEOUT);
> +		if (!wait_for_completion_timeout(&ab->driver_recovery,
> +						 ATH11K_AHB_RECOVERY_TIMEOUT)) {
> +			ath11k_warn(ab, "fail to receive recovery response completion.\n");
> +			ret = -ETIMEDOUT;
> +		}

This is a good find. Rajkumar, can you take a look if this is ok?

>
> 	set_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags);
> 	cancel_work_sync(&ab->restart_work);
> @@ -999,7 +1003,7 @@ static int ath11k_ahb_remove(struct platform_device *pdev)
> 	ath11k_core_free(ab);
> 	platform_set_drvdata(pdev, NULL);
>
> -	return 0;
> +	return ret;
> }

Especially I wonder what happens if ath11k_ahb_remove() returns an
error. Should we just print a warning and return 0 instead?

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches