linux-kernel - Re: [PATCH] staging:rtl8712:xmit

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

Message-ID: <1503709036.12569.36.camel@perches.com>
Date:   Fri, 25 Aug 2017 17:57:16 -0700
From:   Joe Perches <joe@...ches.com>
To:     Harsha Sharma <harshasharmaiitr@...il.com>
Cc:     gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging:rtl8712:xmit_linux.c: Avoid CamelCase

On Sat, 2017-08-26 at 01:21 +0530, Harsha Sharma wrote:
> Hello,
> 
> Sorry, this was my first contribution in linux-kernel. I will take care
> about this from next time.
> Do I need to send these patches once again one by one?

No, just pick the "best" one of what you sent
without any of the "style" modifications like
the 80 column adjustments and see what happens.

Read the code outside of staging for awhile
to get a feel for what style seems appropriate
before you send style-only patches.

For instance, this proposed change:

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
> index 0b39676..57fc65f 100644
> --- a/drivers/staging/unisys/visornic/visornic_main.c
> +++ b/drivers/staging/unisys/visornic/visornic_main.c
> @@ -253,10 +253,9 @@ static int visor_copy_fragsinfo_from_skb(struct sk_buff *skb,
>  		for (frag = 0; frag < numfrags; frag++) {
>  			count = add_physinfo_entries(page_to_pfn(
>  				skb_frag_page(&skb_shinfo(skb)->frags[frag])),
> -					      skb_shinfo(skb)->frags[frag].
> -					      page_offset,
> -					      skb_shinfo(skb)->frags[frag].
> -					      size, count, frags_max, frags);
> +				skb_shinfo(skb)->frags[frag].page_offset,
> +				skb_shinfo(skb)->frags[frag].size,
> +				count, frags_max, frags);
>  			/* add_physinfo_entries only returns
>  			 * zero if the frags array is out of room
>  			 * That should never happen because we

while it's nice that you put the multi-line dereferences
together, it's not nice that you change the indentation.

Sometimes it's simply better to exceed 80 columns.
But here, it'd probably be nicer to use temporaries.

Something like:
---
 drivers/staging/unisys/visornic/visornic_main.c | 26 +++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/unisys/visornic/visornic_main.c b/drivers/staging/unisys/visornic/visornic_main.c
index 2891622eef18..aafd849dc1e8 100644
--- a/drivers/staging/unisys/visornic/visornic_main.c
+++ b/drivers/staging/unisys/visornic/visornic_main.c
@@ -195,7 +195,7 @@ visor_copy_fragsinfo_from_skb(struct sk_buff *skb, unsigned int firstfraglen,
 			      unsigned int frags_max,
 			      struct phys_info frags[])
 {
-	unsigned int count = 0, frag, size, offset = 0, numfrags;
+	unsigned int count = 0, size, offset = 0, numfrags;
 	unsigned int total_count;
 
 	numfrags = skb_shinfo(skb)->nr_frags;
@@ -234,21 +234,23 @@ visor_copy_fragsinfo_from_skb(struct sk_buff *skb, unsigned int firstfraglen,
 		count++;
 	}
 	if (numfrags) {
+		int i;
+
 		if ((count + numfrags) > frags_max)
 			return -EINVAL;
 
-		for (frag = 0; frag < numfrags; frag++) {
-			count = add_physinfo_entries(page_to_pfn(
-				skb_frag_page(&skb_shinfo(skb)->frags[frag])),
-					      skb_shinfo(skb)->frags[frag].
-					      page_offset,
-					      skb_shinfo(skb)->frags[frag].
-					      size, count, frags_max, frags);
-			/* add_physinfo_entries only returns
-			 * zero if the frags array is out of room
-			 * That should never happen because we
-			 * fail above, if count+numfrags > frags_max.
+		for (i = 0; i < numfrags; i++) {
+			skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
+			unsigned long pfn = page_to_pfn(skb_frag_page(frag));
+
+			/* add_physinfo_entries only returns zero if the
+			 * frags array is out of room.
+			 * That should never happen because we fail above,
+			 * if count+numfrags > frags_max.
 			 */
+			count = add_physinfo_entries(pfn, frag->page_offset,
+						     frag->size, count,
+						     frags_max, frags);
 			if (!count)
 				return -EINVAL;
 		}