linux-kernel - Re: [PATCH 06/10] staging: r8188eu: remove DBG_88E calls from os_dep/ioctl

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]

Date:   Tue, 25 Jan 2022 22:03:06 +0300
From:   Pavel Skripkin <paskripkin@...il.com>
To:     Phillip Potter <phil@...lpotter.co.uk>, gregkh@...uxfoundation.org
Cc:     dan.carpenter@...cle.com, Larry.Finger@...inger.net,
        straube.linux@...il.com, martin@...ser.cx,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 06/10] staging: r8188eu: remove DBG_88E calls from
 os_dep/ioctl_linux.c

Hi Phillip,

On 1/25/22 01:44, Phillip Potter wrote:
> Remove all DBG_88E calls from os_dep/ioctl_linux.c, as they do not
> conform to kernel coding standards and are superfluous. Also restructure
> where appropriate to remove no longer needed code left behind by removal
> of these calls. This will allow the eventual removal of the DBG_88E macro
> itself.
> 
> Signed-off-by: Phillip Potter <phil@...lpotter.co.uk>
> ---

[code snip]

> @@ -3746,7 +3541,6 @@ static int rtw_dbg_port(struct net_device *dev,
>   			u32 write_num = extra_arg;
>   
>   			int i;
> -			u16 final;
>   			struct xmit_frame	*xmit_frame;
>   
>   			xmit_frame = rtw_IOL_accquire_xmit_frame(padapter);
> @@ -3760,11 +3554,7 @@ static int rtw_dbg_port(struct net_device *dev,
>   			if (rtl8188e_IOL_exec_cmds_sync(padapter, xmit_frame, 5000, 0) != _SUCCESS)
>   				ret = -EPERM;
>   
> -			final = rtw_read16(padapter, reg);
> -			if (start_value + write_num - 1 == final)
> -				DBG_88E("continuous IOL_CMD_WW_REG to 0x%x %u times Success, start:%u, final:%u\n", reg, write_num, start_value, final);
> -			else
> -				DBG_88E("continuous IOL_CMD_WW_REG to 0x%x %u times Fail, start:%u, final:%u\n", reg, write_num, start_value, final);
> +			rtw_read16(padapter, reg);
>   		}
>   			break;

I see, that you somewhere removes reads and somewhere leaves them. 
What's the difference? I saw, that one of the places has the comment, 
that asks not to remove the read, but others do not have such comment

I can point to few places in 2 and 4 patches where you have removed reads.



[code snip]

> @@ -4014,16 +3664,8 @@ static int rtw_dbg_port(struct net_device *dev,
>   			{
>   				u32 odm_flag;
>   
> -				if (0xf == extra_arg) {
> +				if (extra_arg == 0xf) {
>   					GetHalDefVar8188EUsb(padapter, HAL_DEF_DBG_DM_FUNC, &odm_flag);
> -					DBG_88E(" === DMFlag(0x%08x) ===\n", odm_flag);
> -					DBG_88E("extra_arg = 0  - disable all dynamic func\n");
> -					DBG_88E("extra_arg = 1  - disable DIG- BIT(0)\n");
> -					DBG_88E("extra_arg = 2  - disable High power - BIT(1)\n");
> -					DBG_88E("extra_arg = 3  - disable tx power tracking - BIT(2)\n");
> -					DBG_88E("extra_arg = 4  - disable BT coexistence - BIT(3)\n");
> -					DBG_88E("extra_arg = 5  - disable antenna diversity - BIT(4)\n");
> -					DBG_88E("extra_arg = 6  - enable all dynamic func\n");
>   				} else {
>   					/*	extra_arg = 0  - disable all dynamic func
>   						extra_arg = 1  - disable DIG
> @@ -4032,51 +3674,17 @@ static int rtw_dbg_port(struct net_device *dev,
>   					*/
>   					SetHalDefVar8188EUsb(padapter, HAL_DEF_DBG_DM_FUNC, &extra_arg);
>   					GetHalDefVar8188EUsb(padapter, HAL_DEF_DBG_DM_FUNC, &odm_flag);
> -					DBG_88E(" === DMFlag(0x%08x) ===\n", odm_flag);
>   				}
>   			}
>   			break;
>   

Is odm_flag needed now? Seems like it was used only for printing random 
debug info

>   		case 0xfd:
>   			rtw_write8(padapter, 0xc50, arg);
> -			DBG_88E("wr(0xc50) = 0x%x\n", rtw_read8(padapter, 0xc50));
>   			rtw_write8(padapter, 0xc58, arg);
> -			DBG_88E("wr(0xc58) = 0x%x\n", rtw_read8(padapter, 0xc58));
> -			break;
> -		case 0xfe:
> -			DBG_88E("rd(0xc50) = 0x%x\n", rtw_read8(padapter, 0xc50));
> -			DBG_88E("rd(0xc58) = 0x%x\n", rtw_read8(padapter, 0xc58));
> -			break;
> -		case 0xff:
> -			DBG_88E("dbg(0x210) = 0x%x\n", rtw_read32(padapter, 0x210));
> -			DBG_88E("dbg(0x608) = 0x%x\n", rtw_read32(padapter, 0x608));
> -			DBG_88E("dbg(0x280) = 0x%x\n", rtw_read32(padapter, 0x280));
> -			DBG_88E("dbg(0x284) = 0x%x\n", rtw_read32(padapter, 0x284));
> -			DBG_88E("dbg(0x288) = 0x%x\n", rtw_read32(padapter, 0x288));
> -
> -			DBG_88E("dbg(0x664) = 0x%x\n", rtw_read32(padapter, 0x664));
> -
> -			DBG_88E("\n");
> -
> -			DBG_88E("dbg(0x430) = 0x%x\n", rtw_read32(padapter, 0x430));
> -			DBG_88E("dbg(0x438) = 0x%x\n", rtw_read32(padapter, 0x438));
> -
> -			DBG_88E("dbg(0x440) = 0x%x\n", rtw_read32(padapter, 0x440));
> -
> -			DBG_88E("dbg(0x458) = 0x%x\n", rtw_read32(padapter, 0x458));
> -
> -			DBG_88E("dbg(0x484) = 0x%x\n", rtw_read32(padapter, 0x484));
> -			DBG_88E("dbg(0x488) = 0x%x\n", rtw_read32(padapter, 0x488));
> -
> -			DBG_88E("dbg(0x444) = 0x%x\n", rtw_read32(padapter, 0x444));
> -			DBG_88E("dbg(0x448) = 0x%x\n", rtw_read32(padapter, 0x448));
> -			DBG_88E("dbg(0x44c) = 0x%x\n", rtw_read32(padapter, 0x44c));
> -			DBG_88E("dbg(0x450) = 0x%x\n", rtw_read32(padapter, 0x450));
>   			break;
>   		}

And here you also removes the reads. I guess, some kind of magic pattern 
is used




With regards,
Pavel Skripkin