Message ID | 1529322610-27215-3-git-send-email-radhey.shyam.pandey@xilinx.com |
---|---|
State | Deferred, archived |
Delegated to: | David Miller |
Headers | show |
Series | Fixes coding style in xilinx_emaclite.c | expand |
On Mon, 2018-06-18 at 17:20 +0530, Radhey Shyam Pandey wrote: > Remove else as it is not required with if doing a return. > Fixes below checkpatch warning. > WARNING: else is not generally useful after a break or return checkpatch is stupid and doesn't understand code flow. Always try to improve code flow instead of merely following brainless instructions from a script. So: > diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c b/drivers/net/ethernet/xilinx/xilinx_emaclite.c [] > @@ -569,13 +569,11 @@ static void xemaclite_tx_handler(struct net_device *dev) > (u8 *) lp->deferred_skb->data, > lp->deferred_skb->len) != 0) > return; > - else { > - dev->stats.tx_bytes += lp->deferred_skb->len; > - dev_kfree_skb_irq(lp->deferred_skb); > - lp->deferred_skb = NULL; > - netif_trans_update(dev); /* prevent tx timeout */ > - netif_wake_queue(dev); > - } > + dev->stats.tx_bytes += lp->deferred_skb->len; > + dev_kfree_skb_irq(lp->deferred_skb); > + lp->deferred_skb = NULL; > + netif_trans_update(dev); /* prevent tx timeout */ > + netif_wake_queue(dev); > } > } If you really want to redo this function, perhaps something like: static void xemaclite_tx_handler(struct net_device *dev) { struct net_local *lp = netdev_priv(dev); dev->stats.tx_packets++; if (!lp->deferred_skb) return; if (xemaclite_send_data(lp, (u8 *)lp->deferred_skb->data, lp->deferred_skb->len)) return; dev->stats.tx_bytes += lp->deferred_skb->len; dev_kfree_skb_irq(lp->deferred_skb); lp->deferred_skb = NULL; netif_trans_update(dev); /* prevent tx timeout */ netif_wake_queue(dev); } > @@ -1052,13 +1050,13 @@ static bool get_bool(struct platform_device *ofdev, const char *s) > { > u32 *p = (u32 *)of_get_property(ofdev->dev.of_node, s, NULL); > > - if (p) { > + if (p) > return (bool)*p; > - } else { > - dev_warn(&ofdev->dev, "Parameter %s not found," > + > + dev_warn(&ofdev->dev, "Parameter %s not found," > "defaulting to false\n", s); > - return false; > - } > + > + return false; > } And this function has backward logic as the failure paths are the ones that should return early or use a goto. Perhaps something like: static bool get_bool(struct platform_device *ofdev, const char *s) { u32 *p = (u32 *)of_get_property(ofdev->dev.of_node, s, NULL); if (!p) { dev_warn(&ofdev->dev, "Parameter '%s' not found, defaulting to false\n", s); return false; } return *p; }
> -----Original Message----- > From: Joe Perches [mailto:joe@perches.com] > Sent: Monday, June 18, 2018 9:33 PM > To: Radhey Shyam Pandey <radheys@xilinx.com>; davem@davemloft.net; > andrew@lunn.ch; Michal Simek <michals@xilinx.com> > Cc: netdev@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH 2/5] net: emaclite: Balance braces in else statement > > On Mon, 2018-06-18 at 17:20 +0530, Radhey Shyam Pandey wrote: > > Remove else as it is not required with if doing a return. > > Fixes below checkpatch warning. > > > WARNING: else is not generally useful after a break or return > > checkpatch is stupid and doesn't understand code flow. > Always try to improve code flow instead of merely > following brainless instructions from a script. > > So: > > > diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c > b/drivers/net/ethernet/xilinx/xilinx_emaclite.c > [] > > @@ -569,13 +569,11 @@ static void xemaclite_tx_handler(struct net_device > *dev) > > (u8 *) lp->deferred_skb->data, > > lp->deferred_skb->len) != 0) > > return; > > - else { > > - dev->stats.tx_bytes += lp->deferred_skb->len; > > - dev_kfree_skb_irq(lp->deferred_skb); > > - lp->deferred_skb = NULL; > > - netif_trans_update(dev); /* prevent tx timeout */ > > - netif_wake_queue(dev); > > - } > > + dev->stats.tx_bytes += lp->deferred_skb->len; > > + dev_kfree_skb_irq(lp->deferred_skb); > > + lp->deferred_skb = NULL; > > + netif_trans_update(dev); /* prevent tx timeout */ > > + netif_wake_queue(dev); > > } > > } > > If you really want to redo this function, perhaps something like: Thanks for the review. Yes, In v2 I will refactor the code to have failure path return early. > > static void xemaclite_tx_handler(struct net_device *dev) > { > struct net_local *lp = netdev_priv(dev); > > dev->stats.tx_packets++; > > if (!lp->deferred_skb) > return; > > if (xemaclite_send_data(lp, (u8 *)lp->deferred_skb->data, > lp->deferred_skb->len)) > return; > > dev->stats.tx_bytes += lp->deferred_skb->len; > dev_kfree_skb_irq(lp->deferred_skb); > lp->deferred_skb = NULL; > netif_trans_update(dev); /* prevent tx timeout */ > netif_wake_queue(dev); > } > > > @@ -1052,13 +1050,13 @@ static bool get_bool(struct platform_device > *ofdev, const char *s) > > { > > u32 *p = (u32 *)of_get_property(ofdev->dev.of_node, s, NULL); > > > > - if (p) { > > + if (p) > > return (bool)*p; > > - } else { > > - dev_warn(&ofdev->dev, "Parameter %s not found," > > + > > + dev_warn(&ofdev->dev, "Parameter %s not found," > > "defaulting to false\n", s); > > - return false; > > - } > > + > > + return false; > > } > > And this function has backward logic as the failure paths > are the ones that should return early or use a goto. > > Perhaps something like: Yes, will change it. > > static bool get_bool(struct platform_device *ofdev, const char *s) > { > u32 *p = (u32 *)of_get_property(ofdev->dev.of_node, s, NULL); > > if (!p) { > dev_warn(&ofdev->dev, > "Parameter '%s' not found, defaulting to false\n", s); > return false; > } > > return *p; > }
diff --git a/drivers/net/ethernet/xilinx/xilinx_emaclite.c b/drivers/net/ethernet/xilinx/xilinx_emaclite.c index 0544134..8d84f58 100644 --- a/drivers/net/ethernet/xilinx/xilinx_emaclite.c +++ b/drivers/net/ethernet/xilinx/xilinx_emaclite.c @@ -569,13 +569,11 @@ static void xemaclite_tx_handler(struct net_device *dev) (u8 *) lp->deferred_skb->data, lp->deferred_skb->len) != 0) return; - else { - dev->stats.tx_bytes += lp->deferred_skb->len; - dev_kfree_skb_irq(lp->deferred_skb); - lp->deferred_skb = NULL; - netif_trans_update(dev); /* prevent tx timeout */ - netif_wake_queue(dev); - } + dev->stats.tx_bytes += lp->deferred_skb->len; + dev_kfree_skb_irq(lp->deferred_skb); + lp->deferred_skb = NULL; + netif_trans_update(dev); /* prevent tx timeout */ + netif_wake_queue(dev); } } @@ -1052,13 +1050,13 @@ static bool get_bool(struct platform_device *ofdev, const char *s) { u32 *p = (u32 *)of_get_property(ofdev->dev.of_node, s, NULL); - if (p) { + if (p) return (bool)*p; - } else { - dev_warn(&ofdev->dev, "Parameter %s not found," + + dev_warn(&ofdev->dev, "Parameter %s not found," "defaulting to false\n", s); - return false; - } + + return false; } static const struct net_device_ops xemaclite_netdev_ops;