[net-next] net: ethernet: ti: cpsw: replace unnecessarily macroses on functions
diff mbox series

Message ID 20180727191318.18698-1-ivan.khoronzhuk@linaro.org
State New
Headers show
Series
  • [net-next] net: ethernet: ti: cpsw: replace unnecessarily macroses on functions
Related show

Commit Message

Ivan Khoronzhuk July 27, 2018, 7:13 p.m. UTC
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(-)

Comments

Grygorii Strashko July 27, 2018, 7:15 p.m. UTC | #1
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>
Joe Perches July 27, 2018, 7:21 p.m. UTC | #2
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
Ivan Khoronzhuk July 27, 2018, 7:36 p.m. UTC | #3
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 */
Ivan Khoronzhuk July 27, 2018, 7:52 p.m. UTC | #4
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
Joe Perches July 27, 2018, 8:04 p.m. UTC | #5
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
Ivan Khoronzhuk July 27, 2018, 8:23 p.m. UTC | #6
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.
Joe Perches July 27, 2018, 8:29 p.m. UTC | #7
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
Andrew Lunn July 27, 2018, 8:32 p.m. UTC | #8
> +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
Ivan Khoronzhuk July 27, 2018, 8:49 p.m. UTC | #9
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

Patch
diff mbox series

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 */