Messages in this thread | | | Date | Fri, 27 Jul 2018 22:52:56 +0300 | From | Ivan Khoronzhuk <> | Subject | Re: [PATCH net-next] net: ethernet: ti: cpsw: replace unnecessarily macroses on functions |
| |
On Fri, Jul 27, 2018 at 10:36:43PM +0300, Ivan Khoronzhuk wrote: >On Fri, Jul 27, 2018 at 12:21:07PM -0700, Joe Perches wrote: >>On Fri, 2018-07-27 at 22:13 +0300, Ivan Khoronzhuk wrote: >>>Replace ugly macroses on functions. >> >>Careful, see below. >> >>>diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c >>[] >>>@@ -565,40 +565,40 @@ static const struct cpsw_stats cpsw_gstrings_ch_stats[] = { >>> (func)(slave++, ##arg); \ >>> } while (0) >>> >>>-#define cpsw_dual_emac_src_port_detect(cpsw, status, ndev, skb) \ >>>- do { \ >>>- if (!cpsw->data.dual_emac) \ >>>- break; \ >>>- if (CPDMA_RX_SOURCE_PORT(status) == 1) { \ >>>- ndev = cpsw->slaves[0].ndev; \ >>>- skb->dev = ndev; \ >>>- } else if (CPDMA_RX_SOURCE_PORT(status) == 2) { \ >>>- ndev = cpsw->slaves[1].ndev; \ >>>- skb->dev = ndev; \ >>>- } \ >>>- } while (0) >>>-#define cpsw_add_mcast(cpsw, priv, addr) \ >>>- do { \ >>>- if (cpsw->data.dual_emac) { \ >>>- struct cpsw_slave *slave = cpsw->slaves + \ >>>- priv->emac_port; \ >>>- int slave_port = cpsw_get_slave_port( \ >>>- slave->slave_num); \ >>>- cpsw_ale_add_mcast(cpsw->ale, addr, \ >>>- 1 << slave_port | ALE_PORT_HOST, \ >>>- ALE_VLAN, slave->port_vlan, 0); \ >>>- } else { \ >>>- cpsw_ale_add_mcast(cpsw->ale, addr, \ >>>- ALE_ALL_PORTS, \ >>>- 0, 0, 0); \ >>>- } \ >>>- } while (0) >>>- >>> static inline int cpsw_get_slave_port(u32 slave_num) >>> { >>> return slave_num + 1; >>> } >>> >>>+static inline void cpsw_src_port_detect(struct cpsw_common *cpsw, int status, >>>+ struct sk_buff *skb) >>>+{ >>>+ if (!cpsw->data.dual_emac) >>>+ return; >>>+ >>>+ if (CPDMA_RX_SOURCE_PORT(status) == 1) >>>+ skb->dev = cpsw->slaves[0].ndev; >>>+ else if (CPDMA_RX_SOURCE_PORT(status) == 2) >>>+ skb->dev = cpsw->slaves[1].ndev; >>>+} >> >>perhaps better as a switch/case >not better, it's shorter. > >> >>>+ >>>+static void cpsw_add_mcast(struct cpsw_priv *priv, u8 *addr) >>>+{ >>>+ struct cpsw_common *cpsw = priv->cpsw; >>>+ >>>+ if (cpsw->data.dual_emac) { >>>+ struct cpsw_slave *slave = cpsw->slaves + priv->emac_port; >>>+ int slave_port = cpsw_get_slave_port(slave->slave_num); >>>+ >>>+ cpsw_ale_add_mcast(cpsw->ale, addr, >>>+ 1 << slave_port | ALE_PORT_HOST, >>>+ ALE_VLAN, slave->port_vlan, 0); >>>+ return; >>>+ } >>>+ >>>+ cpsw_ale_add_mcast(cpsw->ale, addr, ALE_ALL_PORTS, 0, 0, 0); >>>+} >>>+ >>> static void cpsw_set_promiscious(struct net_device *ndev, bool enable) >>> { >>> struct cpsw_common *cpsw = ndev_to_cpsw(ndev); >>>@@ -706,7 +706,7 @@ static void cpsw_ndo_set_rx_mode(struct net_device *ndev) >>> >>> /* program multicast address list into ALE register */ >>> netdev_for_each_mc_addr(ha, ndev) { >>>- cpsw_add_mcast(cpsw, priv, (u8 *)ha->addr); >>>+ cpsw_add_mcast(priv, (u8 *)ha->addr); >>> } >>> } >>> } >>>@@ -801,7 +801,8 @@ static void cpsw_rx_handler(void *token, int len, int status) >>> int ret = 0; >>> struct cpsw_common *cpsw = ndev_to_cpsw(ndev); >>> >>>- cpsw_dual_emac_src_port_detect(cpsw, status, ndev, skb); >>>+ cpsw_src_port_detect(cpsw, status, skb); >>>+ ndev = skb->dev; >> >>This is not the same code as the new function >>is not guaranteed to succeed or set skb->dev. >Guaranteed by above skb->dev is initialized already. >The function reflects previous macro. > >Even more, seems that here is duplication of ndev=skb->dev; >It should be removed at init ), even if it 100% is optimized by complier. >So guaranteed a little more then needed ), will send v2 with removed >ndev = skb->dev at init if no objection.
But no, not duplicates. It's used to get cpsw. So everyting is fine. Patch should go as is.
> >> >>> if (unlikely(status < 0) || unlikely(!netif_running(ndev))) { >>> /* In dual emac mode check for all interfaces */ > >-- >Regards, >Ivan Khoronzhuk
-- Regards, Ivan Khoronzhuk
| |