Re: [PATCH v6 4/6] dmaengine: dw-edma: Add delegated channel request helpers
From: Koichiro Den
Date: Wed Aug 12 2026 - 00:12:36 EST
On Mon, Aug 10, 2026 at 05:52:36PM -0400, Frank Li wrote:
> On Thu, Aug 06, 2026 at 12:45:13PM +0900, Koichiro Den wrote:
> > On Wed, Aug 05, 2026 at 01:47:45PM -0500, Frank Li wrote:
> > > On Wed, Aug 05, 2026 at 11:05:21AM +0900, Koichiro Den wrote:
> > > > On Tue, Aug 04, 2026 at 11:23:47AM -0500, Frank Li wrote:
> > > > > On Tue, Aug 04, 2026 at 12:38:53PM +0900, Koichiro Den wrote:
> > > > > > Endpoint functions that expose endpoint-local DesignWare eDMA channels
> > > > > > to a remote host need to reserve exact hardware channels and hand
> > > > > > interrupt ownership to the remote side before publishing the channels.
> > > > > >
> > > > > > Add DW eDMA-specific helpers that request a write/read hardware channel
> > > > > > through DMAengine, keep the hardware-channel filter private to dw-edma,
> > > > > > and switch the selected endpoint-local channel to remote interrupt
> > > > > > routing after the channel has been successfully reserved. The matching
> > > > > > release helper can quiesce the channel while it is still remote-routed,
> > > > > > then restores the channel's default routing before releasing the
> > > > > > DMAengine reservation. This lets callers skip quiesce when unwinding a
> > > > > > reservation that was never exposed to host programming.
> > > > > >
> > > > > > Release is best-effort because its callers cannot abort teardown. Report
> > > > > > a quiesce failure locally, but always restore the default routing and
> > > > > > release the DMAengine reservation.
> > > > > >
> > > > > > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > > > > > ---
> > > > > > Changes in v6:
> > > > > > - No changes.
> > > > > >
> > > > > > drivers/dma/dw-edma/dw-edma-core.c | 91 ++++++++++++++++++++++++++++++
> > > > > > include/linux/dma/edma.h | 14 +++++
> > > > > > 2 files changed, 105 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > > > > > index 1f893dc54c79..4c0d2bba755b 100644
> > > > > > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > > > > > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > > > > > @@ -1302,6 +1302,97 @@ int dw_edma_remove(struct dw_edma_chip *chip)
> > > > > > }
> > > > > > EXPORT_SYMBOL_GPL(dw_edma_remove);
> > > > > >
> > > > > > +struct dw_edma_delegated_chan_filter_args {
> > > > > > + struct device *dma_dev;
> > > > > > + bool write;
> > > > > > + u16 id;
> > > > > > +};
> > > > > > +
> > > > > > +static bool dw_edma_delegated_chan_filter(struct dma_chan *dchan, void *param)
> > > > > > +{
> > > > > > + struct dw_edma_delegated_chan_filter_args *filter = param;
> > > > > > + struct dw_edma_chan *chan;
> > > > > > +
> > > > > > + if (!filter || dchan->device->dev != filter->dma_dev)
> > > > > > + return false;
> > > > > > +
> > > > > > + chan = dchan2dw_edma_chan(dchan);
> > > > > > +
> > > > > > + return chan->dir == (filter->write ? EDMA_DIR_WRITE : EDMA_DIR_READ) &&
> > > > > > + chan->id == filter->id;
> > > > > > +}
> > > > > > +
> > > > > > +static int dw_edma_delegate_chan(struct dma_chan *dchan)
> > > > > > +{
> > > > > > + struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> > > > > > + int ret = 0;
> > > > > > +
> > > > > > + if (!(chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL))
> > > > > > + return -EINVAL;
> > > > > > +
> > > > > > + guard(spinlock_irqsave)(&chan->vc.lock);
> > > > > > +
> > > > > > + if (chan->configured || chan->status != EDMA_ST_IDLE ||
> > > > > > + chan->request != EDMA_REQ_NONE)
> > > > > > + ret = -EBUSY;
> > > > > > + else
> > > > > > + chan->irq_mode = DW_EDMA_CH_IRQ_REMOTE;
> > > > > > +
> > > > > > + return ret;
> > > > > > +}
> > > > > > +
> > > > > > +struct dma_chan *dw_edma_request_delegated_chan(struct device *dma_dev,
> > > > > > + bool write, u16 id)
> > > > > > +{
> > > > > > + struct dw_edma_delegated_chan_filter_args filter = {
> > > > > > + .dma_dev = dma_dev,
> > > > > > + .write = write,
> > > > > > + .id = id,
> > > > > > + };
> > > > > > + struct dma_chan *dchan;
> > > > > > + dma_cap_mask_t mask;
> > > > > > +
> > > > > > + if (!dma_dev)
> > > > > > + return NULL;
> > > > > > +
> > > > > > + dma_cap_zero(mask);
> > > > > > + dma_cap_set(DMA_SLAVE, mask);
> > > > > > +
> > > > > > + dchan = dma_request_channel(mask, dw_edma_delegated_chan_filter,
> > > > > > + &filter);
> > > > > > + if (!dchan)
> > > > > > + return NULL;
> > > > >
> > > > > I forget, why need export delegrated dma channel to dmanegine? If EP
> > > > > driver doesn't report delegrated channel to dma engine, what's happen?
> > > >
> > > > This just reserves the exact channel against any dmaengine clients before
> > > > handing its programming ownership to the host. Without it, a local client
> > > > could request the same channel too.
> > >
> > > why not epf function driver call dma_request_channel() to decide which
> > > channel pass to ownership to the host.
> >
> > I actually tried that model in v2. pci_epf_dma called dma_request_channel() with
> > a filter supplied through the EPC resource.
> >
> > In v2, .filter_fn was part of the pci_epc_aux_resource definition:
> > https://lore.kernel.org/r/20260525063129.3316894-3-den@xxxxxxxxxxxxx/
> > ... and pci_epf_dma_claim_channel() used it to claim the channel here:
> > https://lore.kernel.org/r/20260525063456.3317509-3-den@xxxxxxxxxxxxx/
> >
> > I changed that in v3 after your comment about why the dw-edma filter needed to
> > be public. The replacement kept the filter inside dw-edma, and moved channel
> > reservation, IRQ ownership handoff and reclaim there too. The current EPC
> > delegation API follows that model.
> >
> > Or is that v2 model what you prefer here?
> >
> > That said, dma_get_slave_channel() might be sufficient inside dw-edma here,
> > since it already knows the exact channel from the direction and hw channel id.
> >
> > P.S. For clarity, the options tried or discussed so far are:
> >
> > (a) Expose dma_chan directly through the EPC resource, then let the EPF
> > reserve it with dma_get_slave_channel(). This was v1.
> > (https://lore.kernel.org/r/20260521063405.2842644-3-den@xxxxxxxxxxxxx/)
> >
> > (b) Expose the hardware ID, direction and a provider filter, then let the
> > EPF call dma_request_channel(). This was v2.
> > (https://lore.kernel.org/r/20260525062420.3315904-2-den@xxxxxxxxxxxxx/)
> >
> > (c) Pass the hardware ID and direction to pci_epc_delegate_dma_chan(), then
> > let the backend reserve and delegate the channel. This is the current
> > v6 design.
> > (https://lore.kernel.org/r/20260804033855.2115817-5-den@xxxxxxxxxxxxx/)
> >
> > (d) Request channels normally and correlate them with EPC resources through
> > a generic dma_slave_caps.hw_id. I proposed this in a much older series.
> > (https://lore.kernel.org/r/20260204145440.950609-2-den@xxxxxxxxxxxxx/)
>
> (d) is close to best solutin.
Hi Frank, thanks for sharing your thoughts.
>
> There are chan_id in dma_chan, but have not implemented good enough.
>
> /*
> * When the chan_id is a negative value, we are dynamically adding
> * the channel. Otherwise we are static enumerating.
> */
> chan->chan_id = ida_alloc(&device->chan_ida, GFP_KERNEL);
> if (chan->chan_id < 0) {
> pr_err("%s: unable to alloc ida for chan: %d\n",
> __func__, chan->chan_id);
> rc = chan->chan_id;
> goto err_free_dev;
> }
>
> But there are not check for negative value eventhough there are comments
> for it.
I looked into the history. Isn't this just a stale comment left behind by
0821009445a8 ("dmaengine: fix channel index enumeration")?
>
> It causes logic channel and hardware channel mismatch if dma-channel mask
> existting. If skip some channel register, it will be mismatched, which will
> cause confuse when debug.
>
> To keep back compatible, we may use reverse logic for dynamically adding.
Just to confirm, by "reverse logic", do you mean keeping the automatic IDA
allocation from 0821009445a8 as the default, while letting a driver explicitly
set a static channel ID? (pls see my next comment below)
>
> BIT(30) as static enumerating.
>
> if (!(chan->chan_id & BIT(30)))
> chan->chan_id = ida_alloc(&device->chan_ida, GFP_KERNEL);
>
>
> Or we simple add hw_id in dma_chan. at debug fs can show hw id. but idealy
> chan_id match hardware sequency.
I agree. It would be nice if we could do without adding a new field. How about a
small helper such as:
static inline void
dmaengine_set_static_chan_id(struct dma_chan *chan, unsigned int hw_id)
{
chan->chan_id = BIT(30) | hw_id;
}
then in the dmaengine core:
if (chan->chan_id & BIT(30)) {
id = chan->chan_id & ~BIT(30);
chan->chan_id = ida_alloc_range(&device->chan_ida, id, id,
GFP_KERNEL);
} else {
chan->chan_id = ida_alloc(&device->chan_ida, GFP_KERNEL);
}
and for dw-edma as an example, use it like this:
diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
index 1f893dc54c79..d214df55da3c 100644
--- a/drivers/dma/dw-edma/dw-edma-core.c
+++ b/drivers/dma/dw-edma/dw-edma-core.c
@@ -987,6 +987,7 @@ static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
&dw->chip->dt_region_rd[chan->id];
vchan_init(&chan->vc, dma);
+ dmaengine_set_static_chan_id(&chan->vc.chan, i);
dw_edma_core_ch_config(chan);
}
Here, 'i' is a direction-flattened ID, unlike dw_edma_chan.id, which is
direction-local. It keeps dma_chan.chan_id unique within one dma_device.
>
> chan_id match hardware instance will beanfit more. There are some code
> assume chan_id is that hardware id, such as
>
> drivers/dma/dw/idma32.c: value = readl(misc + DMA_CTL_CH(dwc->chan.chan_id));
Thanks for the pointer. I wasn't aware of it, TBH. The current semantics might
be a bit fragile.
Thanks again for the review, it's super helpful.
Best regards,
Koichiro
>
> Frank
>
> >
> > Best regards,
> > Koichiro
> >
> > >
> > > Frank
> > >
> > > >
> > > > Best regards,
> > > > Koichiro
> > > >
> > > > >
> > > > > Frank
> > > > >
> > > > > > +
> > > > > > + if (dw_edma_delegate_chan(dchan)) {
> > > > > > + dma_release_channel(dchan);
> > > > > > + return NULL;
> > > > > > + }
> > > > > > +
> > > > > > + return dchan;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(dw_edma_request_delegated_chan);
> > > > > > +
> > > > > > +void dw_edma_release_delegated_chan(struct dma_chan *dchan, bool quiesce)
> > > > > > +{
> > > > > > + struct dw_edma_chan *chan;
> > > > > > +
> > > > > > + if (!dchan)
> > > > > > + return;
> > > > > > +
> > > > > > + chan = dchan2dw_edma_chan(dchan);
> > > > > > + if (quiesce && dw_edma_core_ch_quiesce(chan))
> > > > > > + dev_warn(chan->dw->chip->dev,
> > > > > > + "failed to quiesce delegated %s channel %u\n",
> > > > > > + chan->dir == EDMA_DIR_WRITE ? "write" : "read",
> > > > > > + chan->id);
> > > > > > +
> > > > > > + scoped_guard(spinlock_irqsave, &chan->vc.lock)
> > > > > > + chan->irq_mode = dw_edma_get_default_irq_mode(chan);
> > > > > > +
> > > > > > + dma_release_channel(dchan);
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(dw_edma_release_delegated_chan);
> > > > > > +
> > > > > > MODULE_LICENSE("GPL v2");
> > > > > > MODULE_DESCRIPTION("Synopsys DesignWare eDMA controller core driver");
> > > > > > MODULE_AUTHOR("Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx>");
> > > > > > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> > > > > > index 3c8e2ef9dee0..944469258b8b 100644
> > > > > > --- a/include/linux/dma/edma.h
> > > > > > +++ b/include/linux/dma/edma.h
> > > > > > @@ -153,6 +153,9 @@ struct dw_edma_chip {
> > > > > > #if IS_REACHABLE(CONFIG_DW_EDMA)
> > > > > > int dw_edma_probe(struct dw_edma_chip *chip);
> > > > > > int dw_edma_remove(struct dw_edma_chip *chip);
> > > > > > +struct dma_chan *dw_edma_request_delegated_chan(struct device *dma_dev,
> > > > > > + bool write, u16 id);
> > > > > > +void dw_edma_release_delegated_chan(struct dma_chan *chan, bool quiesce);
> > > > > > #else
> > > > > > static inline int dw_edma_probe(struct dw_edma_chip *chip)
> > > > > > {
> > > > > > @@ -163,6 +166,17 @@ static inline int dw_edma_remove(struct dw_edma_chip *chip)
> > > > > > {
> > > > > > return 0;
> > > > > > }
> > > > > > +
> > > > > > +static inline struct dma_chan *
> > > > > > +dw_edma_request_delegated_chan(struct device *dma_dev, bool write, u16 id)
> > > > > > +{
> > > > > > + return NULL;
> > > > > > +}
> > > > > > +
> > > > > > +static inline void dw_edma_release_delegated_chan(struct dma_chan *chan,
> > > > > > + bool quiesce)
> > > > > > +{
> > > > > > +}
> > > > > > #endif /* CONFIG_DW_EDMA */
> > > > > >
> > > > > > #endif /* _DW_EDMA_H */
> > > > > > --
> > > > > > 2.51.0
> > > > > >