Re: [PATCH 09/11] dmaengine: add support for device_link

[Frank Li <Frank.li@nxp.com>]

On Tue, Sep 09, 2025 at 02:03:09PM +0200, Marco Felsch wrote:
> Hi Frank,
>
> On 25-09-03, Frank Li wrote:
> > On Wed, Sep 03, 2025 at 03:06:17PM +0200, Marco Felsch wrote:
> > > Add support to create device_links between dmaengine suppliers and the
> > > dma consumers. This shifts the device dep-chain teardown/bringup logic
> > > to the driver core.
> > >
> > > Moving this to the core allows the dmaengine drivers to simplify the
> > > .remove() hooks and also to ensure that no dmaengine driver is ever
> > > removed before the consumer is removed.
> > >
> > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > ---
> >
> > Thank you work for devlink between dmaengine and devices. I have similar
> > idea.
> >
> > This patch should be first patch.
>
> I can shuffle it of course!
>
> > The below what planned commit message in my local tree.
>
> Okay, so you focused on runtime PM handling. Not quite sure if I can
> test this feature with the SDMA engine. I also have limited time for
> this feature.
>
> Is it okay for you and the DMA maintainers to add the runtime PM feature
> as separate patch (provided by NXP/Frank)?

we can support runtime pm later.

>
> > Implementing runtime PM for DMA channels is challenging. If a channel
> > resumes at allocation and suspends at free, the DMA engine often remains on
> > because most drivers request a channel at probe.
> >
> > Tracking the number of pending DMA descriptors is also problematic, as some
> > consumers append new descriptors in atomic contexts, such as IRQ handlers,
> > where runtime resume cannot be called.
> >
> > Using a device link simplifies this issue. If a consumer requires data
> > transfer, it must be in a runtime-resumed state, ensuring that the DMA
> > channel is also active by device link. This allows safe operations, like
> > appending new descriptors. Conversely, when the consumer no longer requires
> > data transfer, both it and the supplier (DMA channel) can enter a suspended
> > state if no other consumer is using it.
> >
> > Introduce the `create_link` flag to enable this feature.
> >
> > also suggest add create_link flag to enable this feature in case some
> > side impact to other dma-engine. After some time test, we can enable it
> > default.
>
> What regressions do you have in mind? I wouldn't hide the feature behind
> a flag because this may slow done the convert process, because no one is
> interessted in, or has no time for testing, ...

Unlike other devices, like phys, regulator, mailbox..., which auto create
devlink at probe. I am not clear why dma skip this one. So I think there
should be some reason behind. Maybe other people, rob or Vinod Koul know
the reason.

static const struct supplier_bindings of_supplier_bindings[] = {
        ...
	{ .parse_prop = parse_dmas, .optional = true, },

If remove "optional = true", devlink will auto create. I am not sure why
set true here.

>
> > >  drivers/dma/dmaengine.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> > > index 758fcd0546d8bde8e8dddc6039848feeb1e24475..a50652bc70b8ce9d4edabfaa781b3432ee47d31e 100644
> > > --- a/drivers/dma/dmaengine.c
> > > +++ b/drivers/dma/dmaengine.c
> > > @@ -817,6 +817,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > >  	struct fwnode_handle *fwnode = dev_fwnode(dev);
> > >  	struct dma_device *d, *_d;
> > >  	struct dma_chan *chan = NULL;
> > > +	struct device_link *dl;
> > >
> > >  	if (is_of_node(fwnode))
> > >  		chan = of_dma_request_slave_channel(to_of_node(fwnode), name);
> > > @@ -858,6 +859,13 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
> > >  	/* No functional issue if it fails, users are supposed to test before use */
> > >  #endif
> > >
> > > +	dl = device_link_add(dev, chan->device->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> >
> > chan->device->dev is dmaengine devices. But some dmaengine's each channel
> > have device, consumer should link to chan's device, not dmaengine device
> > because some dmaengine support per channel clock\power management.
>
> I get your point. Can you give me some pointers please? To me it seems
> like the dma_chan_dev is only used for sysfs purpose according the
> dmaengine.h.

Not really, there are other dma engineer already reuse it for other purpose.
So It needs update kernel doc for dma_chan_dev.

>
> > chan's device's parent devices is dmaengine devices. it should also work
> > for sdma case
>
> I see, this must be tested of course.
> > >         if (chan->device->create_devlink) {
> >                 u32 flags = DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER;
>
> According device_link.rst: using DL_FLAG_STATELESS and
> DL_FLAG_AUTOREMOVE_CONSUMER is invalid.
>
> >                 if (pm_runtime_active(dev))
> >                         flags |= DL_FLAG_RPM_ACTIVE;
>
> This is of course interessting, thanks for the hint.
>
> > When create device link (apply channel), consume may active.
>
> I have read it as: "resue the supplier and ensure that the supplier
> follows the consumer runtime state".
>
> >                 dl = device_link_add(chan->slave, &chan->dev->device, flags);
>
> Huh.. you used the dmaengine device too?

/**
 * struct dma_chan_dev - relate sysfs device node to backing channel device
 * @chan: driver channel device
 * @device: sysfs device
 * @dev_id: parent dma_device dev_id
 * @chan_dma_dev: The channel is using custom/different dma-mapping
 * compared to the parent dma_device
 */
struct dma_chan_dev {
	struct dma_chan *chan;
	struct device device;
	int dev_id;
	bool chan_dma_dev;
};

struct dma_chan {
	struct dma_device *device; /// this one should be dmaengine
	struct dma_chan_dev *dev; /// this one is pre-chan device.
}

Frank
>
> Regards,
>   Marco
>
>
> >         }
> >
> > Need update kernel doc
> >
> > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> > index bb146c5ac3e4c..ffb3a8f0070ba 100644
> > --- a/include/linux/dmaengine.h
> > +++ b/include/linux/dmaengine.h
> > @@ -323,7 +323,8 @@ struct dma_router {
> >   * @cookie: last cookie value returned to client
> >   * @completed_cookie: last completed cookie for this channel
> >   * @chan_id: channel ID for sysfs
> > - * @dev: class device for sysfs
> > + * @dev: class device for sysfs, also use for pre channel runtime pm and
> > + *       use custom/different dma-mapping
> >
> > Frank
> >
> >
> > > +	if (!dl) {
> > > +		dev_err(dev, "failed to create device link to %s\n",
> > > +			dev_name(chan->device->dev));
> > > +		return ERR_PTR(-EINVAL);
> > > +	}
> > >  	chan->name = kasprintf(GFP_KERNEL, "dma:%s", name);
> > >  	if (!chan->name)
> > >  		return chan;
> > >
> > > --
> > > 2.47.2
> > >
> >