linux-kernel - Re: [PATCH v2] staging: r8188eu: convert rtw_pwr

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: <YuDcKoSibAo93a6P@kroah.com>
Date:   Wed, 27 Jul 2022 08:33:14 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Phillip Potter <phil@...lpotter.co.uk>
Cc:     dan.carpenter@...cle.com, Larry.Finger@...inger.net,
        paskripkin@...il.com, straube.linux@...il.com, martin@...ser.cx,
        abdun.nihaal@...il.com, philipp.g.hortmann@...il.com,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] staging: r8188eu: convert rtw_pwr_wakeup to correct
 error code semantics

On Mon, Jul 25, 2022 at 11:07:45PM +0100, 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@...lpotter.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;

Nit, you don't need to set this again, as you already set it above to 0.

I'll take this as-is, as you are just keeping the original duplicated
logic, but it's something to clean up later.

Nice to see that moving to using the standard error values actually
removed lines of code, that's encouraging.

thanks,

greg k-h