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;
Date:   Tue, 12 Jul 2022 16:46:10 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Binyi Han <dantengknight@gmail.com>
Cc:     Manish Chopra <manishc@marvell.com>, GR-Linux-NIC-Dev@marvell.com,
        Coiby Xu <coiby.xu@gmail.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Joe Perches <joe@perches.com>, netdev@vger.kernel.org,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] staging: qlge: Fix indentation issue under long for
 loop
Message-ID: <20220712134610.GO2338@kadam>
References: <20220710210418.GA148412@cloud-MacBookPro>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220710210418.GA148412@cloud-MacBookPro>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?pmbmpgkebH8fQ+4WMiuWZY5Ci+bUVrk1fimjLOOJtftNV9dHwvxiuTSCMC40?=
 =?us-ascii?Q?vLCj0Si4+7i4ynXIKx1ut2CheLFY9PihD1pxOp1vo78Qinb6lYECkuYoaRRr?=
 =?us-ascii?Q?C14+fnMTQ/a8iISTJIuObD3PL6NLZuD0f4Ui3sp+IvTFZIwBE6qdTuBH2/H0?=
 =?us-ascii?Q?NvuftBPOzHtOVbwX5JnHrRLwAWozwhUwjT3MfVlk4b+D/JNjopGi/PwkXlPA?=
 =?us-ascii?Q?xRI1ButqtJ9SC7287yv5KsXrEdnhY1qKr3XdzZotOpzVnkBbZ6supcSQiLFc?=
 =?us-ascii?Q?BW4f+r5BoMgRO36U0350iBRqkgGFpRtWfSTyLDQVXsyNrvETmAL1KHL41isP?=
 =?us-ascii?Q?zWSHtCBmBiV3j1SQgsKx2y3KaGiB9v+p4BTCR4jUXG2yQhRK8KwMRO2eF8LO?=
 =?us-ascii?Q?8XZD+5eRqJzZ1ua/KG6Wa7Ucl747NJtuMrdrbmKs0atynaAZy4bSSJ9Mmzih?=
 =?us-ascii?Q?Qiru7pRnuPP9bHBlo8tVVUPXaVh2JSsRHbTX0/I/fl1CIQ8K/RGjeX77IoK4?=
 =?us-ascii?Q?mllD8H5AlfdHTMu9G2dTNih2mwnmRnxAfqF2/YqYqtLMIWrCnAeF3Ag91dEA?=
 =?us-ascii?Q?2pBa6+YbMAO7DmP6Pk2fjVYqHX9ukFI+sNHbuI9+S9qcIdPlW0t/JqORUseI?=
 =?us-ascii?Q?jJGxzPfy47laRo7BMb55f2B8H65fJ06VCvP7NcjvZQbtu4PU5dvMJ7F9pNOx?=
 =?us-ascii?Q?aQSvTwebS+tsqKCFFRepFsBJlVwHguynrDBbiiGmZoZ3W+50vPPlOawHo23U?=
 =?us-ascii?Q?ViKnAo5RIGhjqAwsY6He45eQ3Z/bLIw2fbfYEXKttnA/ifKPWKX5eXA/wlz7?=
 =?us-ascii?Q?Ya82vz0y0S7sGgVzSfuAcaO369+D33D4Vw28Rek/STqqa3In2OLhafBc9WNv?=
 =?us-ascii?Q?7AZE3UAxBauKDyoj4a/lzkUi0W9cwdFpwiKD9iBAEQ4Hlb+AUcrT/F4vMIIP?=
 =?us-ascii?Q?MxbA8/6sMQxe+RXYX0mi31+y+siZOXfOoHG+amRaL/IhVdJtruva9l4Rw9ac?=
 =?us-ascii?Q?ZcJugKdcEA9CRQO5MG/LP4F3Zg9+yG50etvUJUrIrmiK04kjBi9hlNSX/ciu?=
 =?us-ascii?Q?ZJlvRZ02FJty+boV5dOWTm5xhPutjXw45fd2N57/sH9lNx88BIz3Z3v/9cQb?=
 =?us-ascii?Q?0JbFZYu3ztxA6Kizow8QamIEyBuJwuX4UZxZqfrfqSfBJy1qA/L0X3TqzAkb?=
 =?us-ascii?Q?8rMXjYSV5gMJE5qV00L5hkvcIPJfzajX2pEJOgCko72q90wDutlCLtWWctFY?=
 =?us-ascii?Q?4kbjxVYBUUVAOemxaWp9tYdz5BZpiZft5uPeyMbyLE/kB2nrjk6UPGgxOIyn?=
 =?us-ascii?Q?rJIf7ARYbHKvga1WWBbzgiWAhSSBs+PfEjtBWFAMl55BgRc76p6H/LAT4HP5?=
 =?us-ascii?Q?T6ubuKFmJ+qE6oP/IWyufyB7Jfhl5L5rSWRyA0J43QwOmhHXfQ6MHsVZdWTr?=
 =?us-ascii?Q?wPI+0YFYBgdcSNkhiVT6wnwWxQ3Hn1gWYoMY8hZvInCxiU6EtVO6wY1ZzNJB?=
 =?us-ascii?Q?c+K2GTcGoXhA9tnsSvbtrCJEqAtocfhIy3zbRWlDEpz9JCdGHGFGBvviGQZr?=
 =?us-ascii?Q?lmcjxjjPDZx1e8EWBNPEECkSguyuI21mP2xiQIAF+UMY9q6lQApnC2NxXmYJ?=
 =?us-ascii?Q?NQ=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ce8ad774-7dab-4fb1-ce48-08da640ce754
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Jul 2022 13:46:21.3576
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2+bnl5O6HTcn/vCYnLoK2PgtaZL2u1QAQLW/b4u+iCpRiFKpEy1oqR3how5eIliarfdNi1MdjNIRt/r/Rq/c2aOmz/ftJYquRJnUeoF61S8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN0PR10MB4904
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.517,18.0.883
 definitions=2022-07-12_08:2022-07-12,2022-07-12 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxlogscore=999
 malwarescore=0 suspectscore=0 mlxscore=0 spamscore=0 adultscore=0
 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2206140000 definitions=main-2207120053
X-Proofpoint-GUID: eSPe78UtmY7xQCdRWTDdQLjM8YZneZaV
X-Proofpoint-ORIG-GUID: eSPe78UtmY7xQCdRWTDdQLjM8YZneZaV
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Sun, Jul 10, 2022 at 02:04:18PM -0700, Binyi Han wrote:
> Fix indentation issue to adhere to Linux kernel coding style,
> Issue found by checkpatch. Change the long for loop into 3 lines. And
> optimize by avoiding the multiplication.

There is no possible way this optimization helps benchmarks.  Better to
focus on readability.

> 
> Signed-off-by: Binyi Han <dantengknight@gmail.com>
> ---
> v2:
> 	- Change the long for loop into 3 lines.
> v3:
> 	- Align page_entries in the for loop to open parenthesis.
> 	- Optimize by avoiding the multiplication.
> 
>  drivers/staging/qlge/qlge_main.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> index 1a378330d775..4b166c66cfc5 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -3007,10 +3007,12 @@ static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct rx_ring *rx_ring
>  		tmp = (u64)rx_ring->lbq.base_dma;
>  		base_indirect_ptr = rx_ring->lbq.base_indirect;
>  
> -		for (page_entries = 0; page_entries <
> -			MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); page_entries++)
> -				base_indirect_ptr[page_entries] =
> -					cpu_to_le64(tmp + (page_entries * DB_PAGE_SIZE));
> +		for (page_entries = 0;
> +		     page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN);
> +		     page_entries++) {
> +			base_indirect_ptr[page_entries] = cpu_to_le64(tmp);
> +			tmp += DB_PAGE_SIZE;

I've previously said that using "int i;" is clearer here.  You would
kind of expect "page_entries" to be the number of entries, so it's kind
of misleading.  In other words, it's not just harmless wordiness and
needless exposition, it's actively bad.

I would probably just put it on one line:

		for (i = 0; i MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); i++)
			base_indirect_ptr[i] = cpu_to_le64(tmp + (i * DB_PAGE_SIZE));

But if you want to break it up you could do:

		for (i = 0; i MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN); i++)
			base_indirect_ptr[i] = cpu_to_le64(tmp +
							   (i * DB_PAGE_SIZE));

"tmp" is kind of a bad name.  Also "base_indirect_ptr" would be better
as "base_indirect".

regards,
dan carpenter