[PATCH v2] staging: octeon: Fix checkpatch warnings

[PATCH v2] staging: octeon: Fix checkpatch warnings

[Luis de Bethencourt]

Fixing 80 character limit warnings in octeon/ethernet-rx.c

Signed-off-by: Luis de Bethencourt <luis@debethencourt.com>
---
 drivers/staging/octeon/ethernet-rx.c | 51 +++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
index 1789a12..e387eb1 100644
--- a/drivers/staging/octeon/ethernet-rx.c
+++ b/drivers/staging/octeon/ethernet-rx.c
@@ -109,6 +109,7 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
 		int interface = cvmx_helper_get_interface_num(work->ipprt);
 		int index = cvmx_helper_get_interface_index_num(work->ipprt);
 		union cvmx_gmxx_rxx_frm_ctl gmxx_rxx_frm_ctl;
+
 		gmxx_rxx_frm_ctl.u64 =
 		    cvmx_read_csr(CVMX_GMXX_RXX_FRM_CTL(index, interface));
 		if (gmxx_rxx_frm_ctl.s.pre_chk == 0) {
@@ -126,13 +127,15 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
 
 			if (*ptr == 0xd5) {
 				/*
-				  printk_ratelimited("Port %d received 0xd5 preamble\n", work->ipprt);
+				  printk_ratelimited("Port %d received 0xd5 preamble\n",
+					  work->ipprt);
 				 */
 				work->packet_ptr.s.addr += i + 1;
 				work->len -= i + 5;
 			} else if ((*ptr & 0xf) == 0xd) {
 				/*
-				  printk_ratelimited("Port %d received 0x?d preamble\n", work->ipprt);
+				  printk_ratelimited("Port %d received 0x?d preamble\n",
+					  work->ipprt);
 				 */
 				work->packet_ptr.s.addr += i;
 				work->len -= i + 4;
@@ -212,17 +215,20 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 		did_work_request = 0;
 		if (work == NULL) {
 			union cvmx_pow_wq_int wq_int;
+
 			wq_int.u64 = 0;
 			wq_int.s.iq_dis = 1 << pow_receive_group;
 			wq_int.s.wq_int = 1 << pow_receive_group;
 			cvmx_write_csr(CVMX_POW_WQ_INT, wq_int.u64);
 			break;
 		}
-		pskb = (struct sk_buff **)(cvm_oct_get_buffer_ptr(work->packet_ptr) - sizeof(void *));
+		pskb = (struct sk_buff **)(cvm_oct_get_buffer_ptr(work->packet_ptr) -
+			sizeof(void *));
 		prefetch(pskb);
 
 		if (USE_ASYNC_IOBDMA && rx_count < (budget - 1)) {
-			cvmx_pow_work_request_async_nocheck(CVMX_SCR_SCRATCH, CVMX_POW_NO_WAIT);
+			cvmx_pow_work_request_async_nocheck(CVMX_SCR_SCRATCH,
+					CVMX_POW_NO_WAIT);
 			did_work_request = 1;
 		}
 		rx_count++;
@@ -247,7 +253,8 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 		 * buffer.
 		 */
 		if (likely(skb_in_hw)) {
-			skb->data = skb->head + work->packet_ptr.s.addr - cvmx_ptr_to_phys(skb->head);
+			skb->data = skb->head + work->packet_ptr.s.addr -
+				cvmx_ptr_to_phys(skb->head);
 			prefetch(skb->data);
 			skb->len = work->len;
 			skb_set_tail_pointer(skb, skb->len);
@@ -284,7 +291,8 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 				/* No packet buffers to free */
 			} else {
 				int segments = work->word2.s.bufs;
-				union cvmx_buf_ptr segment_ptr = work->packet_ptr;
+				union cvmx_buf_ptr segment_ptr =
+				    work->packet_ptr;
 				int len = work->len;
 
 				while (segments--) {
@@ -300,8 +308,11 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 			 * one: int segment_size =
 			 * segment_ptr.s.size;
 			 */
-					int segment_size = CVMX_FPA_PACKET_POOL_SIZE -
-						(segment_ptr.s.addr - (((segment_ptr.s.addr >> 7) - segment_ptr.s.back) << 7));
+					int segment_size =
+					    CVMX_FPA_PACKET_POOL_SIZE -
+					    (segment_ptr.s.addr -
+					    (((segment_ptr.s.addr >> 7) -
+					    segment_ptr.s.back) << 7));
 					/*
 					 * Don't copy more than what
 					 * is left in the packet.
@@ -332,8 +343,10 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 				skb->protocol = eth_type_trans(skb, dev);
 				skb->dev = dev;
 
-				if (unlikely(work->word2.s.not_IP || work->word2.s.IP_exc ||
-					work->word2.s.L4_error || !work->word2.s.tcp_or_udp))
+				if (unlikely(work->word2.s.not_IP ||
+					work->word2.s.IP_exc ||
+					work->word2.s.L4_error ||
+					!work->word2.s.tcp_or_udp))
 					skb->ip_summed = CHECKSUM_NONE;
 				else
 					skb->ip_summed = CHECKSUM_UNNECESSARY;
@@ -341,11 +354,15 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 				/* Increment RX stats for virtual ports */
 				if (work->ipprt >= CVMX_PIP_NUM_INPUT_PORTS) {
 #ifdef CONFIG_64BIT
-					atomic64_add(1, (atomic64_t *)&priv->stats.rx_packets);
-					atomic64_add(skb->len, (atomic64_t *)&priv->stats.rx_bytes);
+					atomic64_add(1,
+						(atomic64_t *)&priv->stats.rx_packets);
+					atomic64_add(skb->len,
+						(atomic64_t *)&priv->stats.rx_bytes);
 #else
-					atomic_add(1, (atomic_t *)&priv->stats.rx_packets);
-					atomic_add(skb->len, (atomic_t *)&priv->stats.rx_bytes);
+					atomic_add(1,
+						(atomic_t *)&priv->stats.rx_packets);
+					atomic_add(skb->len,
+						(atomic_t *)&priv->stats.rx_bytes);
 #endif
 				}
 				netif_receive_skb(skb);
@@ -356,9 +373,11 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 					   dev->name);
 				*/
 #ifdef CONFIG_64BIT
-				atomic64_add(1, (atomic64_t *)&priv->stats.rx_dropped);
+				atomic64_add(1,
+				    (atomic64_t *)&priv->stats.rx_dropped);
 #else
-				atomic_add(1, (atomic_t *)&priv->stats.rx_dropped);
+				atomic_add(1,
+				    (atomic_t *)&priv->stats.rx_dropped);
 #endif
 				dev_kfree_skb_irq(skb);
 			}
-- 
2.1.3

Re: [PATCH v2] staging: octeon: Fix checkpatch warnings

[Joe Perches]

On Thu, 2014-11-27 at 17:18 +0100, Luis de Bethencourt wrote:
> Fixing 80 character limit warnings in octeon/ethernet-rx.c

Hello again Luis.

Another thing you might consider is to  align
multiple line statements to the appropriate
open parenthesis.

> diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
[]
> @@ -126,13 +127,15 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
>  
>  			if (*ptr == 0xd5) {
>  				/*
> -				  printk_ratelimited("Port %d received 0xd5 preamble\n", work->ipprt);
> +				  printk_ratelimited("Port %d received 0xd5 preamble\n",
> +					  work->ipprt);
>  				 */

This is in a commented out block, but this
would look better like:

		printk_ratelimited("format",
				   args...);


> @@ -212,17 +215,20 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
>  		did_work_request = 0;
>  		if (work == NULL) {
>  			union cvmx_pow_wq_int wq_int;
> +
>  			wq_int.u64 = 0;
>  			wq_int.s.iq_dis = 1 << pow_receive_group;
>  			wq_int.s.wq_int = 1 << pow_receive_group;
>  			cvmx_write_csr(CVMX_POW_WQ_INT, wq_int.u64);
>  			break;
>  		}
> -		pskb = (struct sk_buff **)(cvm_oct_get_buffer_ptr(work->packet_ptr) - sizeof(void *));
> +		pskb = (struct sk_buff **)(cvm_oct_get_buffer_ptr(work->packet_ptr) -
> +			sizeof(void *));

cvm_oct_get_buffer_ptr returns a void pointer
so it doesn't need a cast.

a possible fix is just to remove the cast

		pskb = cvm_oct_get_buffer_ptr(work->packet_ptr) - sizeof(void *);

>  		prefetch(pskb);

If the cast type above to a ** is to be believed,
maybe this should be prefetch(*pskb)

If you do these, they should be separate patches.

Re: [PATCH v2] staging: octeon: Fix checkpatch warnings

[Luis de Bethencourt]

On Thu, Nov 27, 2014 at 08:34:00AM -0800, Joe Perches wrote:
> On Thu, 2014-11-27 at 17:18 +0100, Luis de Bethencourt wrote:
> > Fixing 80 character limit warnings in octeon/ethernet-rx.c
> 
> Hello again Luis.
> 
> Another thing you might consider is to  align
> multiple line statements to the appropriate
> open parenthesis.

Sure :) I will do this and merge it into the patch in a few hours.

> 
> > diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
> []
> > @@ -126,13 +127,15 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
> >  
> >  			if (*ptr == 0xd5) {
> >  				/*
> > -				  printk_ratelimited("Port %d received 0xd5 preamble\n", work->ipprt);
> > +				  printk_ratelimited("Port %d received 0xd5 preamble\n",
> > +					  work->ipprt);
> >  				 */
> 
> This is in a commented out block, but this
> would look better like:
> 
> 		printk_ratelimited("format",
> 				   args...);
> 

I agree. Thanks for the pointer.

> 
> > @@ -212,17 +215,20 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
> >  		did_work_request = 0;
> >  		if (work == NULL) {
> >  			union cvmx_pow_wq_int wq_int;
> > +
> >  			wq_int.u64 = 0;
> >  			wq_int.s.iq_dis = 1 << pow_receive_group;
> >  			wq_int.s.wq_int = 1 << pow_receive_group;
> >  			cvmx_write_csr(CVMX_POW_WQ_INT, wq_int.u64);
> >  			break;
> >  		}
> > -		pskb = (struct sk_buff **)(cvm_oct_get_buffer_ptr(work->packet_ptr) - sizeof(void *));
> > +		pskb = (struct sk_buff **)(cvm_oct_get_buffer_ptr(work->packet_ptr) -
> > +			sizeof(void *));
> 
> cvm_oct_get_buffer_ptr returns a void pointer
> so it doesn't need a cast.
> 
> a possible fix is just to remove the cast
> 
> 		pskb = cvm_oct_get_buffer_ptr(work->packet_ptr) - sizeof(void *);
> 
> >  		prefetch(pskb);
> 
> If the cast type above to a ** is to be believed,
> maybe this should be prefetch(*pskb)
> 
> If you do these, they should be separate patches.
> 

Cool! Once I do the above I will fix this in a separate patch.

Thanks again for your time reviewing Joe,
Luis

Re: [PATCH v2] staging: octeon: Fix checkpatch warnings

[Dan Carpenter]

On Thu, Nov 27, 2014 at 05:18:10PM +0100, Luis de Bethencourt wrote:
> Fixing 80 character limit warnings in octeon/ethernet-rx.c
> 
> Signed-off-by: Luis de Bethencourt <luis@debethencourt.com>
> ---
>  drivers/staging/octeon/ethernet-rx.c | 51 +++++++++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
> index 1789a12..e387eb1 100644
> --- a/drivers/staging/octeon/ethernet-rx.c
> +++ b/drivers/staging/octeon/ethernet-rx.c
> @@ -109,6 +109,7 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
>  		int interface = cvmx_helper_get_interface_num(work->ipprt);
>  		int index = cvmx_helper_get_interface_index_num(work->ipprt);
>  		union cvmx_gmxx_rxx_frm_ctl gmxx_rxx_frm_ctl;
> +

Do this in a separate patch.

> @@ -332,8 +343,10 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
>  				skb->protocol = eth_type_trans(skb, dev);
>  				skb->dev = dev;
>  
> -				if (unlikely(work->word2.s.not_IP || work->word2.s.IP_exc ||
> -					work->word2.s.L4_error || !work->word2.s.tcp_or_udp))
> +				if (unlikely(work->word2.s.not_IP ||
> +					work->word2.s.IP_exc ||
> +					work->word2.s.L4_error ||
> +					!work->word2.s.tcp_or_udp))
>  					skb->ip_summed = CHECKSUM_NONE;

Your patch aligns everything with tabs but it should be slightly
different.  Run your patch through checkpatch.pl --strict to see what I
mean and then redo it all.  This one should look like:

				if (unlikely(work->word2.s.not_IP ||
					     work->word2.s.IP_exc ||
					     work->word2.s.L4_error ||
					     !work->word2.s.tcp_or_udp))
					skb->ip_summed = CHECKSUM_NONE;

[tab][tab][tab][tab][tab][space][space][space][space][space]work->...

regards,
dan carpenter

Re: [PATCH v2] staging: octeon: Fix checkpatch warnings

[Luis de Bethencourt]

On Fri, Nov 28, 2014 at 01:29:28PM +0300, Dan Carpenter wrote:
> On Thu, Nov 27, 2014 at 05:18:10PM +0100, Luis de Bethencourt wrote:
> > Fixing 80 character limit warnings in octeon/ethernet-rx.c
> > 
> > Signed-off-by: Luis de Bethencourt <luis@debethencourt.com>
> > ---
> >  drivers/staging/octeon/ethernet-rx.c | 51 +++++++++++++++++++++++++-----------
> >  1 file changed, 35 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
> > index 1789a12..e387eb1 100644
> > --- a/drivers/staging/octeon/ethernet-rx.c
> > +++ b/drivers/staging/octeon/ethernet-rx.c
> > @@ -109,6 +109,7 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
> >  		int interface = cvmx_helper_get_interface_num(work->ipprt);
> >  		int index = cvmx_helper_get_interface_index_num(work->ipprt);
> >  		union cvmx_gmxx_rxx_frm_ctl gmxx_rxx_frm_ctl;
> > +
> 
> Do this in a separate patch.

Will do within the hour.

> 
> > @@ -332,8 +343,10 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
> >  				skb->protocol = eth_type_trans(skb, dev);
> >  				skb->dev = dev;
> >  
> > -				if (unlikely(work->word2.s.not_IP || work->word2.s.IP_exc ||
> > -					work->word2.s.L4_error || !work->word2.s.tcp_or_udp))
> > +				if (unlikely(work->word2.s.not_IP ||
> > +					work->word2.s.IP_exc ||
> > +					work->word2.s.L4_error ||
> > +					!work->word2.s.tcp_or_udp))
> >  					skb->ip_summed = CHECKSUM_NONE;
> 
> Your patch aligns everything with tabs but it should be slightly
> different.  Run your patch through checkpatch.pl --strict to see what I
> mean and then redo it all.  This one should look like:
> 
> 				if (unlikely(work->word2.s.not_IP ||
> 					     work->word2.s.IP_exc ||
> 					     work->word2.s.L4_error ||
> 					     !work->word2.s.tcp_or_udp))
> 					skb->ip_summed = CHECKSUM_NONE;
> 
> [tab][tab][tab][tab][tab][space][space][space][space][space]work->...
> 
> regards,
> dan carpenter
> 
> 

Fixed the aligns and resubmitting the patch as v3.

Thanks for the review!
Luis

Re: [PATCH v2] staging: octeon: Fix checkpatch warnings

[Dan Carpenter]

On Fri, Nov 28, 2014 at 02:30:34PM +0100, Luis de Bethencourt wrote:
> On Fri, Nov 28, 2014 at 01:29:28PM +0300, Dan Carpenter wrote:
> > On Thu, Nov 27, 2014 at 05:18:10PM +0100, Luis de Bethencourt wrote:
> > > Fixing 80 character limit warnings in octeon/ethernet-rx.c
> > > 
> > > Signed-off-by: Luis de Bethencourt <luis@debethencourt.com>
> > > ---
> > >  drivers/staging/octeon/ethernet-rx.c | 51 +++++++++++++++++++++++++-----------
> > >  1 file changed, 35 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
> > > index 1789a12..e387eb1 100644
> > > --- a/drivers/staging/octeon/ethernet-rx.c
> > > +++ b/drivers/staging/octeon/ethernet-rx.c
> > > @@ -109,6 +109,7 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
> > >  		int interface = cvmx_helper_get_interface_num(work->ipprt);
> > >  		int index = cvmx_helper_get_interface_index_num(work->ipprt);
> > >  		union cvmx_gmxx_rxx_frm_ctl gmxx_rxx_frm_ctl;
> > > +
> > 
> > Do this in a separate patch.
> 
> Will do within the hour.
> 

:(  You should always wait overnight or a few hours before sending a
patch.  It's not a race.  Go slowly.

regards,
dan carpenter

Re: [PATCH v2] staging: octeon: Fix checkpatch warnings

[Luis de Bethencourt]

On Fri, Nov 28, 2014 at 05:14:32PM +0300, Dan Carpenter wrote:
> On Fri, Nov 28, 2014 at 02:30:34PM +0100, Luis de Bethencourt wrote:
> > On Fri, Nov 28, 2014 at 01:29:28PM +0300, Dan Carpenter wrote:
> > > On Thu, Nov 27, 2014 at 05:18:10PM +0100, Luis de Bethencourt wrote:
> > > > Fixing 80 character limit warnings in octeon/ethernet-rx.c
> > > > 
> > > > Signed-off-by: Luis de Bethencourt <luis@debethencourt.com>
> > > > ---
> > > >  drivers/staging/octeon/ethernet-rx.c | 51 +++++++++++++++++++++++++-----------
> > > >  1 file changed, 35 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
> > > > index 1789a12..e387eb1 100644
> > > > --- a/drivers/staging/octeon/ethernet-rx.c
> > > > +++ b/drivers/staging/octeon/ethernet-rx.c
> > > > @@ -109,6 +109,7 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
> > > >  		int interface = cvmx_helper_get_interface_num(work->ipprt);
> > > >  		int index = cvmx_helper_get_interface_index_num(work->ipprt);
> > > >  		union cvmx_gmxx_rxx_frm_ctl gmxx_rxx_frm_ctl;
> > > > +
> > > 
> > > Do this in a separate patch.
> > 
> > Will do within the hour.
> > 
> 
> :(  You should always wait overnight or a few hours before sending a
> patch.  It's not a race.  Go slowly.
> 
> regards,
> dan carpenter
> 

Is the wait so people have more time to review the patch as it is?
I wanted to show my interest by keeping up with reviews but now that you
mention that, it makes sense.

I screwed up and didn't submit the version 3 of this patch with [01/02] for the
character limit warnings. Will wait, since the patch for the empty lines after
variable declarations needs to be applied on-top of this. I will wait until that
one is accepted or rejected before rebasing and submitting the second patch.

Sorry about this.

Thanks for all the help :)

Luis