| Message ID | 20180727191318.18698-1-ivan.khoronzhuk@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net-next] net: ethernet: ti: cpsw: replace unnecessarily macroses on functions | expand |
On 07/27/2018 02:13 PM, Ivan Khoronzhuk wrote: > Replace ugly macroses on functions. > > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>
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 > + > +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. > if (unlikely(status < 0) || unlikely(!netif_running(ndev))) { > /* In dual emac mode check for all interfaces */ -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > >> if (unlikely(status < 0) || unlikely(!netif_running(ndev))) { >> /* In dual emac mode check for all interfaces */
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
On Fri, 2018-07-27 at 22:36 +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. True for the source code but it compiles to more object code. $ cat foo.c struct cpsw_common { struct { int dual_emac; } data; struct { int ndev; } slaves[2]; }; struct sk_buff { int dev; }; #define CPDMA_RX_SOURCE_PORT(__status__) ((__status__ >> 16) & 0x7) #if defined SWITCH void foo(struct cpsw_common *cpsw, int status, struct sk_buff *skb) { if (!cpsw->data.dual_emac) return; switch (CPDMA_RX_SOURCE_PORT(status)) { case 1: skb->dev = cpsw->slaves[0].ndev; break; case 2: skb->dev = cpsw->slaves[1].ndev; break; } } #else void foo(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; } #endif $ gcc -c -O2 -DSWITCH foo.c $ size foo.o text data bss dec hex filename 94 0 0 94 5e foo.o $ objdump -d foo.o foo.o: file format elf64-x86-64 Disassembly of section .text: 0000000000000000 <foo>: 0: 8b 07 mov (%rdi),%eax 2: 85 c0 test %eax,%eax 4: 74 15 je 1b <foo+0x1b> 6: c1 fe 10 sar $0x10,%esi 9: 83 e6 07 and $0x7,%esi c: 83 fe 01 cmp $0x1,%esi f: 74 17 je 28 <foo+0x28> 11: 83 fe 02 cmp $0x2,%esi 14: 75 0a jne 20 <foo+0x20> 16: 8b 47 08 mov 0x8(%rdi),%eax 19: 89 02 mov %eax,(%rdx) 1b: f3 c3 repz retq 1d: 0f 1f 00 nopl (%rax) 20: f3 c3 repz retq 22: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1) 28: 8b 47 04 mov 0x4(%rdi),%eax 2b: 89 02 mov %eax,(%rdx) 2d: c3 retq $ gcc -c -O2 foo.c $ size foo.o text data bss dec hex filename 102 0 0 102 66 foo.o $ objdump -d foo.o foo.o: file format elf64-x86-64 Disassembly of section .text: 0000000000000000 <foo>: 0: 8b 07 mov (%rdi),%eax 2: 85 c0 test %eax,%eax 4: 74 10 je 16 <foo+0x16> 6: c1 fe 10 sar $0x10,%esi 9: 83 e6 07 and $0x7,%esi c: 83 fe 01 cmp $0x1,%esi f: 74 0f je 20 <foo+0x20> 11: 83 fe 02 cmp $0x2,%esi 14: 74 1a je 30 <foo+0x30> 16: f3 c3 repz retq 18: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1) 1f: 00 20: 8b 47 04 mov 0x4(%rdi),%eax 23: 89 02 mov %eax,(%rdx) 25: c3 retq 26: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1) 2d: 00 00 00 30: 8b 47 08 mov 0x8(%rdi),%eax 33: 89 02 mov %eax,(%rdx) 35: c3 retq -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 27, 2018 at 01:04:22PM -0700, Joe Perches wrote: >On Fri, 2018-07-27 at 22:36 +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. > >True for the source code but it compiles to more object code. > >$ cat foo.c >struct cpsw_common { > struct { > int dual_emac; > } data; > struct { > int ndev; > } slaves[2]; >}; > >struct sk_buff { > int dev; >}; > >#define CPDMA_RX_SOURCE_PORT(__status__) ((__status__ >> 16) & 0x7) > >#if defined SWITCH > >void foo(struct cpsw_common *cpsw, int status, struct sk_buff *skb) >{ > if (!cpsw->data.dual_emac) > return; > > switch (CPDMA_RX_SOURCE_PORT(status)) { > case 1: > skb->dev = cpsw->slaves[0].ndev; > break; > case 2: > skb->dev = cpsw->slaves[1].ndev; > break; > } >} > >#else > >void foo(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; >} > >#endif >$ gcc -c -O2 -DSWITCH foo.c >$ size foo.o > text data bss dec hex filename > 94 0 0 94 5e foo.o >$ objdump -d foo.o > >foo.o: file format elf64-x86-64 > > >Disassembly of section .text: > >0000000000000000 <foo>: > 0: 8b 07 mov (%rdi),%eax > 2: 85 c0 test %eax,%eax > 4: 74 15 je 1b <foo+0x1b> > 6: c1 fe 10 sar $0x10,%esi > 9: 83 e6 07 and $0x7,%esi > c: 83 fe 01 cmp $0x1,%esi > f: 74 17 je 28 <foo+0x28> > 11: 83 fe 02 cmp $0x2,%esi > 14: 75 0a jne 20 <foo+0x20> > 16: 8b 47 08 mov 0x8(%rdi),%eax > 19: 89 02 mov %eax,(%rdx) > 1b: f3 c3 repz retq > 1d: 0f 1f 00 nopl (%rax) > 20: f3 c3 repz retq > 22: 66 0f 1f 44 00 00 nopw 0x0(%rax,%rax,1) > 28: 8b 47 04 mov 0x4(%rdi),%eax > 2b: 89 02 mov %eax,(%rdx) > 2d: c3 retq >$ gcc -c -O2 foo.c >$ size foo.o > text data bss dec hex filename > 102 0 0 102 66 foo.o >$ objdump -d foo.o > >foo.o: file format elf64-x86-64 > > >Disassembly of section .text: > >0000000000000000 <foo>: > 0: 8b 07 mov (%rdi),%eax > 2: 85 c0 test %eax,%eax > 4: 74 10 je 16 <foo+0x16> > 6: c1 fe 10 sar $0x10,%esi > 9: 83 e6 07 and $0x7,%esi > c: 83 fe 01 cmp $0x1,%esi > f: 74 0f je 20 <foo+0x20> > 11: 83 fe 02 cmp $0x2,%esi > 14: 74 1a je 30 <foo+0x30> > 16: f3 c3 repz retq > 18: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1) > 1f: 00 > 20: 8b 47 04 mov 0x4(%rdi),%eax > 23: 89 02 mov %eax,(%rdx) > 25: c3 retq > 26: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1) > 2d: 00 00 00 > 30: 8b 47 08 mov 0x8(%rdi),%eax > 33: 89 02 mov %eax,(%rdx) > 35: c3 retq > > This driver, mainly used for ARM For ARM, situation a little bit different: $ arm-linux-gnueabihf-gcc -c -O2 -DSWITCH foo.c $ size foo.o text data bss dec hex filename 32 0 0 32 20 foo.o $ arm-linux-gnueabihf-objdump -d foo.o foo.o: file format elf32-littlearm Disassembly of section .text: 00000000 <foo>: 0: 6803 ldr r3, [r0, #0] 2: b143 cbz r3, 16 <foo+0x16> 4: f3c1 4102 ubfx r1, r1, #16, #3 8: 2901 cmp r1, #1 a: d005 beq.n 18 <foo+0x18> c: 2902 cmp r1, #2 e: d000 beq.n 12 <foo+0x12> 10: 4770 bx lr 12: 6883 ldr r3, [r0, #8] 14: 6013 str r3, [r2, #0] 16: 4770 bx lr 18: 6843 ldr r3, [r0, #4] 1a: 6013 str r3, [r2, #0] 1c: 4770 bx lr 1e: bf00 nop $ arm-linux-gnueabihf-gcc -c -O2 foo.c $ size foo.o text data bss dec hex filename 28 0 0 28 1c foo.o $ arm-linux-gnueabihf-objdump -d foo.o foo.o: file format elf32-littlearm Disassembly of section .text: 00000000 <foo>: 0: 6803 ldr r3, [r0, #0] 2: b13b cbz r3, 14 <foo+0x14> 4: f3c1 4102 ubfx r1, r1, #16, #3 8: 2901 cmp r1, #1 a: d004 beq.n 16 <foo+0x16> c: 2902 cmp r1, #2 e: bf04 itt eq 10: 6883 ldreq r3, [r0, #8] 12: 6013 streq r3, [r2, #0] 14: 4770 bx lr 16: 6843 ldr r3, [r0, #4] 18: 6013 str r3, [r2, #0] 1a: 4770 bx lr So, it's shorter.
On Fri, 2018-07-27 at 23:23 +0300, Ivan Khoronzhuk wrote: > For ARM, situation a little bit different: > $ arm-linux-gnueabihf-gcc -c -O2 -DSWITCH foo.c > $ size foo.o > text data bss dec hex filename > 32 0 0 32 20 foo.o [] > $ arm-linux-gnueabihf-gcc -c -O2 foo.c > $ size foo.o > text data bss dec hex filename > 28 0 0 28 1c foo.o [] > So, it's shorter. No worries. I was kinda surprised the object code wasn't identical anyway. cheers, Joe -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> +static inline void cpsw_src_port_detect(struct cpsw_common *cpsw, int status, > + struct sk_buff *skb) Please don't use inline. Let the compiler decide. Andrew -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jul 27, 2018 at 10:32:03PM +0200, Andrew Lunn wrote: >> +static inline void cpsw_src_port_detect(struct cpsw_common *cpsw, int status, >> + struct sk_buff *skb) > >Please don't use inline. Let the compiler decide. > > Andrew Corrected in v2
diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c index 1b54c26c2bec..62326a98945b 100644 --- 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; +} + +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; if (unlikely(status < 0) || unlikely(!netif_running(ndev))) { /* In dual emac mode check for all interfaces */
Replace ugly macroses on functions. Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> --- Based on net-next/master drivers/net/ethernet/ti/cpsw.c | 63 +++++++++++++++++----------------- 1 file changed, 32 insertions(+), 31 deletions(-)