Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <13992777-97c4-2b1b-99dc-876bec9a6dde@gmail.com>
Date:   Tue, 26 Jul 2022 07:09:49 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v2] staging: r8188eu: convert rtw_pwr_wakeup to correct
 error code semantics
Content-Language: en-US
To:     Phillip Potter <phil@philpotter.co.uk>, gregkh@linuxfoundation.org,
        dan.carpenter@oracle.com
Cc:     Larry.Finger@lwfinger.net, paskripkin@gmail.com,
        straube.linux@gmail.com, martin@kaiser.cx, abdun.nihaal@gmail.com,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
References: <20220725220745.12739-1-phil@philpotter.co.uk>
From:   Philipp Hortmann <philipp.g.hortmann@gmail.com>
In-Reply-To: <20220725220745.12739-1-phil@philpotter.co.uk>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 7/26/22 00:07, Phillip Potter wrote:
> Convert the rtw_pwr_wakeup function to use 0 on success and an appropriate
> error code on error. For the first failure block where ips_leave is
> invoked, use -ENOMEM as this is the main cause of failure here anyway.
> For the second failure block, use -EBUSY, as it seems the most
> appropriate.
> 
> Finally, within the functions rtw_wx_set_mode, rtw_wx_set_wap,
> rtw_wx_set_scan and rtw_wx_set_essid, pass the error code on from
> rtw_pwr_wakeup as appropriate now that it is converted.
> 
> This gets the driver closer to removal of the non-standard _SUCCESS and
> _FAIL definitions, which are inverted compared to the standard in-kernel
> error code mechanism.
> 
> Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> ---
> 
> Changes from V1: Act on feedback from Dan Carpenter:
> * Try to use more appropriate error codes than -EPERM.
> * Revert the places where existing -1 was converted as they are out of
>    scope.
> * Preserve error codes in places where calling function already uses
>    proper negative semantics, so that they can be passed through to the
>    caller.
> 
> ---
>   drivers/staging/r8188eu/core/rtw_p2p.c       |  4 ++--
>   drivers/staging/r8188eu/core/rtw_pwrctrl.c   | 10 ++++----
>   drivers/staging/r8188eu/os_dep/ioctl_linux.c | 24 +++++++-------------
>   3 files changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/core/rtw_p2p.c b/drivers/staging/r8188eu/core/rtw_p2p.c
> index c306aafa183b..bd654d4ff8b4 100644
> --- a/drivers/staging/r8188eu/core/rtw_p2p.c
> +++ b/drivers/staging/r8188eu/core/rtw_p2p.c
> @@ -1888,7 +1888,7 @@ int rtw_p2p_enable(struct adapter *padapter, enum P2P_ROLE role)
>   
>   	if (role == P2P_ROLE_DEVICE || role == P2P_ROLE_CLIENT || role == P2P_ROLE_GO) {
>   		/* leave IPS/Autosuspend */
> -		if (rtw_pwr_wakeup(padapter) == _FAIL) {
> +		if (rtw_pwr_wakeup(padapter)) {
>   			ret = _FAIL;
>   			goto exit;
>   		}
> @@ -1902,7 +1902,7 @@ int rtw_p2p_enable(struct adapter *padapter, enum P2P_ROLE role)
>   		init_wifidirect_info(padapter, role);
>   
>   	} else if (role == P2P_ROLE_DISABLE) {
> -		if (rtw_pwr_wakeup(padapter) == _FAIL) {
> +		if (rtw_pwr_wakeup(padapter)) {
>   			ret = _FAIL;
>   			goto exit;
>   		}
> diff --git a/drivers/staging/r8188eu/core/rtw_pwrctrl.c b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> index cf9020a73933..8b1c50668dfe 100644
> --- a/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> +++ b/drivers/staging/r8188eu/core/rtw_pwrctrl.c
> @@ -381,24 +381,24 @@ int rtw_pwr_wakeup(struct adapter *padapter)
>   	struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
>   	unsigned long timeout = jiffies + msecs_to_jiffies(3000);
>   	unsigned long deny_time;
> -	int ret = _SUCCESS;
> +	int ret = 0;
>   
>   	while (pwrpriv->ps_processing && time_before(jiffies, timeout))
>   		msleep(10);
>   
>   	/* I think this should be check in IPS, LPS, autosuspend functions... */
>   	if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> -		ret = _SUCCESS;
> +		ret = 0;
>   		goto exit;
>   	}
>   
>   	if (pwrpriv->rf_pwrstate == rf_off && ips_leave(padapter) == _FAIL) {
> -		ret = _FAIL;
> +		ret = -ENOMEM;
>   		goto exit;
>   	}
>   
>   	if (padapter->bDriverStopped || !padapter->bup || !padapter->hw_init_completed) {
> -		ret = _FAIL;
> +		ret = -EBUSY;
>   		goto exit;
>   	}
>   
> @@ -439,7 +439,7 @@ int rtw_pm_set_ips(struct adapter *padapter, u8 mode)
>   		return 0;
>   	} else if (mode == IPS_NONE) {
>   		rtw_ips_mode_req(pwrctrlpriv, mode);
> -		if ((padapter->bSurpriseRemoved == 0) && (rtw_pwr_wakeup(padapter) == _FAIL))
> +		if ((padapter->bSurpriseRemoved == 0) && rtw_pwr_wakeup(padapter))
>   			return -EFAULT;
>   	} else {
>   		return -EINVAL;
> diff --git a/drivers/staging/r8188eu/os_dep/ioctl_linux.c b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> index 930bb4aea435..7f91dac2e41b 100644
> --- a/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> +++ b/drivers/staging/r8188eu/os_dep/ioctl_linux.c
> @@ -687,12 +687,9 @@ static int rtw_wx_set_mode(struct net_device *dev, struct iw_request_info *a,
>   	enum ndis_802_11_network_infra networkType;
>   	int ret = 0;
>   
> -
> -
> -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> -		ret = -EPERM;
> +	ret = rtw_pwr_wakeup(padapter);
> +	if (ret)
>   		goto exit;
> -	}
>   
>   	if (!padapter->hw_init_completed) {
>   		ret = -EPERM;
> @@ -931,12 +928,9 @@ static int rtw_wx_set_wap(struct net_device *dev,
>   	struct	wlan_network	*pnetwork = NULL;
>   	enum ndis_802_11_auth_mode	authmode;
>   
> -
> -
> -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> -		ret = -1;
> +	ret = rtw_pwr_wakeup(padapter);
> +	if (ret)
>   		goto exit;
> -	}
>   
>   	if (!padapter->bup) {
>   		ret = -1;
> @@ -1049,10 +1043,9 @@ static int rtw_wx_set_scan(struct net_device *dev, struct iw_request_info *a,
>   	struct ndis_802_11_ssid ssid[RTW_SSID_SCAN_AMOUNT];
>   	struct wifidirect_info *pwdinfo = &padapter->wdinfo;
>   
> -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> -		ret = -1;
> +	ret = rtw_pwr_wakeup(padapter);
> +	if (ret)
>   		goto exit;
> -	}
>   
>   	if (padapter->bDriverStopped) {
>   		ret = -1;
> @@ -1252,10 +1245,9 @@ static int rtw_wx_set_essid(struct net_device *dev,
>   
>   	uint ret = 0, len;
>   
> -	if (_FAIL == rtw_pwr_wakeup(padapter)) {
> -		ret = -1;
> +	ret = rtw_pwr_wakeup(padapter);
> +	if (ret)
>   		goto exit;
> -	}
>   
>   	if (!padapter->bup) {
>   		ret = -1;

Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com> # Edimax N150