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 <raf...@cadence.com>
> Cc: Julia Lawall <julia.law...@lip6.fr>
> Signed-off-by: Julia Cartwright <ju...@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.fe...@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
