lkml.org 
[lkml]   [2018]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2018-07-27 21:53    [W:0.060 / U:0.232 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site