Re: [PATCH v6 4/6] dmaengine: dw-edma: Add delegated channel request helpers

On Wed, Aug 12, 2026 at 01:11:41PM +0900, Koichiro Den wrote:
> 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 <[email protected]>
> > > > > > > ---
> > > > > > > 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/[email protected]/
> > >   ... and pci_epf_dma_claim_channel() used it to claim the channel here:
> > >   https://lore.kernel.org/r/[email protected]/
> > >
> > > 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/[email protected]/)
> > >
> > >   (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/[email protected]/)
> > >
> > >   (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/[email protected]/)
> > >
> > >   (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/[email protected]/)
> >
> > (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.

The method looks good.

Frank

>
> >
> > 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 <[email protected]>");
> > > > > > > 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
> > > > > > >