netdev - Re: [PATCH] Staging: qlge: Fix indentation in conditional statement

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] [day] [month] [year] [list]

Message-ID: <cf0bf6c1-30fb-4c63-9c17-575c87c986e3@kili.mountain>
Date:   Sat, 11 Mar 2023 19:58:32 +0300
From:   Dan Carpenter <error27@...il.com>
To:     Sumitra Sharma <sumitraartsy@...il.com>
Cc:     outreachy@...ts.linux.dev, manishc@...vell.com,
        GR-Linux-NIC-Dev@...vell.com, coiby.xu@...il.com,
        gregkh@...uxfoundation.org, netdev@...r.kernel.org,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: qlge: Fix indentation in conditional statement

On Sat, Mar 11, 2023 at 07:24:53AM -0800, Sumitra Sharma wrote:
> Add tabs/spaces in conditional statements in qlge_dbg.c to fix the
> indentation.
> 
> Signed-off-by: Sumitra Sharma <sumitraartsy@...il.com>
> ---
>  drivers/staging/qlge/qlge_dbg.c | 35 +++++++++++++++------------------
>  1 file changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
> index b190a2993033..c7e865f515cf 100644
> --- a/drivers/staging/qlge/qlge_dbg.c
> +++ b/drivers/staging/qlge/qlge_dbg.c
> @@ -351,26 +351,23 @@ static int qlge_get_xgmac_regs(struct qlge_adapter *qdev, u32 *buf,
>  		/* We're reading 400 xgmac registers, but we filter out
>  		 * several locations that are non-responsive to reads.
>  		 */
> -		if (i == 0x00000114 ||
> -		    i == 0x00000118 ||
> -			i == 0x0000013c ||
> -			i == 0x00000140 ||

You've written this on top of the other patch which we're not going
to apply so it's not going to work.

> -			(i > 0x00000150 && i < 0x000001fc) ||
> -			(i > 0x00000278 && i < 0x000002a0) ||
> -			(i > 0x000002c0 && i < 0x000002cf) ||
> -			(i > 0x000002dc && i < 0x000002f0) ||
> -			(i > 0x000003c8 && i < 0x00000400) ||
> -			(i > 0x00000400 && i < 0x00000410) ||
> -			(i > 0x00000410 && i < 0x00000420) ||
> -			(i > 0x00000420 && i < 0x00000430) ||
> -			(i > 0x00000430 && i < 0x00000440) ||
> -			(i > 0x00000440 && i < 0x00000450) ||
> -			(i > 0x00000450 && i < 0x00000500) ||
> -			(i > 0x0000054c && i < 0x00000568) ||
> -			(i > 0x000005c8 && i < 0x00000600)) {
> +		if ((i == 0x00000114) || (i == 0x00000118) ||
> +		    (i == 0x0000013c) || (i == 0x00000140) ||

If we could have applied the patch then I wouldn't comment here.  But
since you're going to have to redo it anyway...  I would probably have
kept these on separate lines.

		if ((i == 0x00000114) ||
		    (i == 0x00000118) ||
		    (i == 0x0000013c) ||
	            (i == 0x00000140) ||
		    (i > 0x00000150 && i < 0x000001fc) ||
		    (i > 0x00000278 && i < 0x000002a0) ||

I like that you are looking around and making changes, like this but to
me it seems more readable if 0x114 0x118 etc are all in the same
column.

> +		    (i > 0x00000150 && i < 0x000001fc) ||
> +		    (i > 0x00000278 && i < 0x000002a0) ||
> +		    (i > 0x000002c0 && i < 0x000002cf) ||
> +		    (i > 0x000002dc && i < 0x000002f0) ||
> +		    (i > 0x000003c8 && i < 0x00000400) ||
> +		    (i > 0x00000400 && i < 0x00000410) ||
> +		    (i > 0x00000410 && i < 0x00000420) ||
> +		    (i > 0x00000420 && i < 0x00000430) ||
> +		    (i > 0x00000430 && i < 0x00000440) ||
> +		    (i > 0x00000440 && i < 0x00000450) ||
> +		    (i > 0x00000450 && i < 0x00000500) ||
> +		    (i > 0x0000054c && i < 0x00000568) ||
> +		    (i > 0x000005c8 && i < 0x00000600)) {
>  			if (other_function)
> -				status =
> -				qlge_read_other_func_xgmac_reg(qdev, i, buf);
> +				status = qlge_read_other_func_xgmac_reg(qdev, i, buf);

This change wasn't described in the commit message.  This change is a
borderline situation on the one thing per patch rule.  We would
probably allow it under certain circumstances if it were described
correctly in the commit message.  But with Outreachy we're crazy strict
about stuff like this.  Just send it as a separate patch.

So now this is a v2 patch situation.  Outreachy has their own docs.  But
I have a blog about this which is super short.
https://staticthinking.wordpress.com/2022/07/27/how-to-send-a-v2-patch/


regards,
dan carpenter