Address different kinds of checkpatch complains for the staging/r8188eu module.
The patches are required to be applied in sequence.
Deepak R Varma (4):
staging: r8188eu: use Linux kernel variable naming convention
staging: r8188eu: reformat long computation lines
staging: r8188eu: remove {} for single statement blocks
staging: r8188eu: use htons macro instead of __constant_htons
drivers/staging/r8188eu/core/rtw_br_ext.c | 125 +++++++++++-----------
1 file changed, 65 insertions(+), 60 deletions(-)
--
2.30.2
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 <[email protected]>
---
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] ^
+ 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 {
--
2.30.2
As per the Linux kernel coding-style guidelines, there is no need to
use {} for single statement blocks. Address following checkpatch script
complaint:
WARNING: braces {} are not necessary for single statement blocks
Signed-off-by: Deepak R Varma <[email protected]>
---
drivers/staging/r8188eu/core/rtw_br_ext.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/r8188eu/core/rtw_br_ext.c b/drivers/staging/r8188eu/core/rtw_br_ext.c
index 427da7e8ba4c..290affe50d0b 100644
--- a/drivers/staging/r8188eu/core/rtw_br_ext.c
+++ b/drivers/staging/r8188eu/core/rtw_br_ext.c
@@ -655,9 +655,8 @@ void *scdb_findEntry(struct adapter *priv, unsigned char *ip_addr)
hash = __nat25_network_hash(network_addr);
db = priv->nethash[hash];
while (db) {
- if (!memcmp(db->networkAddr, network_addr, MAX_NETWORK_ADDR_LEN)) {
+ if (!memcmp(db->networkAddr, network_addr, MAX_NETWORK_ADDR_LEN))
return (void *)db;
- }
db = db->next_hash;
}
--
2.30.2
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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > > ---
> > > 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 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 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, 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
>
> >
>
>
>