Message ID | 2dd27eff9aab5ffe31e61086c0584982794507cf.1666011479.git.drv@mailo.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp1464565wrs; Mon, 17 Oct 2022 07:03:28 -0700 (PDT) X-Google-Smtp-Source: AMsMyM718D5fwfuulMIsjotGAwrGyUYhXikS8D5bqpkudhqKfwhW8olQl4hdtvpAmznev0vrjozt X-Received: by 2002:a05:6402:1686:b0:459:4ddf:8f05 with SMTP id a6-20020a056402168600b004594ddf8f05mr10386829edv.375.1666015408162; Mon, 17 Oct 2022 07:03:28 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666015408; cv=none; d=google.com; s=arc-20160816; b=x+t+zHM8kWrInxZGyckq4cC6NU4UQnq5g/421Mn8RlJseKBhri02SyQ8P+BKSET3et y6idK1Ixi6A7WjHzWWTxvX+4l/Jp47NGT85klu66JUQzKA1Ifa7NXvPVPxu2OxGFPeTK 9xu7EnzwV/kRv3UtyyPKTFvI9fEBmEn2eGvHgi5j04+peMEH29Hdegt6B9xBK2EX3qZH wZ7DwQmcNTccsdSLxpt9P9mlnqRPu5c7TDMEw5ubxkXxsiMOViw29TMye2U3FcJcUSxB A3MwJdlBIaXe1re7kLDGyx/mdfGnbSwBhr4YJxHFG7B9SGK0HPsUyonLAU4AyQ7HgJzK W4kA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=BAOxoONT/XwmZInxLm9YMn4HtwE0fHIEH5U+aUHaFdY=; b=DWXhKZq1lC7L5q2gsLNWDCu8xLOh9DEU3VPvPJ7+LTo4KZLGoMfrhK+Puqhc/q4uoa S5hpMWE32qcO+qgJO8a5M0fk9LUxXhc+xEmKHvYXXK+93F9gY7YRlhWnPPbL1vjnp0ra dowfhpVqonfwqYgcUjrSMTCxh7CDwSRdniKcoFhCPyKo/8fvaIi3wElBPwnK2EHJUAok 2uzZuUDnGc1qlaKonusYnkj4r/yojwU5wT61s8qBC4L5JMu0dBZXQo79+neGnFkYWEzD 5j35/24IwD4fYtUxoVrrSeoN7lk5/JJe5wrWvAog9sFOSKZgVOtAfC4phmAEwFdRbX20 LWgw== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=hvlC17pu; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id l16-20020aa7cad0000000b00459c1e64776si8329302edt.412.2022.10.17.07.02.23; Mon, 17 Oct 2022 07:03:28 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=fail header.i=@mailo.com header.s=mailo header.b=hvlC17pu; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=mailo.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230119AbiJQNwm (ORCPT <rfc822;ouuuleilei@gmail.com> + 99 others); Mon, 17 Oct 2022 09:52:42 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54220 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229969AbiJQNwk (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 17 Oct 2022 09:52:40 -0400 Received: from msg-2.mailo.com (msg-2.mailo.com [213.182.54.12]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 980221C125 for <linux-kernel@vger.kernel.org>; Mon, 17 Oct 2022 06:52:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=mailo.com; s=mailo; t=1666014744; bh=oYVwPDhXlI3Y4t67S9SXV2J/p4NYzrT/hdmHZXrTn7A=; h=X-EA-Auth:Date:From:To:Cc:Subject:Message-ID:References: MIME-Version:Content-Type:In-Reply-To; b=hvlC17puZA5L1lKIYDi5h5K/Byf14bivO7iCwpO0l0IZTgEVy81+YNIn/wtUNEfDw muhD3Iq+bpqBGNd4ZjsfJI/+IUSOXvocujRDbCk78McMlQxMUvErtspeVJ9V2E0z8V eTXKTO6g1DHCmQqXzSymGrntRSV8DFv41PceR4CI= Received: by b-4.in.mailobj.net [192.168.90.14] with ESMTP via [213.182.55.206] Mon, 17 Oct 2022 15:52:24 +0200 (CEST) X-EA-Auth: dh3puqb9svTS10k+Nb9Vk8Bw4QeB0YC0aC3X5FK0Xu9oWOTuOSqm9EvVedcOtwE1VhO6BpMlPSPi9xH1mnk2PS2tXCzE19vJ Date: Mon, 17 Oct 2022 18:52:50 +0530 From: Deepak R Varma <drv@mailo.com> To: outreachy@lists.linux.dev, Larry.Finger@lwfinger.net, phil@philpotter.co.uk, paskripkin@gmail.com, gregkh@linuxfoundation.org, linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org Cc: kumarpraveen@linux.microsoft.com, saurabh.truth@gmail.com Subject: [PATCH 2/4] staging: r8188eu: reformat long computation lines Message-ID: <2dd27eff9aab5ffe31e61086c0584982794507cf.1666011479.git.drv@mailo.com> References: <cover.1666011479.git.drv@mailo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <cover.1666011479.git.drv@mailo.com> X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS 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 X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1746943772910472483?= X-GMAIL-MSGID: =?utf-8?q?1746943772910472483?= |
Series |
staging: r8188eu: trivial code cleanup patches
|
|
Commit Message
Deepak R Varma
Oct. 17, 2022, 1:22 p.m. UTC
Reformat long running computation instructions to improve code readability.
Address following checkpatch script complaints:
CHECK: line length of 171 exceeds 100 columns
CHECK: line length of 113 exceeds 100 columns
Signed-off-by: Deepak R Varma <drv@mailo.com>
---
drivers/staging/r8188eu/core/rtw_br_ext.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
--
2.30.2
Comments
On Mon, Oct 17, 2022 at 06:52:50PM +0530, Deepak R Varma wrote: > Reformat long running computation instructions to improve code readability. > Address following checkpatch script complaints: > CHECK: line length of 171 exceeds 100 columns > CHECK: line length of 113 exceeds 100 columns > > Signed-off-by: Deepak R Varma <drv@mailo.com> > --- > drivers/staging/r8188eu/core/rtw_br_ext.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c > index 79daf8f269d6..427da7e8ba4c 100644 > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c > @@ -211,8 +211,10 @@ static int __nat25_network_hash(unsigned char *network_addr) > } else if (network_addr[0] == NAT25_IPX) { > unsigned long x; > > - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ network_addr[5] ^ > - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ network_addr[10]; > + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ Why not go out to [4] here and then you are one line shorter? > + network_addr[4] ^ network_addr[5] ^ network_addr[6] ^ > + network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ > + network_addr[10]; > > return x & (NAT25_HASH_SIZE - 1); > } else if (network_addr[0] == NAT25_APPLE) { > @@ -224,16 +226,20 @@ static int __nat25_network_hash(unsigned char *network_addr) > } else if (network_addr[0] == NAT25_PPPOE) { > unsigned long x; > > - x = network_addr[0] ^ network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ network_addr[5] ^ network_addr[6] ^ network_addr[7] ^ network_addr[8]; > + x = network_addr[0] ^ network_addr[1] ^ network_addr[2] ^ > + network_addr[3] ^ network_addr[4] ^ network_addr[5] ^ Same here > + network_addr[6] ^ network_addr[7] ^ network_addr[8]; > > return x & (NAT25_HASH_SIZE - 1); > } else if (network_addr[0] == NAT25_IPV6) { > unsigned long x; > > - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ network_addr[5] ^ > - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ network_addr[10] ^ > - network_addr[11] ^ network_addr[12] ^ network_addr[13] ^ network_addr[14] ^ network_addr[15] ^ > - network_addr[16]; > + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ > + network_addr[4] ^ network_addr[5] ^ network_addr[6] ^ > + network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ > + network_addr[10] ^ network_addr[11] ^ network_addr[12] ^ > + network_addr[13] ^ network_addr[14] ^ network_addr[15] ^ > + network_addr[16]; And here. thanks, greg k-h
On Mon, Oct 17, 2022 at 04:09:49PM +0200, Greg KH wrote: > On Mon, Oct 17, 2022 at 06:52:50PM +0530, Deepak R Varma wrote: > > Reformat long running computation instructions to improve code readability. > > Address following checkpatch script complaints: > > CHECK: line length of 171 exceeds 100 columns > > CHECK: line length of 113 exceeds 100 columns > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > --- > > drivers/staging/r8188eu/core/rtw_br_ext.c | 20 +++++++++++++------- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c > > index 79daf8f269d6..427da7e8ba4c 100644 > > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c > > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c > > @@ -211,8 +211,10 @@ static int __nat25_network_hash(unsigned char *network_addr) > > } else if (network_addr[0] == NAT25_IPX) { > > unsigned long x; > > > > - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ network_addr[5] ^ > > - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ network_addr[10]; > > + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ > > Why not go out to [4] here and then you are one line shorter? Thank you for the feedback. Arranging 4 on a line still made the line extend 90+ columns. It appeared wide to me. Stacking three in one line made it look better. I will resend the patch with 4 in line and let you suggest if that is acceptable. Thank you, ./drv > > > + network_addr[4] ^ network_addr[5] ^ network_addr[6] ^ > > + network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ > > + network_addr[10]; > > > > return x & (NAT25_HASH_SIZE - 1); > > } else if (network_addr[0] == NAT25_APPLE) { > > @@ -224,16 +226,20 @@ static int __nat25_network_hash(unsigned char *network_addr) > > } else if (network_addr[0] == NAT25_PPPOE) { > > unsigned long x; > > > > - x = network_addr[0] ^ network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ network_addr[5] ^ network_addr[6] ^ network_addr[7] ^ network_addr[8]; > > + x = network_addr[0] ^ network_addr[1] ^ network_addr[2] ^ > > + network_addr[3] ^ network_addr[4] ^ network_addr[5] ^ > > Same here > > > > + network_addr[6] ^ network_addr[7] ^ network_addr[8]; > > > > return x & (NAT25_HASH_SIZE - 1); > > } else if (network_addr[0] == NAT25_IPV6) { > > unsigned long x; > > > > - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ network_addr[5] ^ > > - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ network_addr[10] ^ > > - network_addr[11] ^ network_addr[12] ^ network_addr[13] ^ network_addr[14] ^ network_addr[15] ^ > > - network_addr[16]; > > + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ > > + network_addr[4] ^ network_addr[5] ^ network_addr[6] ^ > > + network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ > > + network_addr[10] ^ network_addr[11] ^ network_addr[12] ^ > > + network_addr[13] ^ network_addr[14] ^ network_addr[15] ^ > > + network_addr[16]; > > And here. > > thanks, > > greg k-h >
On Mon, Oct 17, 2022 at 07:40:46PM +0530, Deepak R Varma wrote: > On Mon, Oct 17, 2022 at 04:09:49PM +0200, Greg KH wrote: > > On Mon, Oct 17, 2022 at 06:52:50PM +0530, Deepak R Varma wrote: > > > Reformat long running computation instructions to improve code readability. > > > Address following checkpatch script complaints: > > > CHECK: line length of 171 exceeds 100 columns > > > CHECK: line length of 113 exceeds 100 columns > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > --- > > > drivers/staging/r8188eu/core/rtw_br_ext.c | 20 +++++++++++++------- > > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c > > > index 79daf8f269d6..427da7e8ba4c 100644 > > > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c > > > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c > > > @@ -211,8 +211,10 @@ static int __nat25_network_hash(unsigned char *network_addr) > > > } else if (network_addr[0] == NAT25_IPX) { > > > unsigned long x; > > > > > > - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ network_addr[5] ^ > > > - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ network_addr[10]; > > > + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ > > > > Why not go out to [4] here and then you are one line shorter? > > Thank you for the feedback. > Arranging 4 on a line still made the line extend 90+ columns. As the tool said, you can go up to 100 columns. thanks, greg k-h
From: Greg KH > Sent: 17 October 2022 15:10 > > On Mon, Oct 17, 2022 at 06:52:50PM +0530, Deepak R Varma wrote: > > Reformat long running computation instructions to improve code readability. > > Address following checkpatch script complaints: > > CHECK: line length of 171 exceeds 100 columns > > CHECK: line length of 113 exceeds 100 columns > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > --- > > drivers/staging/r8188eu/core/rtw_br_ext.c | 20 +++++++++++++------- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c > > index 79daf8f269d6..427da7e8ba4c 100644 > > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c > > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c > > @@ -211,8 +211,10 @@ static int __nat25_network_hash(unsigned char *network_addr) > > } else if (network_addr[0] == NAT25_IPX) { > > unsigned long x; > > > > - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ > network_addr[5] ^ > > - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ > network_addr[10]; > > + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ > > Why not go out to [4] here and then you are one line shorter? and/or use a shorter variable name.... David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Tue, Oct 18, 2022 at 11:21:26AM +0000, David Laight wrote: > From: Greg KH > > Sent: 17 October 2022 15:10 > > > > On Mon, Oct 17, 2022 at 06:52:50PM +0530, Deepak R Varma wrote: > > > Reformat long running computation instructions to improve code readability. > > > Address following checkpatch script complaints: > > > CHECK: line length of 171 exceeds 100 columns > > > CHECK: line length of 113 exceeds 100 columns > > > > > > Signed-off-by: Deepak R Varma <drv@mailo.com> > > > --- > > > drivers/staging/r8188eu/core/rtw_br_ext.c | 20 +++++++++++++------- > > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c > > > index 79daf8f269d6..427da7e8ba4c 100644 > > > --- a/drivers/staging/r8188eu/core/rtw_br_ext.c > > > +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c > > > @@ -211,8 +211,10 @@ static int __nat25_network_hash(unsigned char *network_addr) > > > } else if (network_addr[0] == NAT25_IPX) { > > > unsigned long x; > > > > > > - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ > > network_addr[5] ^ > > > - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ > > network_addr[10]; > > > + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ > > > > Why not go out to [4] here and then you are one line shorter? > > and/or use a shorter variable name.... Hi David, I have already re-submitted the patch set with 4 in line arrangement. Do you still suggest using shorter variable names? Thank you, ./drv > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > >
On Tue, 2022-10-18 at 18:12 +0530, Deepak R Varma wrote: > On Tue, Oct 18, 2022 at 11:21:26AM +0000, David Laight wrote: > > From: Greg KH > > > Sent: 17 October 2022 15:10 > > > > > > On Mon, Oct 17, 2022 at 06:52:50PM +0530, Deepak R Varma wrote: > > > > Reformat long running computation instructions to improve code readability. > > > > Address following checkpatch script complaints: > > > > CHECK: line length of 171 exceeds 100 columns > > > > CHECK: line length of 113 exceeds 100 columns [] > > > > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c [] > > > > @@ -211,8 +211,10 @@ static int __nat25_network_hash(unsigned char *network_addr) > > > > } else if (network_addr[0] == NAT25_IPX) { > > > > unsigned long x; > > > > > > > > - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ > > > network_addr[5] ^ > > > > - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ > > > network_addr[10]; > > > > + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ > > > > > > Why not go out to [4] here and then you are one line shorter? > > > > and/or use a shorter variable name.... > Hi David, > I have already re-submitted the patch set with 4 in line arrangement. Do you > still suggest using shorter variable names? Assuming this code is not performance sensitive, I suggest not just molifying checkpatch but perhaps improving the code by adding a helper function something like: u8 xor_array_u8(u8 *x, size_t len) { size_t i; u8 xor = x[0]; for (i = 1; i < len; i++) xor ^= x[i]; return xor; } so for instance this could be: x = xor_array_u8(&network_addr[1], 10);
On Tue, Oct 18, 2022 at 10:43:07PM -0700, Joe Perches wrote: > On Tue, 2022-10-18 at 18:12 +0530, Deepak R Varma wrote: > > On Tue, Oct 18, 2022 at 11:21:26AM +0000, David Laight wrote: > > > From: Greg KH > > > > Sent: 17 October 2022 15:10 > > > > > > > > On Mon, Oct 17, 2022 at 06:52:50PM +0530, Deepak R Varma wrote: > > > > > Reformat long running computation instructions to improve code readability. > > > > > Address following checkpatch script complaints: > > > > > CHECK: line length of 171 exceeds 100 columns > > > > > CHECK: line length of 113 exceeds 100 columns > [] > > > > > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c > [] > > > > > @@ -211,8 +211,10 @@ static int __nat25_network_hash(unsigned char *network_addr) > > > > > } else if (network_addr[0] == NAT25_IPX) { > > > > > unsigned long x; > > > > > > > > > > - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ > > > > network_addr[5] ^ > > > > > - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ > > > > network_addr[10]; > > > > > + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ > > > > > > > > Why not go out to [4] here and then you are one line shorter? > > > > > > and/or use a shorter variable name.... > > Hi David, > > I have already re-submitted the patch set with 4 in line arrangement. Do you > > still suggest using shorter variable names? > > Assuming this code is not performance sensitive, I suggest not just > molifying checkpatch but perhaps improving the code by adding a helper > function something like: > > u8 xor_array_u8(u8 *x, size_t len) > { > size_t i; > u8 xor = x[0]; > > for (i = 1; i < len; i++) > xor ^= x[i]; > > return xor; > } > > so for instance this could be: > > x = xor_array_u8(&network_addr[1], 10); > Hi Joe, Great suggestion. Thank you. Is there a way to confirm that this improvement won't impact performance? Will I need any specific hardware / device to run tests? ./drv
On Wed, 2022-10-19 at 11:47 +0530, Deepak R Varma wrote: > On Tue, Oct 18, 2022 at 10:43:07PM -0700, Joe Perches wrote: > > On Tue, 2022-10-18 at 18:12 +0530, Deepak R Varma wrote: > > > On Tue, Oct 18, 2022 at 11:21:26AM +0000, David Laight wrote: > > > > From: Greg KH > > > > > Sent: 17 October 2022 15:10 > > > > > > > > > > On Mon, Oct 17, 2022 at 06:52:50PM +0530, Deepak R Varma wrote: > > > > > > Reformat long running computation instructions to improve code readability. > > > > > > Address following checkpatch script complaints: > > > > > > CHECK: line length of 171 exceeds 100 columns > > > > > > CHECK: line length of 113 exceeds 100 columns > > [] > > > > > > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c > > [] > > > > > > @@ -211,8 +211,10 @@ static int __nat25_network_hash(unsigned char *network_addr) > > > > > > } else if (network_addr[0] == NAT25_IPX) { > > > > > > unsigned long x; > > > > > > > > > > > > - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ > > > > > network_addr[5] ^ > > > > > > - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ > > > > > network_addr[10]; > > > > > > + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ > > > > > > > > > > Why not go out to [4] here and then you are one line shorter? > > > > > > > > and/or use a shorter variable name.... > > > Hi David, > > > I have already re-submitted the patch set with 4 in line arrangement. Do you > > > still suggest using shorter variable names? > > > > Assuming this code is not performance sensitive, I suggest not just > > molifying checkpatch but perhaps improving the code by adding a helper > > function something like: > > > > u8 xor_array_u8(u8 *x, size_t len) > > { > > size_t i; > > u8 xor = x[0]; > > > > for (i = 1; i < len; i++) > > xor ^= x[i]; > > > > return xor; > > } > > > > so for instance this could be: > > > > x = xor_array_u8(&network_addr[1], 10); > > > > Hi Joe, > Great suggestion. Thank you. > Is there a way to confirm that this improvement won't impact performance? Will I > need any specific hardware / device to run tests? I suggest reading the code to see if the uses are in some fast path.
On Tue, Oct 18, 2022 at 11:38:22PM -0700, Joe Perches wrote: > On Wed, 2022-10-19 at 11:47 +0530, Deepak R Varma wrote: > > On Tue, Oct 18, 2022 at 10:43:07PM -0700, Joe Perches wrote: > > > On Tue, 2022-10-18 at 18:12 +0530, Deepak R Varma wrote: > > > > On Tue, Oct 18, 2022 at 11:21:26AM +0000, David Laight wrote: > > > > > From: Greg KH > > > > > > Sent: 17 October 2022 15:10 > > > > > > > > > > > > On Mon, Oct 17, 2022 at 06:52:50PM +0530, Deepak R Varma wrote: > > > > > > > Reformat long running computation instructions to improve code readability. > > > > > > > Address following checkpatch script complaints: > > > > > > > CHECK: line length of 171 exceeds 100 columns > > > > > > > CHECK: line length of 113 exceeds 100 columns > > > [] > > > > > > > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c > > > [] > > > > > > > @@ -211,8 +211,10 @@ static int __nat25_network_hash(unsigned char *network_addr) > > > > > > > } else if (network_addr[0] == NAT25_IPX) { > > > > > > > unsigned long x; > > > > > > > > > > > > > > - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ > > > > > > network_addr[5] ^ > > > > > > > - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ > > > > > > network_addr[10]; > > > > > > > + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ > > > > > > > > > > > > Why not go out to [4] here and then you are one line shorter? > > > > > > > > > > and/or use a shorter variable name.... > > > > Hi David, > > > > I have already re-submitted the patch set with 4 in line arrangement. Do you > > > > still suggest using shorter variable names? > > > > > > Assuming this code is not performance sensitive, I suggest not just > > > molifying checkpatch but perhaps improving the code by adding a helper > > > function something like: > > > > > > u8 xor_array_u8(u8 *x, size_t len) > > > { > > > size_t i; > > > u8 xor = x[0]; > > > > > > for (i = 1; i < len; i++) > > > xor ^= x[i]; > > > > > > return xor; > > > } > > > > > > so for instance this could be: > > > > > > x = xor_array_u8(&network_addr[1], 10); > > > > > > > Hi Joe, > > Great suggestion. Thank you. > > Is there a way to confirm that this improvement won't impact performance? Will I > > need any specific hardware / device to run tests? > > I suggest reading the code to see if the uses are in some fast path. Sounds good. Thank you for your guidance. >
On Wed, Oct 19, 2022 at 12:14:38PM +0530, Deepak R Varma wrote: > On Tue, Oct 18, 2022 at 11:38:22PM -0700, Joe Perches wrote: > > On Wed, 2022-10-19 at 11:47 +0530, Deepak R Varma wrote: > > > On Tue, Oct 18, 2022 at 10:43:07PM -0700, Joe Perches wrote: > > > > On Tue, 2022-10-18 at 18:12 +0530, Deepak R Varma wrote: > > > > > On Tue, Oct 18, 2022 at 11:21:26AM +0000, David Laight wrote: > > > > > > From: Greg KH > > > > > > > Sent: 17 October 2022 15:10 > > > > > > > > > > > > > > On Mon, Oct 17, 2022 at 06:52:50PM +0530, Deepak R Varma wrote: > > > > > > > > Reformat long running computation instructions to improve code readability. > > > > > > > > Address following checkpatch script complaints: > > > > > > > > CHECK: line length of 171 exceeds 100 columns > > > > > > > > CHECK: line length of 113 exceeds 100 columns > > > > [] > > > > > > > > diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c > > > > [] > > > > > > > > @@ -211,8 +211,10 @@ static int __nat25_network_hash(unsigned char *network_addr) > > > > > > > > } else if (network_addr[0] == NAT25_IPX) { > > > > > > > > unsigned long x; > > > > > > > > > > > > > > > > - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ > > > > > > > network_addr[5] ^ > > > > > > > > - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ > > > > > > > network_addr[10]; > > > > > > > > + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ > > > > > > > > > > > > > > Why not go out to [4] here and then you are one line shorter? > > > > > > > > > > > > and/or use a shorter variable name.... > > > > > Hi David, > > > > > I have already re-submitted the patch set with 4 in line arrangement. Do you > > > > > still suggest using shorter variable names? > > > > > > > > Assuming this code is not performance sensitive, I suggest not just > > > > molifying checkpatch but perhaps improving the code by adding a helper > > > > function something like: > > > > > > > > u8 xor_array_u8(u8 *x, size_t len) > > > > { > > > > size_t i; > > > > u8 xor = x[0]; > > > > > > > > for (i = 1; i < len; i++) > > > > xor ^= x[i]; > > > > > > > > return xor; > > > > } > > > > > > > > so for instance this could be: > > > > > > > > x = xor_array_u8(&network_addr[1], 10); > > > > > > > > > > Hi Joe, > > > Great suggestion. Thank you. > > > Is there a way to confirm that this improvement won't impact performance? Will I > > > need any specific hardware / device to run tests? > > > > I suggest reading the code to see if the uses are in some fast path. > > Sounds good. Thank you for your guidance. Hi Joe, based on the code review so far, I am unable to determine if the chain of function calls are part of any fast path. There is not enough code comments or documentation available with this code. Considering my Outreachy patch submission targets and timelines, I am unable to spend much time on this research right now; unless an expert can confirm it is okay to add the routine you outlined. Else, I will put this in on my TODO list and revisit when I have time. R8188EU maintainers / experts, Can you confirm if it is sensible to implement the helper function suggested by Joe? If yes, I will include the improvement in my current patch set and resubmit the set for review. Thank you, ./drv > > > > > >
diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c index 79daf8f269d6..427da7e8ba4c 100644 --- a/drivers/staging/r8188eu/core/rtw_br_ext.c +++ b/drivers/staging/r8188eu/core/rtw_br_ext.c @@ -211,8 +211,10 @@ static int __nat25_network_hash(unsigned char *network_addr) } else if (network_addr[0] == NAT25_IPX) { unsigned long x; - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ network_addr[5] ^ - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ network_addr[10]; + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ + network_addr[4] ^ network_addr[5] ^ network_addr[6] ^ + network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ + network_addr[10]; return x & (NAT25_HASH_SIZE - 1); } else if (network_addr[0] == NAT25_APPLE) { @@ -224,16 +226,20 @@ static int __nat25_network_hash(unsigned char *network_addr) } else if (network_addr[0] == NAT25_PPPOE) { unsigned long x; - x = network_addr[0] ^ network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ network_addr[5] ^ network_addr[6] ^ network_addr[7] ^ network_addr[8]; + x = network_addr[0] ^ network_addr[1] ^ network_addr[2] ^ + network_addr[3] ^ network_addr[4] ^ network_addr[5] ^ + network_addr[6] ^ network_addr[7] ^ network_addr[8]; return x & (NAT25_HASH_SIZE - 1); } else if (network_addr[0] == NAT25_IPV6) { unsigned long x; - x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ network_addr[4] ^ network_addr[5] ^ - network_addr[6] ^ network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ network_addr[10] ^ - network_addr[11] ^ network_addr[12] ^ network_addr[13] ^ network_addr[14] ^ network_addr[15] ^ - network_addr[16]; + x = network_addr[1] ^ network_addr[2] ^ network_addr[3] ^ + network_addr[4] ^ network_addr[5] ^ network_addr[6] ^ + network_addr[7] ^ network_addr[8] ^ network_addr[9] ^ + network_addr[10] ^ network_addr[11] ^ network_addr[12] ^ + network_addr[13] ^ network_addr[14] ^ network_addr[15] ^ + network_addr[16]; return x & (NAT25_HASH_SIZE - 1); } else {