[PATCH] staging: octeon: fixed few coding style warnings

[PATCH] staging: octeon: fixed few coding style warnings

[Sakshi Bansal]

Fixed allignment issues and line over 80 characters

Signed-off-by: Sakshi Bansal <sakshi.april5@gmail.com>
---
 drivers/staging/octeon/ethernet-defines.h |  2 +-
 drivers/staging/octeon/ethernet-mdio.c    |  2 +-
 drivers/staging/octeon/ethernet-mem.c     |  4 ++--
 drivers/staging/octeon/ethernet-rgmii.c   |  7 +++----
 drivers/staging/octeon/ethernet-rx.c      | 27 +++++++++++++--------------
 drivers/staging/octeon/ethernet-rx.h      |  2 +-
 drivers/staging/octeon/ethernet-spi.c     |  2 --
 drivers/staging/octeon/ethernet-tx.c      | 26 ++++++++++++--------------
 drivers/staging/octeon/ethernet-xaui.c    |  2 +-
 drivers/staging/octeon/ethernet.c         |  2 +-
 10 files changed, 35 insertions(+), 41 deletions(-)

diff --git a/drivers/staging/octeon/ethernet-defines.h b/drivers/staging/octeon/ethernet-defines.h
index 7164005..c86d905 100644
--- a/drivers/staging/octeon/ethernet-defines.h
+++ b/drivers/staging/octeon/ethernet-defines.h
@@ -40,7 +40,7 @@
 #define FAU_TOTAL_TX_TO_CLEAN (CVMX_FAU_REG_END - sizeof(u32))
 #define FAU_NUM_PACKET_BUFFERS_TO_FREE (FAU_TOTAL_TX_TO_CLEAN - sizeof(u32))
 
-#define TOTAL_NUMBER_OF_PORTS       (CVMX_PIP_NUM_INPUT_PORTS+1)
+#define TOTAL_NUMBER_OF_PORTS       (CVMX_PIP_NUM_INPUT_PORTS + 1)
 
 
 #endif /* __ETHERNET_DEFINES_H__ */
diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c
index fd9b3d8..590a6cb 100644
--- a/drivers/staging/octeon/ethernet-mdio.c
+++ b/drivers/staging/octeon/ethernet-mdio.c
@@ -180,7 +180,7 @@ int cvm_oct_phy_setup_device(struct net_device *dev)
 	priv->phydev = of_phy_connect(dev, phy_node, cvm_oct_adjust_link, 0,
 				      PHY_INTERFACE_MODE_GMII);
 
-	if (priv->phydev == NULL)
+	if (!priv->phydev)
 		return -ENODEV;
 
 	priv->last_link = 0;
diff --git a/drivers/staging/octeon/ethernet-mem.c b/drivers/staging/octeon/ethernet-mem.c
index 5a5cdb3..d6172e4 100644
--- a/drivers/staging/octeon/ethernet-mem.c
+++ b/drivers/staging/octeon/ethernet-mem.c
@@ -34,7 +34,7 @@ static int cvm_oct_fill_hw_skbuff(int pool, int size, int elements)
 	while (freed) {
 		struct sk_buff *skb = dev_alloc_skb(size + 256);
 
-		if (unlikely(skb == NULL))
+		if (unlikely(!skb))
 			break;
 		skb_reserve(skb, 256 - (((unsigned long)skb->data) & 0x7f));
 		*(struct sk_buff **)(skb->data - sizeof(void *)) = skb;
@@ -98,7 +98,7 @@ static int cvm_oct_fill_hw_memory(int pool, int size, int elements)
 		 * just before the block.
 		 */
 		memory = kmalloc(size + 256, GFP_ATOMIC);
-		if (unlikely(memory == NULL)) {
+		if (unlikely(!memory)) {
 			pr_warn("Unable to allocate %u bytes for FPA pool %d\n",
 				elements * size, pool);
 			break;
diff --git a/drivers/staging/octeon/ethernet-rgmii.c b/drivers/staging/octeon/ethernet-rgmii.c
index 51dcb61..3d7513c 100644
--- a/drivers/staging/octeon/ethernet-rgmii.c
+++ b/drivers/staging/octeon/ethernet-rgmii.c
@@ -68,7 +68,7 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
 	struct octeon_ethernet *priv = netdev_priv(dev);
 	unsigned long flags = 0;
 	cvmx_helper_link_info_t link_info;
-	int use_global_register_lock = (priv->phydev == NULL);
+	int use_global_register_lock = (!priv->phydev);
 
 	BUG_ON(in_interrupt());
 	if (use_global_register_lock) {
@@ -124,7 +124,7 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
 	 */
 	cvm_oct_set_hw_preamble(priv, true);
 
-	if (priv->phydev == NULL) {
+	if (!priv->phydev) {
 		link_info = cvmx_helper_link_autoconf(priv->port);
 		priv->link_info = link_info.u64;
 	}
@@ -134,7 +134,7 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
 	else
 		mutex_unlock(&priv->phydev->bus->mdio_lock);
 
-	if (priv->phydev == NULL) {
+	if (!priv->phydev) {
 		/* Tell core. */
 		if (link_info.s.link_up) {
 			if (!netif_carrier_ok(dev))
@@ -285,7 +285,6 @@ void cvm_oct_rgmii_uninit(struct net_device *dev)
 	    || (priv->imode == CVMX_HELPER_INTERFACE_MODE_RGMII)) {
 
 		if (!octeon_is_simulation()) {
-
 			union cvmx_gmxx_rxx_int_en gmx_rx_int_en;
 			int interface = INTERFACE(priv->port);
 			int index = INDEX(priv->port);
diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
index 6aed3cf..170cb12 100644
--- a/drivers/staging/octeon/ethernet-rx.c
+++ b/drivers/staging/octeon/ethernet-rx.c
@@ -103,7 +103,6 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
 		gmxx_rxx_frm_ctl.u64 =
 		    cvmx_read_csr(CVMX_GMXX_RXX_FRM_CTL(index, interface));
 		if (gmxx_rxx_frm_ctl.s.pre_chk == 0) {
-
 			u8 *ptr =
 			    cvmx_phys_to_ptr(work->packet_ptr.s.addr);
 			int i = 0;
@@ -117,15 +116,15 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
 
 			if (*ptr == 0xd5) {
 				/*
-				  printk_ratelimited("Port %d received 0xd5 preamble\n",
-					  port);
+				 * printk_ratelimited("Port %d received 0xd5 preamble\n"
+				 *		      port);
 				 */
 				work->packet_ptr.s.addr += i + 1;
 				work->word1.len -= i + 5;
 			} else if ((*ptr & 0xf) == 0xd) {
 				/*
-				  printk_ratelimited("Port %d received 0x?d preamble\n",
-					  port);
+				 * printk_ratelimited("Port %d received 0x?d preamble\n",
+				 *		      port);
 				 */
 				work->packet_ptr.s.addr += i;
 				work->word1.len -= i + 4;
@@ -139,7 +138,7 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
 				printk_ratelimited("Port %d unknown preamble, packet dropped\n",
 						   port);
 				/*
-				   cvmx_helper_dump_packet(work);
+				 * cvmx_helper_dump_packet(work);
 				 */
 				cvm_oct_free_work(work);
 				return 1;
@@ -184,7 +183,7 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 	if (OCTEON_IS_MODEL(OCTEON_CN68XX)) {
 		old_group_mask = cvmx_read_csr(CVMX_SSO_PPX_GRP_MSK(coreid));
 		cvmx_write_csr(CVMX_SSO_PPX_GRP_MSK(coreid),
-				1ull << pow_receive_group);
+			       1ull << pow_receive_group);
 		cvmx_read_csr(CVMX_SSO_PPX_GRP_MSK(coreid)); /* Flush */
 	} else {
 		old_group_mask = cvmx_read_csr(CVMX_POW_PP_GRP_MSKX(coreid));
@@ -211,7 +210,7 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 
 		prefetch(work);
 		did_work_request = 0;
-		if (work == NULL) {
+		if (!work) {
 			if (OCTEON_IS_MODEL(OCTEON_CN68XX)) {
 				cvmx_write_csr(CVMX_SSO_WQ_IQ_DIS,
 					       1ull << pow_receive_group);
@@ -380,9 +379,9 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 			} else {
 				/* Drop any packet received for a device that isn't up */
 				/*
-				  printk_ratelimited("%s: Device not up, packet dropped\n",
-					   dev->name);
-				*/
+				 * printk_ratelimited("%s: Device not up, packet dropped\n",
+				 *                    dev->name);
+				 */
 #ifdef CONFIG_64BIT
 				atomic64_add(1,
 					     (atomic64_t *)&priv->stats.rx_dropped);
@@ -398,7 +397,7 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 			 * doesn't exist.
 			 */
 			printk_ratelimited("Port %d not controlled by Linux, packet dropped\n",
-				   port);
+					   port);
 			dev_kfree_skb_irq(skb);
 		}
 		/*
@@ -433,7 +432,7 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 	}
 	cvm_oct_rx_refill_pool(0);
 
-	if (rx_count < budget && napi != NULL) {
+	if (rx_count < budget && napi) {
 		/* No more work */
 		napi_complete(napi);
 		enable_irq(OCTEON_IRQ_WORKQ0 + pow_receive_group);
@@ -466,7 +465,7 @@ void cvm_oct_rx_initialize(void)
 		}
 	}
 
-	if (NULL == dev_for_napi)
+	if (!dev_for_napi)
 		panic("No net_devices were allocated.");
 
 	netif_napi_add(dev_for_napi, &cvm_oct_napi, cvm_oct_napi_poll,
diff --git a/drivers/staging/octeon/ethernet-rx.h b/drivers/staging/octeon/ethernet-rx.h
index a5973fd..315a63d 100644
--- a/drivers/staging/octeon/ethernet-rx.h
+++ b/drivers/staging/octeon/ethernet-rx.h
@@ -30,7 +30,7 @@ static inline void cvm_oct_rx_refill_pool(int fill_threshold)
 						 number_to_free);
 		if (num_freed != number_to_free) {
 			cvmx_fau_atomic_add32(FAU_NUM_PACKET_BUFFERS_TO_FREE,
-					number_to_free - num_freed);
+					      number_to_free - num_freed);
 		}
 	}
 }
diff --git a/drivers/staging/octeon/ethernet-spi.c b/drivers/staging/octeon/ethernet-spi.c
index 2ae1944..063dcd0 100644
--- a/drivers/staging/octeon/ethernet-spi.c
+++ b/drivers/staging/octeon/ethernet-spi.c
@@ -167,9 +167,7 @@ static void cvm_oct_spi_poll(struct net_device *dev)
 	int interface;
 
 	for (interface = 0; interface < 2; interface++) {
-
 		if ((priv->port == interface * 16) && need_retrain[interface]) {
-
 			if (cvmx_spi_restart_interface
 			    (interface, CVMX_SPI_MODE_DUPLEX, 10) == 0) {
 				need_retrain[interface] = 0;
diff --git a/drivers/staging/octeon/ethernet-tx.c b/drivers/staging/octeon/ethernet-tx.c
index 3a5d8f5..afbe537 100644
--- a/drivers/staging/octeon/ethernet-tx.c
+++ b/drivers/staging/octeon/ethernet-tx.c
@@ -95,11 +95,10 @@ static void cvm_oct_free_tx_skbs(struct net_device *dev)
 	for (qos = 0; qos < queues_per_port; qos++) {
 		if (skb_queue_len(&priv->tx_free_list[qos]) == 0)
 			continue;
-		skb_to_free = cvmx_fau_fetch_and_add32(priv->fau+qos*4,
+		skb_to_free = cvmx_fau_fetch_and_add32(priv->fau + qos * 4,
 						       MAX_SKB_TO_FREE);
 		skb_to_free = cvm_oct_adjust_skb_to_free(skb_to_free,
-							 priv->fau+qos*4);
-
+							 priv->fau + qos * 4);
 
 		total_freed += skb_to_free;
 		if (skb_to_free > 0) {
@@ -318,37 +317,37 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (unlikely
 	    ((skb_end_pointer(skb) - fpa_head) < CVMX_FPA_PACKET_POOL_SIZE)) {
 		/*
-		   printk("TX buffer isn't large enough for the FPA\n");
+		 * printk("TX buffer isn't large enough for the FPA\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
 	if (unlikely(skb_shared(skb))) {
 		/*
-		   printk("TX buffer sharing data with someone else\n");
+		 * printk("TX buffer sharing data with someone else\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
 	if (unlikely(skb_cloned(skb))) {
 		/*
-		   printk("TX buffer has been cloned\n");
+		 * printk("TX buffer has been cloned\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
 	if (unlikely(skb_header_cloned(skb))) {
 		/*
-		   printk("TX buffer header has been cloned\n");
+		 * printk("TX buffer header has been cloned\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
 	if (unlikely(skb->destructor)) {
 		/*
-		   printk("TX buffer has a destructor\n");
+		 * printk("TX buffer has a destructor\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
 	if (unlikely(skb_shinfo(skb)->nr_frags)) {
 		/*
-		   printk("TX buffer has fragments\n");
+		 * printk("TX buffer has fragments\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
@@ -356,7 +355,7 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
 	    (skb->truesize !=
 	     sizeof(*skb) + skb_end_offset(skb))) {
 		/*
-		   printk("TX buffer truesize has been changed\n");
+		 * printk("TX buffer truesize has been changed\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
@@ -430,7 +429,7 @@ dont_put_skbuff_in_hw:
 
 	if (pko_command.s.dontfree) {
 		queue_type = QUEUE_CORE;
-		pko_command.s.reg0 = priv->fau+qos*4;
+		pko_command.s.reg0 = priv->fau + qos * 4;
 	} else {
 		queue_type = QUEUE_HW;
 	}
@@ -443,7 +442,6 @@ dont_put_skbuff_in_hw:
 	/* Drop this packet if we have too many already queued to the HW */
 	if (unlikely(skb_queue_len(&priv->tx_free_list[qos]) >=
 		     MAX_OUT_QUEUE_DEPTH)) {
-
 		if (dev->tx_queue_len != 0) {
 			/* Drop the lock when notifying the core.  */
 			spin_unlock_irqrestore(&priv->tx_free_list[qos].lock,
@@ -549,7 +547,7 @@ int cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev)
 	/* Get a work queue entry */
 	cvmx_wqe_t *work = cvmx_fpa_alloc(CVMX_FPA_WQE_POOL);
 
-	if (unlikely(work == NULL)) {
+	if (unlikely(!work)) {
 		printk_ratelimited("%s: Failed to allocate a work queue entry\n",
 				   dev->name);
 		priv->stats.tx_dropped++;
@@ -559,7 +557,7 @@ int cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev)
 
 	/* Get a packet buffer */
 	packet_buffer = cvmx_fpa_alloc(CVMX_FPA_PACKET_POOL);
-	if (unlikely(packet_buffer == NULL)) {
+	if (unlikely(!packet_buffer)) {
 		printk_ratelimited("%s: Failed to allocate a packet buffer\n",
 				   dev->name);
 		cvmx_fpa_free(work, CVMX_FPA_WQE_POOL, 1);
diff --git a/drivers/staging/octeon/ethernet-xaui.c b/drivers/staging/octeon/ethernet-xaui.c
index 4b47bcf..978407e 100644
--- a/drivers/staging/octeon/ethernet-xaui.c
+++ b/drivers/staging/octeon/ethernet-xaui.c
@@ -35,7 +35,7 @@ int cvm_oct_xaui_init(struct net_device *dev)
 	struct octeon_ethernet *priv = netdev_priv(dev);
 
 	cvm_oct_common_init(dev);
-	if (!octeon_is_simulation() && priv->phydev == NULL)
+	if (!octeon_is_simulation() && !priv->phydev)
 		priv->poll = cvm_oct_link_poll;
 
 	return 0;
diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c
index c9183e0..6e81e79 100644
--- a/drivers/staging/octeon/ethernet.c
+++ b/drivers/staging/octeon/ethernet.c
@@ -674,7 +674,7 @@ static int cvm_oct_probe(struct platform_device *pdev)
 	}
 
 	cvm_oct_poll_queue = create_singlethread_workqueue("octeon-ethernet");
-	if (cvm_oct_poll_queue == NULL) {
+	if (!cvm_oct_poll_queue) {
 		pr_err("octeon-ethernet: Cannot create workqueue");
 		return -ENOMEM;
 	}
-- 
2.1.0

Re: [PATCH] staging: octeon: fixed few coding style warnings

[David Daney]

On 10/14/2015 07:06 AM, Sakshi Bansal wrote:
> Fixed allignment issues and line over 80 characters

Use spell checking on 'allignment'

But that is not the main problem with the patch...


You are changing things other than white space and comment formatting, 
can you tell us on which platforms the patch was tested to verify that 
you didn't break anything?

>
> Signed-off-by: Sakshi Bansal <sakshi.april5@gmail.com>


NAK.

[...]
> diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c
> index fd9b3d8..590a6cb 100644
> --- a/drivers/staging/octeon/ethernet-mdio.c
> +++ b/drivers/staging/octeon/ethernet-mdio.c
> @@ -180,7 +180,7 @@ int cvm_oct_phy_setup_device(struct net_device *dev)
>   	priv->phydev = of_phy_connect(dev, phy_node, cvm_oct_adjust_link, 0,
>   				      PHY_INTERFACE_MODE_GMII);
>
> -	if (priv->phydev == NULL)
> +	if (!priv->phydev)


Not a coding style change.  There is no WARNING generated for this case.



>   		return -ENODEV;
>
>   	priv->last_link = 0;
> diff --git a/drivers/staging/octeon/ethernet-mem.c b/drivers/staging/octeon/ethernet-mem.c
> index 5a5cdb3..d6172e4 100644
> --- a/drivers/staging/octeon/ethernet-mem.c
> +++ b/drivers/staging/octeon/ethernet-mem.c
> @@ -34,7 +34,7 @@ static int cvm_oct_fill_hw_skbuff(int pool, int size, int elements)
>   	while (freed) {
>   		struct sk_buff *skb = dev_alloc_skb(size + 256);
>
> -		if (unlikely(skb == NULL))
> +		if (unlikely(!skb))

Same

>   			break;
>   		skb_reserve(skb, 256 - (((unsigned long)skb->data) & 0x7f));
>   		*(struct sk_buff **)(skb->data - sizeof(void *)) = skb;
> @@ -98,7 +98,7 @@ static int cvm_oct_fill_hw_memory(int pool, int size, int elements)
>   		 * just before the block.
>   		 */
>   		memory = kmalloc(size + 256, GFP_ATOMIC);
> -		if (unlikely(memory == NULL)) {
> +		if (unlikely(!memory)) {

Same

>   			pr_warn("Unable to allocate %u bytes for FPA pool %d\n",
>   				elements * size, pool);
>   			break;
> diff --git a/drivers/staging/octeon/ethernet-rgmii.c b/drivers/staging/octeon/ethernet-rgmii.c
> index 51dcb61..3d7513c 100644
> --- a/drivers/staging/octeon/ethernet-rgmii.c
> +++ b/drivers/staging/octeon/ethernet-rgmii.c
> @@ -68,7 +68,7 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
>   	struct octeon_ethernet *priv = netdev_priv(dev);
>   	unsigned long flags = 0;
>   	cvmx_helper_link_info_t link_info;
> -	int use_global_register_lock = (priv->phydev == NULL);
> +	int use_global_register_lock = (!priv->phydev);

Same.

I could go on, but I think you see the pattern here.

Your changelog says you are fixing warnings, but none of these are 
warning fixes.

In fact it is perfectly acceptable to compare a pointer to NULL.  It is 
a common idiom in the kernel.  The original author of the code thought 
it was more clear this way, and you are causing code churn for no reason.

Try to run this command on the kernel sources:
$ git grep -e '== NULL' | wc -l
21488

I would suggest that you convince people that the other 21,000 cases of 
comparison to NULL need changing before you do it to this driver.

David Daney

Re: [PATCH] staging: octeon: fixed few coding style warnings

[David Daney]

On 10/14/2015 08:51 AM, Sakshi Bansal wrote:
> I am testing it on Fedora 21.

Does your Fedora 21 platform use this driver in any way?  Does it even 
build the driver (produce any .o files for any of the files you changed)?

No, I didn't think so.

You have to have a way to test the patch (or at least compile the 
files), or have other people test it for you if you make changes.


> The change under concern was mentioned as
> a "Check ".

Yes, notice that 'CHECK:' is emitted in the color green.  It is not a 
warning.  Your patch subject line said you were fixing warnings.  This 
is not true.


>
> On Oct 14, 2015 9:17 PM, "David Daney" <ddaney@caviumnetworks.com
> <mailto:ddaney@caviumnetworks.com>> wrote:
>
>     On 10/14/2015 07:06 AM, Sakshi Bansal wrote:
>
>         Fixed allignment issues and line over 80 characters
>
>
>     Use spell checking on 'allignment'
>
>     But that is not the main problem with the patch...
>
>
>     You are changing things other than white space and comment
>     formatting, can you tell us on which platforms the patch was tested
>     to verify that you didn't break anything?
>
>
>         Signed-off-by: Sakshi Bansal <sakshi.april5@gmail.com
>         <mailto:sakshi.april5@gmail.com>>
>
>
>
>     NAK.
>
>     [...]
>
>         diff --git a/drivers/staging/octeon/ethernet-mdio.c
>         b/drivers/staging/octeon/ethernet-mdio.c
>         index fd9b3d8..590a6cb 100644
>         --- a/drivers/staging/octeon/ethernet-mdio.c
>         +++ b/drivers/staging/octeon/ethernet-mdio.c
>         @@ -180,7 +180,7 @@ int cvm_oct_phy_setup_device(struct
>         net_device *dev)
>                  priv->phydev = of_phy_connect(dev, phy_node,
>         cvm_oct_adjust_link, 0,
>                                                PHY_INTERFACE_MODE_GMII);
>
>         -       if (priv->phydev == NULL)
>         +       if (!priv->phydev)
>
>
>
>     Not a coding style change.  There is no WARNING generated for this case.
>
>
>
>                          return -ENODEV;
>
>                  priv->last_link = 0;
>         diff --git a/drivers/staging/octeon/ethernet-mem.c
>         b/drivers/staging/octeon/ethernet-mem.c
>         index 5a5cdb3..d6172e4 100644
>         --- a/drivers/staging/octeon/ethernet-mem.c
>         +++ b/drivers/staging/octeon/ethernet-mem.c
>         @@ -34,7 +34,7 @@ static int cvm_oct_fill_hw_skbuff(int pool,
>         int size, int elements)
>                  while (freed) {
>                          struct sk_buff *skb = dev_alloc_skb(size + 256);
>
>         -               if (unlikely(skb == NULL))
>         +               if (unlikely(!skb))
>
>
>     Same
>
>                                  break;
>                          skb_reserve(skb, 256 - (((unsigned
>         long)skb->data) & 0x7f));
>                          *(struct sk_buff **)(skb->data - sizeof(void
>         *)) = skb;
>         @@ -98,7 +98,7 @@ static int cvm_oct_fill_hw_memory(int pool,
>         int size, int elements)
>                           * just before the block.
>                           */
>                          memory = kmalloc(size + 256, GFP_ATOMIC);
>         -               if (unlikely(memory == NULL)) {
>         +               if (unlikely(!memory)) {
>
>
>     Same
>
>                                  pr_warn("Unable to allocate %u bytes
>         for FPA pool %d\n",
>                                          elements * size, pool);
>                                  break;
>         diff --git a/drivers/staging/octeon/ethernet-rgmii.c
>         b/drivers/staging/octeon/ethernet-rgmii.c
>         index 51dcb61..3d7513c 100644
>         --- a/drivers/staging/octeon/ethernet-rgmii.c
>         +++ b/drivers/staging/octeon/ethernet-rgmii.c
>         @@ -68,7 +68,7 @@ static void cvm_oct_rgmii_poll(struct
>         net_device *dev)
>                  struct octeon_ethernet *priv = netdev_priv(dev);
>                  unsigned long flags = 0;
>                  cvmx_helper_link_info_t link_info;
>         -       int use_global_register_lock = (priv->phydev == NULL);
>         +       int use_global_register_lock = (!priv->phydev);
>
>
>     Same.
>
>     I could go on, but I think you see the pattern here.
>
>     Your changelog says you are fixing warnings, but none of these are
>     warning fixes.
>
>     In fact it is perfectly acceptable to compare a pointer to NULL.  It
>     is a common idiom in the kernel.  The original author of the code
>     thought it was more clear this way, and you are causing code churn
>     for no reason.
>
>     Try to run this command on the kernel sources:
>     $ git grep -e '== NULL' | wc -l
>     21488
>
>     I would suggest that you convince people that the other 21,000 cases
>     of comparison to NULL need changing before you do it to this driver.
>
>     David Daney
>

Re: [PATCH] staging: octeon: fixed few coding style warnings

[David Daney]

On 10/14/2015 09:04 AM, Sakshi Bansal wrote:
> Well I did not test the patch

Never send patches to code that you don't at lease attempt to compile.

> since the changes were mostly coding style
> related or would not affect the code.
>
> As for the null check, if the original check is valid I can remove them
> from the patch.

They are valid, and I have seen no good reason presented to change them.

If you want to churn up the code with white space changes, I cannot 
prevent it, but at least accurately describe what you are doing, and 
make sure you don't break the driver.


David Daney

>
> On Oct 14, 2015 9:27 PM, "David Daney" <ddaney@caviumnetworks.com
> <mailto:ddaney@caviumnetworks.com>> wrote:
>
>     On 10/14/2015 08:51 AM, Sakshi Bansal wrote:
>
>         I am testing it on Fedora 21.
>
>
>     Does your Fedora 21 platform use this driver in any way?  Does it
>     even build the driver (produce any .o files for any of the files you
>     changed)?
>
>     No, I didn't think so.
>
>     You have to have a way to test the patch (or at least compile the
>     files), or have other people test it for you if you make changes.
>
>
>         The change under concern was mentioned as
>         a "Check ".
>
>
>     Yes, notice that 'CHECK:' is emitted in the color green.  It is not
>     a warning.  Your patch subject line said you were fixing warnings.
>     This is not true.
>
>
>
>         On Oct 14, 2015 9:17 PM, "David Daney"
>         <ddaney@caviumnetworks.com <mailto:ddaney@caviumnetworks.com>
>         <mailto:ddaney@caviumnetworks.com
>         <mailto:ddaney@caviumnetworks.com>>> wrote:
>
>              On 10/14/2015 07:06 AM, Sakshi Bansal wrote:
>
>                  Fixed allignment issues and line over 80 characters
>
>
>              Use spell checking on 'allignment'
>
>              But that is not the main problem with the patch...
>
>
>              You are changing things other than white space and comment
>              formatting, can you tell us on which platforms the patch
>         was tested
>              to verify that you didn't break anything?
>
>
>                  Signed-off-by: Sakshi Bansal <sakshi.april5@gmail.com
>         <mailto:sakshi.april5@gmail.com>
>                  <mailto:sakshi.april5@gmail.com
>         <mailto:sakshi.april5@gmail.com>>>
>
>
>
>              NAK.
>
>              [...]
>
>                  diff --git a/drivers/staging/octeon/ethernet-mdio.c
>                  b/drivers/staging/octeon/ethernet-mdio.c
>                  index fd9b3d8..590a6cb 100644
>                  --- a/drivers/staging/octeon/ethernet-mdio.c
>                  +++ b/drivers/staging/octeon/ethernet-mdio.c
>                  @@ -180,7 +180,7 @@ int cvm_oct_phy_setup_device(struct
>                  net_device *dev)
>                           priv->phydev = of_phy_connect(dev, phy_node,
>                  cvm_oct_adjust_link, 0,
>
>           PHY_INTERFACE_MODE_GMII);
>
>                  -       if (priv->phydev == NULL)
>                  +       if (!priv->phydev)
>
>
>
>              Not a coding style change.  There is no WARNING generated
>         for this case.
>
>
>
>                                   return -ENODEV;
>
>                           priv->last_link = 0;
>                  diff --git a/drivers/staging/octeon/ethernet-mem.c
>                  b/drivers/staging/octeon/ethernet-mem.c
>                  index 5a5cdb3..d6172e4 100644
>                  --- a/drivers/staging/octeon/ethernet-mem.c
>                  +++ b/drivers/staging/octeon/ethernet-mem.c
>                  @@ -34,7 +34,7 @@ static int cvm_oct_fill_hw_skbuff(int
>         pool,
>                  int size, int elements)
>                           while (freed) {
>                                   struct sk_buff *skb =
>         dev_alloc_skb(size + 256);
>
>                  -               if (unlikely(skb == NULL))
>                  +               if (unlikely(!skb))
>
>
>              Same
>
>                                           break;
>                                   skb_reserve(skb, 256 - (((unsigned
>                  long)skb->data) & 0x7f));
>                                   *(struct sk_buff **)(skb->data -
>         sizeof(void
>                  *)) = skb;
>                  @@ -98,7 +98,7 @@ static int cvm_oct_fill_hw_memory(int
>         pool,
>                  int size, int elements)
>                                    * just before the block.
>                                    */
>                                   memory = kmalloc(size + 256, GFP_ATOMIC);
>                  -               if (unlikely(memory == NULL)) {
>                  +               if (unlikely(!memory)) {
>
>
>              Same
>
>                                           pr_warn("Unable to allocate %u
>         bytes
>                  for FPA pool %d\n",
>                                                   elements * size, pool);
>                                           break;
>                  diff --git a/drivers/staging/octeon/ethernet-rgmii.c
>                  b/drivers/staging/octeon/ethernet-rgmii.c
>                  index 51dcb61..3d7513c 100644
>                  --- a/drivers/staging/octeon/ethernet-rgmii.c
>                  +++ b/drivers/staging/octeon/ethernet-rgmii.c
>                  @@ -68,7 +68,7 @@ static void cvm_oct_rgmii_poll(struct
>                  net_device *dev)
>                           struct octeon_ethernet *priv = netdev_priv(dev);
>                           unsigned long flags = 0;
>                           cvmx_helper_link_info_t link_info;
>                  -       int use_global_register_lock = (priv->phydev ==
>         NULL);
>                  +       int use_global_register_lock = (!priv->phydev);
>
>
>              Same.
>
>              I could go on, but I think you see the pattern here.
>
>              Your changelog says you are fixing warnings, but none of
>         these are
>              warning fixes.
>
>              In fact it is perfectly acceptable to compare a pointer to
>         NULL.  It
>              is a common idiom in the kernel.  The original author of
>         the code
>              thought it was more clear this way, and you are causing
>         code churn
>              for no reason.
>
>              Try to run this command on the kernel sources:
>              $ git grep -e '== NULL' | wc -l
>              21488
>
>              I would suggest that you convince people that the other
>         21,000 cases
>              of comparison to NULL need changing before you do it to
>         this driver.
>
>              David Daney
>
>

Re: [PATCH] staging: octeon: fixed few coding style warnings

[Dan Carpenter]

On Wed, Oct 14, 2015 at 09:09:04AM -0700, David Daney wrote:
> and make sure you don't break the driver.

I'm not seeing how this patch breaks anything?  What am I missing?

regards,
dan carpenter

Re: [PATCH] staging: octeon: fixed few coding style warnings

[David Daney]

On 10/15/2015 06:40 AM, Dan Carpenter wrote:
> On Wed, Oct 14, 2015 at 09:09:04AM -0700, David Daney wrote:
>> and make sure you don't break the driver.
>
> I'm not seeing how this patch breaks anything?  What am I missing?

Did you apply it and then compile the result?  If not, then how can you 
assert that it does not break anything?

 From SubmitChecklist item #2: "Builds cleanly ..."

I don't want to waste time considering patches that don't, at a minimum, 
meet this criterion.  Other people may want to do the build testing, but 
I prefer that the patch submitter at least vet the patch enough that 
they can assert that it still builds cleanly.

David Daney

Re: [PATCH] staging: octeon: fixed few coding style warnings

[Dan Carpenter]

Oh.  I use a script to check these...  It doesn't break anything except
for that commented out code which should be deleted anyway.

regards,
dan carpenter

Re: [PATCH] staging: octeon: fixed few coding style warnings

[Arnd Bergmann]

On Wednesday 14 October 2015 08:47:44 David Daney wrote:
> On 10/14/2015 07:06 AM, Sakshi Bansal wrote:
> > Fixed allignment issues and line over 80 characters
> 
> Use spell checking on 'allignment'
> 
> But that is not the main problem with the patch...
> 
> 
> You are changing things other than white space and comment formatting, 
> can you tell us on which platforms the patch was tested to verify that 
> you didn't break anything?

In general a good advice, but for trivial whitespace or comment
changes, this is normally not necessary. Compile-testing a patch
as you say is normally expected, if only to avoid embarrassing
complaints if it does break later.

For drivers that are not enabled in the x86 allmodconfig, it sure
helps to say something like "Compile-tested using MIPS cross toolchain
from https://www.kernel.org/pub/tools/crosstool/". Even better would
be to send a fix to decouple the driver from asm/octeon/*.h
to make it build on all architectures, but that is much more work
than I'd expect for a trivial patch.

> > Signed-off-by: Sakshi Bansal <sakshi.april5@gmail.com>
> 
> 
> NAK.

Actually all the changes in the patch look reasonable for a staging driver,
but the submission form needs to be improved.

I'm guessing this was a first-time patch submission (presumably for
an outreachy application), which means it's pretty much expected to
get a few things wrong and correct them in a follow-up.

> > @@ -68,7 +68,7 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
> >       struct octeon_ethernet *priv = netdev_priv(dev);
> >       unsigned long flags = 0;
> >       cvmx_helper_link_info_t link_info;
> > -     int use_global_register_lock = (priv->phydev == NULL);
> > +     int use_global_register_lock = (!priv->phydev);
> 
> Same.
> 
> I could go on, but I think you see the pattern here.
> 
> Your changelog says you are fixing warnings, but none of these are 
> warning fixes.
> 
> In fact it is perfectly acceptable to compare a pointer to NULL.  It is 
> a common idiom in the kernel.  The original author of the code thought 
> it was more clear this way, and you are causing code churn for no reason.

The style she changed it to is the preferred style however. We don't
normally send or take patches to change this in regular code, but
for drivers/staging/ this is the kind of patch you should expect to
see.

To improve the patch, I would suggest to split it up into smaller chunks
in more logical units. "fixed few coding style warnings" is not a good
one-line description for a patch, as it already hints that the patch
is doing multiple unrelated changes, and is not unique in the sense
that another patch could have the exact same description and do something
completely different.

Instead, this can be done as a series of three patches:

[PATCH 1/3] staging: octeon: fix commenting style
[PATCH 2/3] staging: octeon: avoid redundant "== NULL" comparison
[PATCH 3/3] staging: octeon: whitespace fixes

All three of these are still trivial of course, but this is already
made clear by the subject line. In the detailed description, there
should be an explanation for each patch why the new version is
considered an improvement over the existing code. If you cannot come
up with an explanation other than 'checkpatch said this', it's better
to drop the patch, but with a good description most arguments about
the merits of a change can be avoided.

	Arnd

Re: [PATCH] staging: octeon: fixed few coding style warnings

[David Daney]

On 10/14/2015 01:58 PM, Arnd Bergmann wrote:
> On Wednesday 14 October 2015 08:47:44 David Daney wrote:
>> On 10/14/2015 07:06 AM, Sakshi Bansal wrote:
>>> Fixed allignment issues and line over 80 characters
>>
>> Use spell checking on 'allignment'
>>
>> But that is not the main problem with the patch...
>>
>>
>> You are changing things other than white space and comment formatting,
>> can you tell us on which platforms the patch was tested to verify that
>> you didn't break anything?
>
> In general a good advice, but for trivial whitespace or comment
> changes, this is normally not necessary. Compile-testing a patch
> as you say is normally expected, if only to avoid embarrassing
> complaints if it does break later.
>
> For drivers that are not enabled in the x86 allmodconfig, it sure
> helps to say something like "Compile-tested using MIPS cross toolchain
> from https://www.kernel.org/pub/tools/crosstool/". Even better would
> be to send a fix to decouple the driver from asm/octeon/*.h
> to make it build on all architectures, but that is much more work
> than I'd expect for a trivial patch.

There is no point in such a patch.  We know, a priori, that there will 
*never* be a non-MIPS system with this hardware.

David Daney

Re: [PATCH] staging: octeon: fixed few coding style warnings

[Arnd Bergmann]

On Wednesday 14 October 2015 14:04:00 David Daney wrote:
> On 10/14/2015 01:58 PM, Arnd Bergmann wrote:
> > On Wednesday 14 October 2015 08:47:44 David Daney wrote:
> >> On 10/14/2015 07:06 AM, Sakshi Bansal wrote:
> >>> Fixed allignment issues and line over 80 characters
> >>
> >> Use spell checking on 'allignment'
> >>
> >> But that is not the main problem with the patch...
> >>
> >>
> >> You are changing things other than white space and comment formatting,
> >> can you tell us on which platforms the patch was tested to verify that
> >> you didn't break anything?
> >
> > In general a good advice, but for trivial whitespace or comment
> > changes, this is normally not necessary. Compile-testing a patch
> > as you say is normally expected, if only to avoid embarrassing
> > complaints if it does break later.
> >
> > For drivers that are not enabled in the x86 allmodconfig, it sure
> > helps to say something like "Compile-tested using MIPS cross toolchain
> > from https://www.kernel.org/pub/tools/crosstool/". Even better would
> > be to send a fix to decouple the driver from asm/octeon/*.h
> > to make it build on all architectures, but that is much more work
> > than I'd expect for a trivial patch.
> 
> There is no point in such a patch.  We know, a priori, that there will 
> *never* be a non-MIPS system with this hardware.

We generally like to have all drivers compilable using COMPILE_TEST
across all architectures, as that enables automated build regression
tests from services like coverity or the 0-day build tester.

It also makes it much easier for normal developers to test changes
when they can verify them using a single toolchain and allmodconfig.

	Arnd

Re: [PATCH] staging: octeon: fixed few coding style warnings

[Aaro Koskinen]

Hi,

On Wed, Oct 14, 2015 at 07:36:37PM +0530, Sakshi Bansal wrote:
> Fixed allignment issues and line over 80 characters

You are also doing many other things which are not mention in the
commit log.

>  				/*
> -				  printk_ratelimited("Port %d received 0xd5 preamble\n",
> -					  port);
> +				 * printk_ratelimited("Port %d received 0xd5 preamble\n"
> +				 *		      port);
>  				 */

This is not really an improvement. Either the dead code should be deleted
altogether or converted to debug printk()s. (Probably the idea has been
that those printk()s can be "easily" enabled for debugging by removing
comment start/end marks.)

You also removed a comma there, breaking the syntax...

A.