Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752036AbdLEV0f (ORCPT ); Tue, 5 Dec 2017 16:26:35 -0500 Received: from esa5.microchip.iphmx.com ([216.71.150.166]:36088 "EHLO esa5.microchip.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751286AbdLEV0b (ORCPT ); Tue, 5 Dec 2017 16:26:31 -0500 X-IronPort-AV: E=Sophos;i="5.45,365,1508828400"; d="scan'208";a="7135268" Subject: Re: [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions To: Julia Cartwright , Julia Lawall CC: Rafal Ozieblo , , , References: <20171205201711.GA18445@jcartwri.amer.corp.natinst.com> From: Nicolas Ferre Organization: microchip Message-ID: <64576603-3d10-b749-2c66-ec0d70cd9ba3@microchip.com> Date: Tue, 5 Dec 2017 22:26:18 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20171205201711.GA18445@jcartwri.amer.corp.natinst.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4328 Lines: 129 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 > Cc: Julia Lawall > Signed-off-by: Julia Cartwright > --- > 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 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