Re: [PATCH 1/2] net: macb: reduce scope of rx_fs_lock-protected regions

[Nicolas Ferre <nicolas.ferre@microchip.com>]

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