Re: [PATCH v4] net: thunderx: rework mac addresses list to u64 array

[Vadim Lomovtsev]

On Sun, Apr 08, 2018 at 12:42:00PM -0400, David Miller wrote:
> From: Vadim Lomovtsev 
> Date: Fri,  6 Apr 2018 12:53:54 -0700
> 
> > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct 
> > *work_arg)
> >   work.work);
> > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > union nic_mbx mbx = {};
> > -   struct xcast_addr *xaddr, *next;
> > +   int idx = 0;
> 
> No need to initialize idx.
> 
> > +   for (idx = 0; idx < vf_work->mc->count; idx++) {
> 
> As it is always explicitly initialized at, and only used inside of,
> this loop.

Ok, will post next version shortly.
Thanks for your time.

Vadim

Re: [PATCH v4] net: thunderx: rework mac addresses list to u64 array

[Vadim Lomovtsev]

On Sun, Apr 08, 2018 at 12:42:00PM -0400, David Miller wrote:
> From: Vadim Lomovtsev 
> Date: Fri,  6 Apr 2018 12:53:54 -0700
> 
> > @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct 
> > *work_arg)
> >   work.work);
> > struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
> > union nic_mbx mbx = {};
> > -   struct xcast_addr *xaddr, *next;
> > +   int idx = 0;
> 
> No need to initialize idx.
> 
> > +   for (idx = 0; idx < vf_work->mc->count; idx++) {
> 
> As it is always explicitly initialized at, and only used inside of,
> this loop.

Ok, will post next version shortly.
Thanks for your time.

Vadim

Re: [PATCH v4] net: thunderx: rework mac addresses list to u64 array

[David Miller]

From: Vadim Lomovtsev 
Date: Fri,  6 Apr 2018 12:53:54 -0700

> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct 
> *work_arg)
> work.work);
>   struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
>   union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + int idx = 0;

No need to initialize idx.

> + for (idx = 0; idx < vf_work->mc->count; idx++) {

As it is always explicitly initialized at, and only used inside of,
this loop.

Re: [PATCH v4] net: thunderx: rework mac addresses list to u64 array

[David Miller]

From: Vadim Lomovtsev 
Date: Fri,  6 Apr 2018 12:53:54 -0700

> @@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct 
> *work_arg)
> work.work);
>   struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
>   union nic_mbx mbx = {};
> - struct xcast_addr *xaddr, *next;
> + int idx = 0;

No need to initialize idx.

> + for (idx = 0; idx < vf_work->mc->count; idx++) {

As it is always explicitly initialized at, and only used inside of,
this loop.

[PATCH v4] net: thunderx: rework mac addresses list to u64 array

[Vadim Lomovtsev]

From: Vadim Lomovtsev 

It is too expensive to pass u64 values via linked list, instead
allocate array for them by overall number of mac addresses from netdev.

This eventually removes multiple kmalloc() calls, aviod memory
fragmentation and allow to put single null check on kmalloc
return value in order to prevent a potential null pointer dereference.

Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback 
implementation for VF")
Reported-by: Dan Carpenter 
Signed-off-by: Vadim Lomovtsev 
---
Changes from v1 to v2:
 - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
Changes from v2 to v3:
 - update commit description with 'Reported-by: Dan Carpenter';
 - update size calculations for mc list to offsetof() call
   instead of explicit arithmetic;
Changes from v3 to v4:
 - change loop control variable type from u8 to int, accordingly
   to mc_count size;
---
 drivers/net/ethernet/cavium/thunder/nic.h|  7 +-
 drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +---
 2 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nic.h 
b/drivers/net/ethernet/cavium/thunder/nic.h
index 5fc46c5..448d1fa 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -265,14 +265,9 @@ struct nicvf_drv_stats {
 
 struct cavium_ptp;
 
-struct xcast_addr {
-   struct list_head list;
-   u64  addr;
-};
-
 struct xcast_addr_list {
-   struct list_head list;
int  count;
+   u64  mc[];
 };
 
 struct nicvf_work {
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c 
b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31f..6bd5658 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct 
*work_arg)
  work.work);
struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
union nic_mbx mbx = {};
-   struct xcast_addr *xaddr, *next;
+   int idx = 0;
 
if (!vf_work)
return;
@@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct 
*work_arg)
/* check if we have any specific MACs to be added to PF DMAC filter */
if (vf_work->mc) {
/* now go through kernel list of MACs and add them one by one */
-   list_for_each_entry_safe(xaddr, next,
-_work->mc->list, list) {
+   for (idx = 0; idx < vf_work->mc->count; idx++) {
mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
-   mbx.xcast.data.mac = xaddr->addr;
+   mbx.xcast.data.mac = vf_work->mc->mc[idx];
nicvf_send_msg_to_pf(nic, );
-
-   /* after receiving ACK from PF release memory */
-   list_del(>list);
-   kfree(xaddr);
-   vf_work->mc->count--;
}
kfree(vf_work->mc);
}
@@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
mode |= BGX_XCAST_MCAST_FILTER;
/* here we need to copy mc addrs */
if (netdev_mc_count(netdev)) {
-   struct xcast_addr *xaddr;
-
-   mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
-   INIT_LIST_HEAD(_list->list);
+   mc_list = kmalloc(offsetof(typeof(*mc_list),
+  
mc[netdev_mc_count(netdev)]),
+ GFP_ATOMIC);
+   if (unlikely(!mc_list))
+   return;
+   mc_list->count = 0;
netdev_hw_addr_list_for_each(ha, >mc) {
-   xaddr = kmalloc(sizeof(*xaddr),
-   GFP_ATOMIC);
-   xaddr->addr =
+   mc_list->mc[mc_list->count] =
ether_addr_to_u64(ha->addr);
-   list_add_tail(>list,
- _list->list);
mc_list->count++;
}
}
-- 
1.8.3.1

[PATCH v4] net: thunderx: rework mac addresses list to u64 array

[Vadim Lomovtsev]

From: Vadim Lomovtsev 

It is too expensive to pass u64 values via linked list, instead
allocate array for them by overall number of mac addresses from netdev.

This eventually removes multiple kmalloc() calls, aviod memory
fragmentation and allow to put single null check on kmalloc
return value in order to prevent a potential null pointer dereference.

Addresses-Coverity-ID: 1467429 ("Dereference null return value")
Fixes: 37c3347eb247 ("net: thunderx: add ndo_set_rx_mode callback 
implementation for VF")
Reported-by: Dan Carpenter 
Signed-off-by: Vadim Lomovtsev 
---
Changes from v1 to v2:
 - C99 syntax: update xcast_addr_list struct field mc[0] -> mc[];
Changes from v2 to v3:
 - update commit description with 'Reported-by: Dan Carpenter';
 - update size calculations for mc list to offsetof() call
   instead of explicit arithmetic;
Changes from v3 to v4:
 - change loop control variable type from u8 to int, accordingly
   to mc_count size;
---
 drivers/net/ethernet/cavium/thunder/nic.h|  7 +-
 drivers/net/ethernet/cavium/thunder/nicvf_main.c | 28 +---
 2 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/cavium/thunder/nic.h 
b/drivers/net/ethernet/cavium/thunder/nic.h
index 5fc46c5..448d1fa 100644
--- a/drivers/net/ethernet/cavium/thunder/nic.h
+++ b/drivers/net/ethernet/cavium/thunder/nic.h
@@ -265,14 +265,9 @@ struct nicvf_drv_stats {
 
 struct cavium_ptp;
 
-struct xcast_addr {
-   struct list_head list;
-   u64  addr;
-};
-
 struct xcast_addr_list {
-   struct list_head list;
int  count;
+   u64  mc[];
 };
 
 struct nicvf_work {
diff --git a/drivers/net/ethernet/cavium/thunder/nicvf_main.c 
b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
index 1e9a31f..6bd5658 100644
--- a/drivers/net/ethernet/cavium/thunder/nicvf_main.c
+++ b/drivers/net/ethernet/cavium/thunder/nicvf_main.c
@@ -1929,7 +1929,7 @@ static void nicvf_set_rx_mode_task(struct work_struct 
*work_arg)
  work.work);
struct nicvf *nic = container_of(vf_work, struct nicvf, rx_mode_work);
union nic_mbx mbx = {};
-   struct xcast_addr *xaddr, *next;
+   int idx = 0;
 
if (!vf_work)
return;
@@ -1956,16 +1956,10 @@ static void nicvf_set_rx_mode_task(struct work_struct 
*work_arg)
/* check if we have any specific MACs to be added to PF DMAC filter */
if (vf_work->mc) {
/* now go through kernel list of MACs and add them one by one */
-   list_for_each_entry_safe(xaddr, next,
-_work->mc->list, list) {
+   for (idx = 0; idx < vf_work->mc->count; idx++) {
mbx.xcast.msg = NIC_MBOX_MSG_ADD_MCAST;
-   mbx.xcast.data.mac = xaddr->addr;
+   mbx.xcast.data.mac = vf_work->mc->mc[idx];
nicvf_send_msg_to_pf(nic, );
-
-   /* after receiving ACK from PF release memory */
-   list_del(>list);
-   kfree(xaddr);
-   vf_work->mc->count--;
}
kfree(vf_work->mc);
}
@@ -1996,17 +1990,15 @@ static void nicvf_set_rx_mode(struct net_device *netdev)
mode |= BGX_XCAST_MCAST_FILTER;
/* here we need to copy mc addrs */
if (netdev_mc_count(netdev)) {
-   struct xcast_addr *xaddr;
-
-   mc_list = kmalloc(sizeof(*mc_list), GFP_ATOMIC);
-   INIT_LIST_HEAD(_list->list);
+   mc_list = kmalloc(offsetof(typeof(*mc_list),
+  
mc[netdev_mc_count(netdev)]),
+ GFP_ATOMIC);
+   if (unlikely(!mc_list))
+   return;
+   mc_list->count = 0;
netdev_hw_addr_list_for_each(ha, >mc) {
-   xaddr = kmalloc(sizeof(*xaddr),
-   GFP_ATOMIC);
-   xaddr->addr =
+   mc_list->mc[mc_list->count] =
ether_addr_to_u64(ha->addr);
-   list_add_tail(>list,
- _list->list);
mc_list->count++;
}
}
-- 
1.8.3.1