Re: [PATCH iwl-next v2 7/8] ixgbe: use GFP_KERNEL in ixgbe_fcoe_ddp_setup()

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
ixgbe: use GFP_KERNEL in ixgbe_fcoe_ddp_setup()

This commit restructures ixgbe_fcoe_ddp_setup() to use GFP_KERNEL
instead of GFP_ATOMIC by splitting get_cpu()/put_cpu() usage into two
narrow critical sections, moving DMA mapping and pool allocation between
them where preemption is enabled.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> index 011fda9c6193..e338ff0e6522 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_fcoe.c

[ ... ]

> @@ -179,29 +180,43 @@ static int ixgbe_fcoe_ddp_setup(struct net_device *netdev, u16 xid,
>  		return 0;
>  	}
>
> +	/* Pin to current CPU only to read the per-CPU pool pointer; drop
> +	 * the pin before any allocations that may sleep under direct reclaim.
> +	 */
>  	ddp_pool = per_cpu_ptr(fcoe->ddp_pool, get_cpu());
>  	if (!ddp_pool->pool) {
>  		e_warn(drv, "xid=0x%x no ddp pool for fcoe\n", xid);
> -		goto out_noddp;
> +		put_cpu();
> +		return 0;
>  	}
> +	pool = ddp_pool->pool;
> +	put_cpu();
>
>  	/* setup dma from scsi command sgl */
>  	dmacount = dma_map_sg(&adapter->pdev->dev, sgl, sgc, DMA_FROM_DEVICE);
>  	if (dmacount == 0) {
>  		e_err(drv, "xid 0x%x DMA map error\n", xid);
> -		goto out_noddp;
> +		return 0;
>  	}
>
> -	/* alloc the udl from per cpu ddp pool */
> -	ddp->udl = dma_pool_alloc(ddp_pool->pool, GFP_ATOMIC, &ddp->udp);
> +	/* Allocate from per-CPU pool; GFP_KERNEL is safe: preemption is
> +	 * re-enabled after the put_cpu() above.  Per-CPU DMA pools are only
> +	 * destroyed under RTNL during interface teardown, so the saved pool
> +	 * pointer remains valid.
> +	 */
> +	ddp->udl = dma_pool_alloc(pool, GFP_KERNEL, &ddp->udp);

Is this safe given the calling context?

Looking at the call path from the FCoE stack:

fc_exch_seq_send()
  fc_exch_alloc()
    fc_exch_em_alloc()
      spin_lock_bh(&pool->lock)
      # allocate exchange
      # set ep->ex_lock = held
      return with ep->ex_lock still held
  fc_fcp_ddp_setup()
    lport->tt.ddp_setup()
      .ndo_fcoe_ddp_setup() == ixgbe_fcoe_ddp_get()
        ixgbe_fcoe_ddp_setup()  # <-- we are here
  # ... continues with ep->ex_lock held
  spin_unlock_bh(&ep->ex_lock)

fc_exch_em_alloc() acquires ep->ex_lock (a spinlock) and returns with
it held. The lock remains held through fc_fcp_ddp_setup() and into
ixgbe_fcoe_ddp_setup(). Additionally, spin_lock_bh() disables bottom
halves.

Can GFP_KERNEL be used while holding ep->ex_lock with BH disabled, or
does this require GFP_ATOMIC?