Re: [PATCH 08/16] NTB: ntb_transport: Add opt-in direct-DMA channel reservation

[Dave Jiang <dave.jiang@intel.com>]

On 8/21/26 7:38 AM, Koichiro Den wrote:
> On Thu, Aug 20, 2026 at 01:03:45PM -0700, Dave Jiang wrote:
>>
>>
>> On 8/10/26 9:51 AM, Koichiro Den wrote:
>>> Direct DMA is opt-in. Add 'use_direct_dma' module parameter to request
>>> it. Some endpoint integrations expose NTB and DMA through sibling PCI
>>> functions, so add direct_dma_func to select the DMA function when the
>>> NTB device cannot return the DMA device directly.
>>>
>>> When use_direct_dma is set and negotiation scratchpads are available,
>>> locate the DMA device and reserve one suitable channel for every QP
>>> before registering clients. This avoids negotiating direct TX before a
>>> later-created QP discovers that no channel is available. If the full set
>>> is unavailable, leave direct TX disabled. Note that direct RX does not
>>> require these channels.
>>>
>>> Signed-off-by: Koichiro Den <den@valinux.co.jp>
>>> ---
>>>  drivers/ntb/ntb_transport.c | 120 ++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 120 insertions(+)
>>>
>>> diff --git a/drivers/ntb/ntb_transport.c b/drivers/ntb/ntb_transport.c
>>> index f873eacd532b..6051d8abfc0d 100644
>>> --- a/drivers/ntb/ntb_transport.c
>>> +++ b/drivers/ntb/ntb_transport.c
>>> @@ -98,6 +98,16 @@ static bool use_dma;
>>>  module_param(use_dma, bool, 0644);
>>>  MODULE_PARM_DESC(use_dma, "Use DMA engine to perform large data copy");
>>>  
>>> +static bool use_direct_dma;
>>> +module_param(use_direct_dma, bool, 0644);
>>> +MODULE_PARM_DESC(use_direct_dma,
>>> +		 "Use PCI endpoint DMA to transfer directly to peer RX buffers");
>>> +
>>> +static unsigned int direct_dma_func;
>>> +module_param(direct_dma_func, uint, 0644);
>>> +MODULE_PARM_DESC(direct_dma_func,
>>> +		 "PCI function number of a sibling endpoint DMA function");
>>
>> This feel like very adhoc way of determining which DMA device to use. Is there a better way to do some sort of auto discovery without required user input?
> 
> Hi, thanks for the review.
> 
> Yes, I agree. The PCI function layout is still being discussed here:
> https://lore.kernel.org/r/fg7cma2ccvdgjgcref7qa4q7ocvvume5dteekedikszebkq7rd@uhrjfp3nmmwh/
> Depending on the outcome, we may not need this parameter at all.
> 
> If EPC-local DMA is exposed as part of the vNTB function, its association is
> explicit and no function-number parameter is needed. That was my original
> approach [1], which you also reviewed as part of my gigantic RFC (thanks
> again!).
> 
> If we keep the separate PCI function used by this v1, its PCI class can narrow
> down the candidates, but we still need a defined association when a slot has
> more than one DMA-class function.
> 
> In any case, I'll align v2 with the outcome of that discussion.
> 
> [1]
> https://lore.kernel.org/r/20260312165005.1148676-1-den@valinux.co.jp/
> https://lore.kernel.org/r/sn67hi7kljh7cgmgodatb3naz2astlaklqfobdbxyyzgoohxqb@4nnetbhqwba4/
> and later tried the separate PCI DMA function here:
> https://lore.kernel.org/r/20260813063757.3131865-1-den@valinux.co.jp/
> 
>>
>>
>>> +
>>>  static unsigned int direct_dma_ring_entries = 64;
>>>  module_param(direct_dma_ring_entries, uint, 0644);
>>>  MODULE_PARM_DESC(direct_dma_ring_entries,
>>> @@ -147,6 +157,7 @@ struct ntb_transport_qp {
>>>  	void *cb_data;
>>>  	struct dma_chan *tx_dma_chan;
>>>  	struct dma_chan *rx_dma_chan;
>>> +	struct dma_chan *direct_dma_chan;
>>>  
>>>  	bool client_ready;
>>>  	bool link_is_up;
>>> @@ -269,6 +280,7 @@ struct ntb_transport_ctx {
>>>  	struct work_struct link_cleanup;
>>>  
>>>  	struct dentry *debugfs_node_dir;
>>> +	struct device *direct_dma_dev;
>>>  	u32 direct_features;
>>>  	u32 peer_direct_features;
>>>  	unsigned int direct_ring_entries;
>>> @@ -352,6 +364,97 @@ static inline u32 ntb_direct_ring_used(u32 head, u32 tail)
>>>  	return head - tail;
>>>  }
>>>  
>>> +static bool ntb_direct_dma_filter_fn(struct dma_chan *chan, void *data)
>>> +{
>>> +	struct dma_slave_caps caps;
>>> +
>>> +	if (chan->device->dev != data || dma_get_slave_caps(chan, &caps))
>>> +		return false;
>>> +
>>> +	/* Payload and completion descriptors must complete in order. */
>>> +	return caps.cmd_terminate &&
>>> +	       !dma_has_cap(DMA_COMPLETION_NO_ORDER, chan->device->cap_mask) &&
>>> +	       (caps.directions & BIT(DMA_MEM_TO_DEV));
>>> +}
>>> +
>>> +static struct device *ntb_direct_get_dma_dev(struct ntb_dev *ndev)
>>> +{
>>> +	struct pci_dev *pdev = ndev->pdev;
>>> +	struct pci_dev *dma_pdev;
>>> +	struct device *dev;
>>> +
>>> +	if (ndev->ops->get_dma_dev) {
>>> +		dev = ntb_get_dma_dev(ndev);
>>> +		return dev ? get_device(dev) : ERR_PTR(-ENODEV);
>>> +	}
>>> +
>>> +	if (!pdev || !pdev->bus || direct_dma_func > 7)
>>> +		return ERR_PTR(-ENODEV);
>>> +
>>> +	dma_pdev = pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
>>> +					       pdev->bus->number,
>>> +					       PCI_DEVFN(PCI_SLOT(pdev->devfn),
>>> +							 direct_dma_func));
>>> +	if (!dma_pdev)
>>> +		return ERR_PTR(-ENODEV);
>>> +
>>> +	return &dma_pdev->dev;
>>> +}
>>> +
>>> +static void ntb_direct_dma_release_channels(struct ntb_transport_ctx *nt)
>>> +{
>>> +	unsigned int i;
>>> +
>>> +	for (i = 0; i < nt->qp_count; i++) {
>>> +		if (!nt->qp_vec[i].direct_dma_chan)
>>> +			continue;
>>> +
>>> +		dma_release_channel(nt->qp_vec[i].direct_dma_chan);
>>> +		nt->qp_vec[i].direct_dma_chan = NULL;
>>> +	}
>>> +}
>>> +
>>> +static void ntb_direct_dma_release(struct ntb_transport_ctx *nt)
>>> +{
>>> +	ntb_direct_dma_release_channels(nt);
>>> +	if (nt->direct_dma_dev) {
>>> +		put_device(nt->direct_dma_dev);
>>> +		nt->direct_dma_dev = NULL;
>>> +	}
>>> +}
>>> +
>>> +static void ntb_direct_dma_init(struct ntb_transport_ctx *nt)
>>> +{
>>> +	dma_cap_mask_t mask;
>>> +	unsigned int i;
>>> +
>>> +	if (!use_direct_dma)
>>> +		return;
>>> +
>>> +	nt->direct_dma_dev = ntb_direct_get_dma_dev(nt->ndev);
>>> +	if (IS_ERR(nt->direct_dma_dev)) {
>>> +		dev_info(&nt->ndev->dev, "direct DMA device unavailable: %pe\n",
>>> +			 nt->direct_dma_dev);
>>> +		nt->direct_dma_dev = NULL;
>>> +		return;
>>> +	}
>>> +
>>> +	dma_cap_zero(mask);
>>> +	dma_cap_set(DMA_SLAVE, mask);
>>
>> Are only slave DMA usable in this case? Is it not possible to do DMA with host DMA engines?
>>
>> My main concern in all of this is looking for a way to make this usable for all ntb_transport users if possible and not niche the generic code to a specific hardware vendor.
> 
> DMA_SLAVE is only the initial capability mask. ntb_direct_dma_filter_fn() then
> filters further.
> 
> A host-local DMA controller may accelerate copies into a fixed NTB MW, as the
> existing MEMCPY path does. But AFAICT it cannot generally write directly to
> arbitrary endpoint RX buffers. Exposing each buffer per transfer through a
> BAR-backed inbound mapping would put mapping reprogramming on the hot path and
> defeat the throughput improvement purpose. Direct mode therefore needs EPC-local
> DMA, but is not tied to a particular hardware vendor.

Ok understood. I think this point needs to go into kernel documentation somewhere if it's not already.

DJ
 > 
> Best regards,
> Koichiro
> 
>>
>> DJ
>>> +
>>> +	for (i = 0; i < nt->qp_count; i++) {
>>> +		nt->qp_vec[i].direct_dma_chan =
>>> +			dma_request_channel(mask, ntb_direct_dma_filter_fn,
>>> +					    nt->direct_dma_dev);
>>> +		if (!nt->qp_vec[i].direct_dma_chan) {
>>> +			dev_info(&nt->ndev->dev,
>>> +				 "not enough direct DMA channels for all QPs\n");
>>> +			ntb_direct_dma_release_channels(nt);
>>> +			return;
>>> +		}
>>> +	}
>>> +}
>>> +
>>>  struct ntb_payload_header {
>>>  	unsigned int ver;
>>>  	unsigned int len;
>>> @@ -1476,6 +1579,16 @@ static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev)
>>>  		return -ENOMEM;
>>>  
>>>  	nt->ndev = ndev;
>>> +	if (use_direct_dma &&
>>> +	    (direct_dma_ring_entries < 2 ||
>>> +	     direct_dma_ring_entries > NTB_DIRECT_MAX_RING_ENTRIES ||
>>> +	     !is_power_of_2(direct_dma_ring_entries))) {
>>> +		dev_err(&ndev->dev,
>>> +			"direct DMA ring entries must be a power of two between 2 and %u\n",
>>> +			NTB_DIRECT_MAX_RING_ENTRIES);
>>> +		rc = -EINVAL;
>>> +		goto err;
>>> +	}
>>>  	nt->direct_ring_entries = direct_dma_ring_entries;
>>>  
>>>  	/*
>>> @@ -1582,6 +1695,11 @@ static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev)
>>>  		if (rc)
>>>  			goto err2;
>>>  	}
>>> +	if (ntb_direct_spads_available(nt))
>>> +		ntb_direct_dma_init(nt);
>>> +	else if (use_direct_dma)
>>> +		dev_info(&ndev->dev,
>>> +			 "not enough scratchpads for direct DMA negotiation\n");
>>>  
>>>  	mutex_init(&nt->link_event_lock);
>>>  	INIT_DELAYED_WORK(&nt->link_work, ntb_transport_link_work);
>>> @@ -1605,6 +1723,7 @@ static int ntb_transport_probe(struct ntb_client *self, struct ntb_dev *ndev)
>>>  err3:
>>>  	ntb_clear_ctx(ndev);
>>>  err2:
>>> +	ntb_direct_dma_release(nt);
>>>  	kfree(nt->qp_vec);
>>>  err1:
>>>  	while (i--) {
>>> @@ -1648,6 +1767,7 @@ static void ntb_transport_free(struct ntb_client *self, struct ntb_dev *ndev)
>>>  		iounmap(nt->mw_vec[i].vbase);
>>>  	}
>>>  
>>> +	ntb_direct_dma_release(nt);
>>>  	kfree(nt->qp_vec);
>>>  	kfree(nt->mw_vec);
>>>  	kfree(nt);
>>