* [PATCH v2] net: macb: change GFP_KERNEL to GFP_ATOMIC @ 2017-12-02 7:01 Julia Lawall 2017-12-05 16:27 ` David Miller 2017-12-05 20:17 ` [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions Julia Cartwright 0 siblings, 2 replies; 13+ messages in thread From: Julia Lawall @ 2017-12-02 7:01 UTC (permalink / raw) To: Rafal Ozieblo; +Cc: Nicolas Ferre, netdev, linux-kernel, kbuild-all Function gem_add_flow_filter called on line 2958 inside lock on line 2949 but uses GFP_KERNEL Generated by: scripts/coccinelle/locks/call_kern.cocci Fixes: ae8223de3df5 ("net: macb: Added support for RX filtering") CC: Rafal Ozieblo <rafalo@cadence.com> Signed-off-by: Julia Lawall <julia.lawall@lip6.fr> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com> --- v2: Fix some broken email addresses. No change to the patch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master head: fb20eb9d798d2f4c1a75b7fe981d72dfa8d7270d commit: ae8223de3df5a0ce651d14a50dad31b9cae029f2 [2033/2251] net: macb: Added support for RX filtering macb_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -2799,7 +2799,7 @@ static int gem_add_flow_filter(struct ne int ret = -EINVAL; bool added = false; - newfs = kmalloc(sizeof(*newfs), GFP_KERNEL); + newfs = kmalloc(sizeof(*newfs), GFP_ATOMIC); if (newfs == NULL) return -ENOMEM; memcpy(&newfs->fs, fs, sizeof(newfs->fs)); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] net: macb: change GFP_KERNEL to GFP_ATOMIC 2017-12-02 7:01 [PATCH v2] net: macb: change GFP_KERNEL to GFP_ATOMIC Julia Lawall @ 2017-12-05 16:27 ` David Miller 2017-12-05 20:17 ` [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions Julia Cartwright 1 sibling, 0 replies; 13+ messages in thread From: David Miller @ 2017-12-05 16:27 UTC (permalink / raw) To: julia.lawall; +Cc: rafalo, nicolas.ferre, netdev, linux-kernel, kbuild-all From: Julia Lawall <julia.lawall@lip6.fr> Date: Sat, 2 Dec 2017 08:01:21 +0100 (CET) > Function gem_add_flow_filter called on line 2958 inside lock on line 2949 > but uses GFP_KERNEL > > Generated by: scripts/coccinelle/locks/call_kern.cocci > > Fixes: ae8223de3df5 ("net: macb: Added support for RX filtering") > CC: Rafal Ozieblo <rafalo@cadence.com> > Signed-off-by: Julia Lawall <julia.lawall@lip6.fr> > Signed-off-by: Fengguang Wu <fengguang.wu@intel.com> Applied to net-next. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions 2017-12-02 7:01 [PATCH v2] net: macb: change GFP_KERNEL to GFP_ATOMIC Julia Lawall 2017-12-05 16:27 ` David Miller @ 2017-12-05 20:17 ` Julia Cartwright 2017-12-05 20:18 ` [PATCH 2/2] net: macb: kill useless use of list_empty() Julia Cartwright ` (2 more replies) 1 sibling, 3 replies; 13+ messages in thread From: Julia Cartwright @ 2017-12-05 20:17 UTC (permalink / raw) To: Julia Lawall Cc: Rafal Ozieblo, Nicolas Ferre, netdev, linux-kernel, kbuild-all Commit ae8223de3df5 ("net: macb: Added support for RX filtering") introduces a lock, rx_fs_lock which is intended to protect the list of rx_flow items and synchronize access to the hardware rx filtering registers. However, the region protected by this lock is overscoped, unnecessarily including things like slab allocation. Reduce this lock scope to only include operations which must be performed atomically: list traversal, addition, and removal, and hitting the macb filtering registers. This fixes the use of kmalloc w/ GFP_KERNEL in atomic context. Fixes: ae8223de3df5 ("net: macb: Added support for RX filtering") Cc: Rafal Ozieblo <rafalo@cadence.com> Cc: Julia Lawall <julia.lawall@lip6.fr> Signed-off-by: Julia Cartwright <julia@ni.com> --- While Julia Lawall's cocci-generated patch fixes the problem, the right solution is to obviate the problem altogether. Thanks, The Other Julia drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index c5fa87cdc6c4..e7ef104a077d 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -2796,6 +2796,7 @@ static int gem_add_flow_filter(struct net_device *netdev, struct macb *bp = netdev_priv(netdev); struct ethtool_rx_flow_spec *fs = &cmd->fs; struct ethtool_rx_fs_item *item, *newfs; + unsigned long flags; int ret = -EINVAL; bool added = false; @@ -2811,6 +2812,8 @@ static int gem_add_flow_filter(struct net_device *netdev, htonl(fs->h_u.tcp_ip4_spec.ip4dst), htons(fs->h_u.tcp_ip4_spec.psrc), htons(fs->h_u.tcp_ip4_spec.pdst)); + spin_lock_irqsave(&bp->rx_fs_lock, flags); + /* find correct place to add in list */ if (list_empty(&bp->rx_fs_list.list)) list_add(&newfs->list, &bp->rx_fs_list.list); @@ -2837,9 +2840,11 @@ static int gem_add_flow_filter(struct net_device *netdev, if (netdev->features & NETIF_F_NTUPLE) gem_enable_flow_filters(bp, 1); + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); return 0; err: + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); kfree(newfs); return ret; } @@ -2850,9 +2855,14 @@ static int gem_del_flow_filter(struct net_device *netdev, struct macb *bp = netdev_priv(netdev); struct ethtool_rx_fs_item *item; struct ethtool_rx_flow_spec *fs; + unsigned long flags; - if (list_empty(&bp->rx_fs_list.list)) + spin_lock_irqsave(&bp->rx_fs_lock, flags); + + if (list_empty(&bp->rx_fs_list.list)) { + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); return -EINVAL; + } list_for_each_entry(item, &bp->rx_fs_list.list, list) { if (item->fs.location == cmd->fs.location) { @@ -2869,12 +2879,14 @@ static int gem_del_flow_filter(struct net_device *netdev, gem_writel_n(bp, SCRT2, fs->location, 0); list_del(&item->list); - kfree(item); bp->rx_fs_list.count--; + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); + kfree(item); return 0; } } + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); return -EINVAL; } @@ -2943,11 +2955,8 @@ static int gem_get_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd, static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd) { struct macb *bp = netdev_priv(netdev); - unsigned long flags; int ret; - spin_lock_irqsave(&bp->rx_fs_lock, flags); - switch (cmd->cmd) { case ETHTOOL_SRXCLSRLINS: if ((cmd->fs.location >= bp->max_tuples) @@ -2966,7 +2975,6 @@ static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd) ret = -EOPNOTSUPP; } - spin_unlock_irqrestore(&bp->rx_fs_lock, flags); return ret; } -- 2.14.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] net: macb: kill useless use of list_empty() 2017-12-05 20:17 ` [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions Julia Cartwright @ 2017-12-05 20:18 ` Julia Cartwright 2017-12-05 21:28 ` Nicolas Ferre 2017-12-05 21:26 ` [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions Nicolas Ferre 2017-12-05 23:00 ` David Miller 2 siblings, 1 reply; 13+ messages in thread From: Julia Cartwright @ 2017-12-05 20:18 UTC (permalink / raw) To: Julia Lawall Cc: Rafal Ozieblo, Nicolas Ferre, netdev, linux-kernel, kbuild-all The list_for_each_entry() macro already handles the case where the list is empty (by not executing the loop body). It's not necessary to handle this case specially, so stop doing so. Cc: Rafal Ozieblo <rafalo@cadence.com> Signed-off-by: Julia Cartwright <julia@ni.com> --- This is an additional cleanup patch found when looking at this code. Julia drivers/net/ethernet/cadence/macb_main.c | 34 ++++++++++++-------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index e7ef104a077d..3643c6ad2322 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -2815,25 +2815,22 @@ static int gem_add_flow_filter(struct net_device *netdev, spin_lock_irqsave(&bp->rx_fs_lock, flags); /* find correct place to add in list */ - if (list_empty(&bp->rx_fs_list.list)) - list_add(&newfs->list, &bp->rx_fs_list.list); - else { - list_for_each_entry(item, &bp->rx_fs_list.list, list) { - if (item->fs.location > newfs->fs.location) { - list_add_tail(&newfs->list, &item->list); - added = true; - break; - } else if (item->fs.location == fs->location) { - netdev_err(netdev, "Rule not added: location %d not free!\n", - fs->location); - ret = -EBUSY; - goto err; - } + list_for_each_entry(item, &bp->rx_fs_list.list, list) { + if (item->fs.location > newfs->fs.location) { + list_add_tail(&newfs->list, &item->list); + added = true; + break; + } else if (item->fs.location == fs->location) { + netdev_err(netdev, "Rule not added: location %d not free!\n", + fs->location); + ret = -EBUSY; + goto err; } - if (!added) - list_add_tail(&newfs->list, &bp->rx_fs_list.list); } + if (!added) + list_add_tail(&newfs->list, &bp->rx_fs_list.list); + gem_prog_cmp_regs(bp, fs); bp->rx_fs_list.count++; /* enable filtering if NTUPLE on */ @@ -2859,11 +2856,6 @@ static int gem_del_flow_filter(struct net_device *netdev, spin_lock_irqsave(&bp->rx_fs_lock, flags); - if (list_empty(&bp->rx_fs_list.list)) { - spin_unlock_irqrestore(&bp->rx_fs_lock, flags); - return -EINVAL; - } - list_for_each_entry(item, &bp->rx_fs_list.list, list) { if (item->fs.location == cmd->fs.location) { /* disable screener regs for the flow entry */ -- 2.14.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] net: macb: kill useless use of list_empty() 2017-12-05 20:18 ` [PATCH 2/2] net: macb: kill useless use of list_empty() Julia Cartwright @ 2017-12-05 21:28 ` Nicolas Ferre 0 siblings, 0 replies; 13+ messages in thread From: Nicolas Ferre @ 2017-12-05 21:28 UTC (permalink / raw) To: Julia Cartwright, Julia Lawall Cc: Rafal Ozieblo, netdev, linux-kernel, kbuild-all On 05/12/2017 at 21:18, Julia Cartwright wrote: > The list_for_each_entry() macro already handles the case where the list > is empty (by not executing the loop body). It's not necessary to handle > this case specially, so stop doing so. > > Cc: Rafal Ozieblo <rafalo@cadence.com> > Signed-off-by: Julia Cartwright <julia@ni.com> > --- > This is an additional cleanup patch found when looking at this code. > > Julia Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> Thanks > > drivers/net/ethernet/cadence/macb_main.c | 34 ++++++++++++-------------------- > 1 file changed, 13 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index e7ef104a077d..3643c6ad2322 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -2815,25 +2815,22 @@ static int gem_add_flow_filter(struct net_device *netdev, > spin_lock_irqsave(&bp->rx_fs_lock, flags); > > /* find correct place to add in list */ > - if (list_empty(&bp->rx_fs_list.list)) > - list_add(&newfs->list, &bp->rx_fs_list.list); > - else { > - list_for_each_entry(item, &bp->rx_fs_list.list, list) { > - if (item->fs.location > newfs->fs.location) { > - list_add_tail(&newfs->list, &item->list); > - added = true; > - break; > - } else if (item->fs.location == fs->location) { > - netdev_err(netdev, "Rule not added: location %d not free!\n", > - fs->location); > - ret = -EBUSY; > - goto err; > - } > + list_for_each_entry(item, &bp->rx_fs_list.list, list) { > + if (item->fs.location > newfs->fs.location) { > + list_add_tail(&newfs->list, &item->list); > + added = true; > + break; > + } else if (item->fs.location == fs->location) { > + netdev_err(netdev, "Rule not added: location %d not free!\n", > + fs->location); > + ret = -EBUSY; > + goto err; > } > - if (!added) > - list_add_tail(&newfs->list, &bp->rx_fs_list.list); > } > > + if (!added) > + list_add_tail(&newfs->list, &bp->rx_fs_list.list); > + > gem_prog_cmp_regs(bp, fs); > bp->rx_fs_list.count++; > /* enable filtering if NTUPLE on */ > @@ -2859,11 +2856,6 @@ static int gem_del_flow_filter(struct net_device *netdev, > > spin_lock_irqsave(&bp->rx_fs_lock, flags); > > - if (list_empty(&bp->rx_fs_list.list)) { > - spin_unlock_irqrestore(&bp->rx_fs_lock, flags); > - return -EINVAL; > - } > - > list_for_each_entry(item, &bp->rx_fs_list.list, list) { > if (item->fs.location == cmd->fs.location) { > /* disable screener regs for the flow entry */ > -- Nicolas Ferre ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions 2017-12-05 20:17 ` [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions Julia Cartwright 2017-12-05 20:18 ` [PATCH 2/2] net: macb: kill useless use of list_empty() Julia Cartwright @ 2017-12-05 21:26 ` Nicolas Ferre 2017-12-05 23:00 ` David Miller 2 siblings, 0 replies; 13+ messages in thread From: Nicolas Ferre @ 2017-12-05 21:26 UTC (permalink / raw) To: Julia Cartwright, Julia Lawall Cc: Rafal Ozieblo, netdev, linux-kernel, kbuild-all On 05/12/2017 at 21:17, Julia Cartwright wrote: > Commit ae8223de3df5 ("net: macb: Added support for RX filtering") > introduces a lock, rx_fs_lock which is intended to protect the list of > rx_flow items and synchronize access to the hardware rx filtering > registers. > > However, the region protected by this lock is overscoped, unnecessarily > including things like slab allocation. Reduce this lock scope to only > include operations which must be performed atomically: list traversal, > addition, and removal, and hitting the macb filtering registers. > > This fixes the use of kmalloc w/ GFP_KERNEL in atomic context. > > Fixes: ae8223de3df5 ("net: macb: Added support for RX filtering") > Cc: Rafal Ozieblo <rafalo@cadence.com> > Cc: Julia Lawall <julia.lawall@lip6.fr> > Signed-off-by: Julia Cartwright <julia@ni.com> > --- > While Julia Lawall's cocci-generated patch fixes the problem, the right > solution is to obviate the problem altogether. > > Thanks, > The Other Julia Julia, Thanks for your patch, it seems good indeed. Here is my: Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> As the patch by Julia L. is already in net-next, I suspect that you would need to add a kind of revert patch if we want to come back to a more usual GFP_KERNEL for the kmalloc. Best regards, Nicolas > drivers/net/ethernet/cadence/macb_main.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index c5fa87cdc6c4..e7ef104a077d 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -2796,6 +2796,7 @@ static int gem_add_flow_filter(struct net_device *netdev, > struct macb *bp = netdev_priv(netdev); > struct ethtool_rx_flow_spec *fs = &cmd->fs; > struct ethtool_rx_fs_item *item, *newfs; > + unsigned long flags; > int ret = -EINVAL; > bool added = false; > > @@ -2811,6 +2812,8 @@ static int gem_add_flow_filter(struct net_device *netdev, > htonl(fs->h_u.tcp_ip4_spec.ip4dst), > htons(fs->h_u.tcp_ip4_spec.psrc), htons(fs->h_u.tcp_ip4_spec.pdst)); > > + spin_lock_irqsave(&bp->rx_fs_lock, flags); > + > /* find correct place to add in list */ > if (list_empty(&bp->rx_fs_list.list)) > list_add(&newfs->list, &bp->rx_fs_list.list); > @@ -2837,9 +2840,11 @@ static int gem_add_flow_filter(struct net_device *netdev, > if (netdev->features & NETIF_F_NTUPLE) > gem_enable_flow_filters(bp, 1); > > + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); > return 0; > > err: > + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); > kfree(newfs); > return ret; > } > @@ -2850,9 +2855,14 @@ static int gem_del_flow_filter(struct net_device *netdev, > struct macb *bp = netdev_priv(netdev); > struct ethtool_rx_fs_item *item; > struct ethtool_rx_flow_spec *fs; > + unsigned long flags; > > - if (list_empty(&bp->rx_fs_list.list)) > + spin_lock_irqsave(&bp->rx_fs_lock, flags); > + > + if (list_empty(&bp->rx_fs_list.list)) { > + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); > return -EINVAL; > + } > > list_for_each_entry(item, &bp->rx_fs_list.list, list) { > if (item->fs.location == cmd->fs.location) { > @@ -2869,12 +2879,14 @@ static int gem_del_flow_filter(struct net_device *netdev, > gem_writel_n(bp, SCRT2, fs->location, 0); > > list_del(&item->list); > - kfree(item); > bp->rx_fs_list.count--; > + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); > + kfree(item); > return 0; > } > } > > + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); > return -EINVAL; > } > > @@ -2943,11 +2955,8 @@ static int gem_get_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd, > static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd) > { > struct macb *bp = netdev_priv(netdev); > - unsigned long flags; > int ret; > > - spin_lock_irqsave(&bp->rx_fs_lock, flags); > - > switch (cmd->cmd) { > case ETHTOOL_SRXCLSRLINS: > if ((cmd->fs.location >= bp->max_tuples) > @@ -2966,7 +2975,6 @@ static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd) > ret = -EOPNOTSUPP; > } > > - spin_unlock_irqrestore(&bp->rx_fs_lock, flags); > return ret; > } > > -- Nicolas Ferre ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions 2017-12-05 20:17 ` [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions Julia Cartwright 2017-12-05 20:18 ` [PATCH 2/2] net: macb: kill useless use of list_empty() Julia Cartwright 2017-12-05 21:26 ` [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions Nicolas Ferre @ 2017-12-05 23:00 ` David Miller 2017-12-06 0:02 ` [PATCH v2 0/3] macb rx filter cleanups Julia Cartwright 2 siblings, 1 reply; 13+ messages in thread From: David Miller @ 2017-12-05 23:00 UTC (permalink / raw) To: julia Cc: julia.lawall, rafalo, nicolas.ferre, netdev, linux-kernel, kbuild-all From: Julia Cartwright <julia@ni.com> Date: Tue, 5 Dec 2017 14:17:11 -0600 > While Julia Lawall's cocci-generated patch fixes the problem, the right > solution is to obviate the problem altogether. I already applied Julia's patch. And I hope that if you generated this against current net-next you would have seen that. So you'll need to redo this series and put the GFP_KERNEL back. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 0/3] macb rx filter cleanups 2017-12-05 23:00 ` David Miller @ 2017-12-06 0:02 ` Julia Cartwright 2017-12-06 0:02 ` [PATCH v2 1/3] net: macb: kill useless use of list_empty() Julia Cartwright ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Julia Cartwright @ 2017-12-06 0:02 UTC (permalink / raw) To: David Miller Cc: julia.lawall, rafalo, nicolas.ferre, netdev, linux-kernel, kbuild-all Here's a proper patchset based on net-next. v1 -> v2: - Rebased on net-next - Add Nicolas's Acks - Reorder commits, putting the list_empty() cleanups prior to the others. - Added commit reverting the GFP_ATOMIC change. Julia Cartwright (3): net: macb: kill useless use of list_empty() net: macb: reduce scope of rx_fs_lock-protected regions net: macb: change GFP_ATOMIC to GFP_KERNEL drivers/net/ethernet/cadence/macb_main.c | 47 ++++++++++++++++---------------- 1 file changed, 23 insertions(+), 24 deletions(-) -- 2.14.2 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/3] net: macb: kill useless use of list_empty() 2017-12-06 0:02 ` [PATCH v2 0/3] macb rx filter cleanups Julia Cartwright @ 2017-12-06 0:02 ` Julia Cartwright 2017-12-06 0:02 ` [PATCH v2 2/3] net: macb: reduce scope of rx_fs_lock-protected regions Julia Cartwright ` (2 subsequent siblings) 3 siblings, 0 replies; 13+ messages in thread From: Julia Cartwright @ 2017-12-06 0:02 UTC (permalink / raw) To: David Miller Cc: julia.lawall, rafalo, nicolas.ferre, netdev, linux-kernel, kbuild-all The list_for_each_entry() macro already handles the case where the list is empty (by not executing the loop body). It's not necessary to handle this case specially, so stop doing so. Cc: Rafal Ozieblo <rafalo@cadence.com> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> Signed-off-by: Julia Cartwright <julia@ni.com> --- drivers/net/ethernet/cadence/macb_main.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index ebfeab853bf4..b7644836aba1 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -2812,24 +2812,20 @@ static int gem_add_flow_filter(struct net_device *netdev, htons(fs->h_u.tcp_ip4_spec.psrc), htons(fs->h_u.tcp_ip4_spec.pdst)); /* find correct place to add in list */ - if (list_empty(&bp->rx_fs_list.list)) - list_add(&newfs->list, &bp->rx_fs_list.list); - else { - list_for_each_entry(item, &bp->rx_fs_list.list, list) { - if (item->fs.location > newfs->fs.location) { - list_add_tail(&newfs->list, &item->list); - added = true; - break; - } else if (item->fs.location == fs->location) { - netdev_err(netdev, "Rule not added: location %d not free!\n", - fs->location); - ret = -EBUSY; - goto err; - } + list_for_each_entry(item, &bp->rx_fs_list.list, list) { + if (item->fs.location > newfs->fs.location) { + list_add_tail(&newfs->list, &item->list); + added = true; + break; + } else if (item->fs.location == fs->location) { + netdev_err(netdev, "Rule not added: location %d not free!\n", + fs->location); + ret = -EBUSY; + goto err; } - if (!added) - list_add_tail(&newfs->list, &bp->rx_fs_list.list); } + if (!added) + list_add_tail(&newfs->list, &bp->rx_fs_list.list); gem_prog_cmp_regs(bp, fs); bp->rx_fs_list.count++; @@ -2851,9 +2847,6 @@ static int gem_del_flow_filter(struct net_device *netdev, struct ethtool_rx_fs_item *item; struct ethtool_rx_flow_spec *fs; - if (list_empty(&bp->rx_fs_list.list)) - return -EINVAL; - list_for_each_entry(item, &bp->rx_fs_list.list, list) { if (item->fs.location == cmd->fs.location) { /* disable screener regs for the flow entry */ -- 2.14.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/3] net: macb: reduce scope of rx_fs_lock-protected regions 2017-12-06 0:02 ` [PATCH v2 0/3] macb rx filter cleanups Julia Cartwright 2017-12-06 0:02 ` [PATCH v2 1/3] net: macb: kill useless use of list_empty() Julia Cartwright @ 2017-12-06 0:02 ` Julia Cartwright 2017-12-06 0:02 ` [PATCH v2 3/3] net: macb: change GFP_ATOMIC to GFP_KERNEL Julia Cartwright 2017-12-06 1:08 ` [PATCH v2 0/3] macb rx filter cleanups David Miller 3 siblings, 0 replies; 13+ messages in thread From: Julia Cartwright @ 2017-12-06 0:02 UTC (permalink / raw) To: David Miller Cc: julia.lawall, rafalo, nicolas.ferre, netdev, linux-kernel, kbuild-all Commit ae8223de3df5 ("net: macb: Added support for RX filtering") introduces a lock, rx_fs_lock which is intended to protect the list of rx_flow items and synchronize access to the hardware rx filtering registers. However, the region protected by this lock is overscoped, unnecessarily including things like slab allocation. Reduce this lock scope to only include operations which must be performed atomically: list traversal, addition, and removal, and hitting the macb filtering registers. This fixes the use of kmalloc w/ GFP_KERNEL in atomic context. Fixes: ae8223de3df5 ("net: macb: Added support for RX filtering") Cc: Rafal Ozieblo <rafalo@cadence.com> Cc: Julia Lawall <julia.lawall@lip6.fr> Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> Signed-off-by: Julia Cartwright <julia@ni.com> --- drivers/net/ethernet/cadence/macb_main.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index b7644836aba1..758e8b3042b2 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -2796,6 +2796,7 @@ static int gem_add_flow_filter(struct net_device *netdev, struct macb *bp = netdev_priv(netdev); struct ethtool_rx_flow_spec *fs = &cmd->fs; struct ethtool_rx_fs_item *item, *newfs; + unsigned long flags; int ret = -EINVAL; bool added = false; @@ -2811,6 +2812,8 @@ static int gem_add_flow_filter(struct net_device *netdev, htonl(fs->h_u.tcp_ip4_spec.ip4dst), htons(fs->h_u.tcp_ip4_spec.psrc), htons(fs->h_u.tcp_ip4_spec.pdst)); + spin_lock_irqsave(&bp->rx_fs_lock, flags); + /* find correct place to add in list */ list_for_each_entry(item, &bp->rx_fs_list.list, list) { if (item->fs.location > newfs->fs.location) { @@ -2833,9 +2836,11 @@ static int gem_add_flow_filter(struct net_device *netdev, if (netdev->features & NETIF_F_NTUPLE) gem_enable_flow_filters(bp, 1); + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); return 0; err: + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); kfree(newfs); return ret; } @@ -2846,6 +2851,9 @@ static int gem_del_flow_filter(struct net_device *netdev, struct macb *bp = netdev_priv(netdev); struct ethtool_rx_fs_item *item; struct ethtool_rx_flow_spec *fs; + unsigned long flags; + + spin_lock_irqsave(&bp->rx_fs_lock, flags); list_for_each_entry(item, &bp->rx_fs_list.list, list) { if (item->fs.location == cmd->fs.location) { @@ -2862,12 +2870,14 @@ static int gem_del_flow_filter(struct net_device *netdev, gem_writel_n(bp, SCRT2, fs->location, 0); list_del(&item->list); - kfree(item); bp->rx_fs_list.count--; + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); + kfree(item); return 0; } } + spin_unlock_irqrestore(&bp->rx_fs_lock, flags); return -EINVAL; } @@ -2936,11 +2946,8 @@ static int gem_get_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd, static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd) { struct macb *bp = netdev_priv(netdev); - unsigned long flags; int ret; - spin_lock_irqsave(&bp->rx_fs_lock, flags); - switch (cmd->cmd) { case ETHTOOL_SRXCLSRLINS: if ((cmd->fs.location >= bp->max_tuples) @@ -2959,7 +2966,6 @@ static int gem_set_rxnfc(struct net_device *netdev, struct ethtool_rxnfc *cmd) ret = -EOPNOTSUPP; } - spin_unlock_irqrestore(&bp->rx_fs_lock, flags); return ret; } -- 2.14.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] net: macb: change GFP_ATOMIC to GFP_KERNEL 2017-12-06 0:02 ` [PATCH v2 0/3] macb rx filter cleanups Julia Cartwright 2017-12-06 0:02 ` [PATCH v2 1/3] net: macb: kill useless use of list_empty() Julia Cartwright 2017-12-06 0:02 ` [PATCH v2 2/3] net: macb: reduce scope of rx_fs_lock-protected regions Julia Cartwright @ 2017-12-06 0:02 ` Julia Cartwright 2017-12-06 5:49 ` Julia Lawall 2017-12-06 1:08 ` [PATCH v2 0/3] macb rx filter cleanups David Miller 3 siblings, 1 reply; 13+ messages in thread From: Julia Cartwright @ 2017-12-06 0:02 UTC (permalink / raw) To: David Miller Cc: julia.lawall, rafalo, nicolas.ferre, netdev, linux-kernel, kbuild-all Now that the rx_fs_lock is no longer held across allocation, it's safe to use GFP_KERNEL for allocating new entries. This reverts commit 81da3bf6e3f88 ("net: macb: change GFP_KERNEL to GFP_ATOMIC"). Cc: Julia Lawall <julia.lawall@lip6.fr> Signed-off-by: Julia Cartwright <julia@ni.com> --- drivers/net/ethernet/cadence/macb_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 758e8b3042b2..234667eaaa92 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -2800,7 +2800,7 @@ static int gem_add_flow_filter(struct net_device *netdev, int ret = -EINVAL; bool added = false; - newfs = kmalloc(sizeof(*newfs), GFP_ATOMIC); + newfs = kmalloc(sizeof(*newfs), GFP_KERNEL); if (newfs == NULL) return -ENOMEM; memcpy(&newfs->fs, fs, sizeof(newfs->fs)); -- 2.14.2 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/3] net: macb: change GFP_ATOMIC to GFP_KERNEL 2017-12-06 0:02 ` [PATCH v2 3/3] net: macb: change GFP_ATOMIC to GFP_KERNEL Julia Cartwright @ 2017-12-06 5:49 ` Julia Lawall 0 siblings, 0 replies; 13+ messages in thread From: Julia Lawall @ 2017-12-06 5:49 UTC (permalink / raw) To: Julia Cartwright Cc: David Miller, Julia Lawall, rafalo, nicolas.ferre, netdev, linux-kernel, kbuild-all On Tue, 5 Dec 2017, Julia Cartwright wrote: > Now that the rx_fs_lock is no longer held across allocation, it's safe > to use GFP_KERNEL for allocating new entries. > > This reverts commit 81da3bf6e3f88 ("net: macb: change GFP_KERNEL to > GFP_ATOMIC"). > > Cc: Julia Lawall <julia.lawall@lip6.fr> > Signed-off-by: Julia Cartwright <julia@ni.com> Acked-by: Julia Lawall <julia.lawall@lip6.fr> > --- > drivers/net/ethernet/cadence/macb_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 758e8b3042b2..234667eaaa92 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -2800,7 +2800,7 @@ static int gem_add_flow_filter(struct net_device *netdev, > int ret = -EINVAL; > bool added = false; > > - newfs = kmalloc(sizeof(*newfs), GFP_ATOMIC); > + newfs = kmalloc(sizeof(*newfs), GFP_KERNEL); > if (newfs == NULL) > return -ENOMEM; > memcpy(&newfs->fs, fs, sizeof(newfs->fs)); > -- > 2.14.2 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/3] macb rx filter cleanups 2017-12-06 0:02 ` [PATCH v2 0/3] macb rx filter cleanups Julia Cartwright ` (2 preceding siblings ...) 2017-12-06 0:02 ` [PATCH v2 3/3] net: macb: change GFP_ATOMIC to GFP_KERNEL Julia Cartwright @ 2017-12-06 1:08 ` David Miller 3 siblings, 0 replies; 13+ messages in thread From: David Miller @ 2017-12-06 1:08 UTC (permalink / raw) To: julia Cc: julia.lawall, rafalo, nicolas.ferre, netdev, linux-kernel, kbuild-all From: Julia Cartwright <julia@ni.com> Date: Tue, 5 Dec 2017 18:02:47 -0600 > Here's a proper patchset based on net-next. > > v1 -> v2: > - Rebased on net-next > - Add Nicolas's Acks > - Reorder commits, putting the list_empty() cleanups prior to the > others. > - Added commit reverting the GFP_ATOMIC change. Thanks for following up so quickly. Series applied to net-next, thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-12-06 5:49 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-02 7:01 [PATCH v2] net: macb: change GFP_KERNEL to GFP_ATOMIC Julia Lawall 2017-12-05 16:27 ` David Miller 2017-12-05 20:17 ` [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions Julia Cartwright 2017-12-05 20:18 ` [PATCH 2/2] net: macb: kill useless use of list_empty() Julia Cartwright 2017-12-05 21:28 ` Nicolas Ferre 2017-12-05 21:26 ` [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions Nicolas Ferre 2017-12-05 23:00 ` David Miller 2017-12-06 0:02 ` [PATCH v2 0/3] macb rx filter cleanups Julia Cartwright 2017-12-06 0:02 ` [PATCH v2 1/3] net: macb: kill useless use of list_empty() Julia Cartwright 2017-12-06 0:02 ` [PATCH v2 2/3] net: macb: reduce scope of rx_fs_lock-protected regions Julia Cartwright 2017-12-06 0:02 ` [PATCH v2 3/3] net: macb: change GFP_ATOMIC to GFP_KERNEL Julia Cartwright 2017-12-06 5:49 ` Julia Lawall 2017-12-06 1:08 ` [PATCH v2 0/3] macb rx filter cleanups David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).