| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aIpmgpXME1BmThxU@lizhi-Precision-Tower-5810>
Date: Wed, 30 Jul 2025 14:37:54 -0400
From: Frank Li <Frank.li@....com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Konrad Dybcio <konradybcio@...nel.org>, Vinod Koul <vkoul@...nel.org>,
Sven Peter <sven@...nel.org>, Janne Grunau <j@...nau.net>,
Alyssa Rosenzweig <alyssa@...enzweig.io>,
Neal Gompa <neal@...pa.dev>,
Ludovic Desroches <ludovic.desroches@...rochip.com>,
Florian Fainelli <florian.fainelli@...adcom.com>,
Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
Ray Jui <rjui@...adcom.com>, Scott Branden <sbranden@...adcom.com>,
Paul Cercueil <paul@...pouillou.net>,
Eugeniy Paltsev <Eugeniy.Paltsev@...opsys.com>,
Viresh Kumar <vireshk@...nel.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Shawn Guo <shawnguo@...nel.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>,
Taichi Sugaya <sugaya.taichi@...ionext.com>,
Takao Orito <orito.takao@...ionext.com>,
Andreas Färber <afaerber@...e.de>,
Manivannan Sadhasivam <mani@...nel.org>,
Daniel Mack <daniel@...que.org>,
Haojian Zhuang <haojian.zhuang@...il.com>,
Robert Jarzmik <robert.jarzmik@...e.fr>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Magnus Damm <magnus.damm@...il.com>,
Patrice Chotard <patrice.chotard@...s.st.com>,
Linus Walleij <linus.walleij@...aro.org>,
Amélie Delaunay <amelie.delaunay@...s.st.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Chen-Yu Tsai <wens@...e.org>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Samuel Holland <samuel@...lland.org>,
Laxman Dewangan <ldewangan@...dia.com>,
Jon Hunter <jonathanh@...dia.com>,
Thierry Reding <thierry.reding@...il.com>,
Peter Ujfalusi <peter.ujfalusi@...il.com>,
Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>,
Masami Hiramatsu <mhiramat@...nel.org>,
Michal Simek <michal.simek@....com>, Rob Herring <robh@...nel.org>,
Saravana Kannan <saravanak@...gle.com>,
Martin Povišer <povik+lin@...ebit.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>,
Viken Dadhaniya <quic_vdadhani@...cinc.com>,
Andi Shyti <andi.shyti@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Marijn Suijten <marijn.suijten@...ainline.org>,
dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
asahi@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
linux-rpi-kernel@...ts.infradead.org, linux-mips@...r.kernel.org,
imx@...ts.linux.dev, linux-actions@...ts.infradead.org,
linux-arm-msm@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com,
linux-sunxi@...ts.linux.dev, linux-tegra@...r.kernel.org,
devicetree@...r.kernel.org, linux-sound@...r.kernel.org,
linux-i2c@...r.kernel.org, linux-spi@...r.kernel.org,
Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Subject: Re: [PATCH RFC 2/6] dmaengine: Make of_dma_request_slave_channel
pass a cookie to of_xlate
On Wed, Jul 30, 2025 at 09:04:17PM +0300, Laurent Pinchart wrote:
> On Wed, Jul 30, 2025 at 12:39:43PM -0400, Frank Li wrote:
> > On Wed, Jul 30, 2025 at 11:33:29AM +0200, Konrad Dybcio wrote:
> > > From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
> > >
> > > The DMA subsystem attempts to make it theoretically possible to pair
> > > any DMA block with any user. While that's convenient from a
> > > codebase sanity perspective, some blocks are more intertwined.
> > >
> > > One such case is the Qualcomm GENI, where each wrapper contains a
> > > number of Serial Engine instances, each one of which can be programmed
> > > to support a different protocol (such as I2C, I3C, SPI, UART, etc.).
> > >
> > > The GPI DMA it's designed together with, needs to receive the ID of the
> > > protocol that's in use, to adjust its behavior accordingly. Currently,
> > > that's done through passing that ID through device tree, with each
> > > Serial Engine expressed NUM_PROTOCOL times, resulting in terrible
> > > dt-bindings that are full of useless copypasta.
> > >
> > > In a step to cut down on that, let the DMA user give the engine driver
> > > a hint at request time.
> > >
> > > Signed-off-by: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
> > > ---
> > > drivers/dma/amba-pl08x.c | 3 ++-
> > > drivers/dma/apple-admac.c | 3 ++-
> > > drivers/dma/at_hdmac.c | 6 ++++--
> > > drivers/dma/at_xdmac.c | 3 ++-
> > > drivers/dma/bcm2835-dma.c | 3 ++-
> > > drivers/dma/dma-jz4780.c | 3 ++-
> > > drivers/dma/dmaengine.c | 20 +++++++++++++++++---
> > > drivers/dma/dw-axi-dmac/dw-axi-dmac-platform.c | 3 ++-
> > > drivers/dma/dw/of.c | 3 ++-
> > > drivers/dma/ep93xx_dma.c | 6 ++++--
> > > drivers/dma/fsl-edma-main.c | 6 ++++--
> > > drivers/dma/img-mdc-dma.c | 3 ++-
> > > drivers/dma/imx-dma.c | 3 ++-
> > > drivers/dma/imx-sdma.c | 3 ++-
> > > drivers/dma/lgm/lgm-dma.c | 3 ++-
> > > drivers/dma/milbeaut-hdmac.c | 4 +++-
> > > drivers/dma/mmp_pdma.c | 3 ++-
> > > drivers/dma/mmp_tdma.c | 3 ++-
> > > drivers/dma/moxart-dma.c | 3 ++-
> > > drivers/dma/mxs-dma.c | 3 ++-
> > > drivers/dma/nbpfaxi.c | 3 ++-
> > > drivers/dma/of-dma.c | 18 +++++++++++-------
> > > drivers/dma/owl-dma.c | 3 ++-
> > > drivers/dma/pl330.c | 3 ++-
> > > drivers/dma/pxa_dma.c | 3 ++-
> > > drivers/dma/qcom/bam_dma.c | 3 ++-
> > > drivers/dma/qcom/gpi.c | 3 ++-
> > > drivers/dma/qcom/qcom_adm.c | 3 ++-
> > > drivers/dma/sh/rcar-dmac.c | 3 ++-
> > > drivers/dma/sh/rz-dmac.c | 3 ++-
> > > drivers/dma/sh/usb-dmac.c | 3 ++-
> > > drivers/dma/st_fdma.c | 3 ++-
> > > drivers/dma/ste_dma40.c | 3 ++-
> > > drivers/dma/stm32/stm32-dma.c | 3 ++-
> > > drivers/dma/stm32/stm32-dma3.c | 4 +++-
> > > drivers/dma/stm32/stm32-mdma.c | 3 ++-
> > > drivers/dma/sun4i-dma.c | 3 ++-
> > > drivers/dma/sun6i-dma.c | 3 ++-
> > > drivers/dma/tegra186-gpc-dma.c | 3 ++-
> > > drivers/dma/tegra20-apb-dma.c | 3 ++-
> > > drivers/dma/tegra210-adma.c | 3 ++-
> > > drivers/dma/ti/cppi41.c | 3 ++-
> > > drivers/dma/ti/edma.c | 3 ++-
> > > drivers/dma/ti/k3-udma.c | 3 ++-
> > > drivers/dma/uniphier-xdmac.c | 3 ++-
> > > drivers/dma/xilinx/xilinx_dma.c | 3 ++-
> > > drivers/dma/xilinx/xilinx_dpdma.c | 3 ++-
> > > drivers/dma/xilinx/zynqmp_dma.c | 3 ++-
> > > include/linux/dmaengine.h | 7 +++++++
> > > include/linux/of_dma.h | 16 +++++++++-------
> > > sound/soc/apple/mca.c | 2 +-
> > > sound/soc/renesas/rcar/dma.c | 2 +-
> > > 52 files changed, 146 insertions(+), 68 deletions(-)
> > >
> > > diff --git a/drivers/dma/amba-pl08x.c b/drivers/dma/amba-pl08x.c
> >
> > ...
> >
> > > const char *name)
> > > {
> > > diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h
> > > index fd706cdf255c61c82ce30ef9a2c44930bef34bc8..9f9bc4207b85d48d73c25aad4b362e7c84c01756 100644
> > > --- a/include/linux/of_dma.h
> > > +++ b/include/linux/of_dma.h
> > > @@ -19,7 +19,7 @@ struct of_dma {
> > > struct list_head of_dma_controllers;
> > > struct device_node *of_node;
> > > struct dma_chan *(*of_dma_xlate)
> > > - (struct of_phandle_args *, struct of_dma *);
> > > + (struct of_phandle_args *, struct of_dma *, void *);
> >
> > I suggest pass down more informaiton, like client's dev point. So we can
> > auto create device link between client's dev and dma chan's device.
>
> Is .of_dma_xlate() really the right place to do that ? If you want to
> create a device link for PM reasons, isn't it better created when the
> channel is requested ? It should also be removed when the channel is
> freed.
I remember just need record client device pointer here.
>
> >
> > DMA Engineer device
> > DMA chan device
> > consumer clients' device.
> >
> > If consumer device runtime pm suspend can auto trigger DMA chan's device's
> > runtime pm function.
> >
> > It will simplifly DMA engine's run time pm manage. Currently many DMA run
> > time pm implement as, runtime_pm_get() when alloc and runtime_pm_put() at
> > free channel. But many devices request dma channel at probe, which make
> > dma engine work at always 'on' state.
> >
> > But ideally, dma chan should be resume only when it is used to transfer.
>
> This is exactly what I was going to mention after reading the last
> paragraph. Is there anything that prevents a DMA engine driver to
> perform a rutime PM get() when a transfer is submitted
DMA description is a queue, It is hard to track each descriptor submit and
finished. espcially cycle buffer case.
And according to dma engine API defination, submit a descriptor not
neccessary to turn on clock, maybe just pure software operation, such as
enqueue it to a software list.
Many driver call dmaengine_submit() in irq context, submit new descriptor
when previous descriptor finished. runtime_pm_get() can NOT be called in
atomic context.
And some driver submit many descripor advance. Only issue_transfer() is
actually trigger hardware to start transfer.
Some client use cycle descripor, such audio devices. Some audio devices
have not free descriptor at their run time suspend function, just disable
audio devices's clocks. Audio devices run time suspend, which means no
one use this dma channel, dma channel can auto suspend if built device link
between audio device and dma chan devices.
Some DMA client have not devices, such as memory to memory. for this kind
case, it need keep chan always on.
issue_transfer() can be call in atomic context. but trigger hardware transfer
need clock and runtime_pm_get() can't be called in atomic context.
Most case issue_transfer() is call in irq handle, which means device should
already be in runtime resume statue. DMA engine can safely access their
register if using device link.
Frank
> and a put() when
> it completes ? (Logically speaking, the actual implementation would
> likely be a bit different in drivers, but the result would be similar.)
>
> > > void *(*of_dma_route_allocate)
> > > (struct of_phandle_args *, struct of_dma *);
> > > struct dma_router *dma_router;
> > > @@ -34,7 +34,7 @@ struct of_dma_filter_info {
> > > #ifdef CONFIG_DMA_OF
> > > extern int of_dma_controller_register(struct device_node *np,
> > > struct dma_chan *(*of_dma_xlate)
> > > - (struct of_phandle_args *, struct of_dma *),
> > > + (struct of_phandle_args *, struct of_dma *, void *),
> > > void *data);
> > > extern void of_dma_controller_free(struct device_node *np);
> > >
> > > @@ -45,16 +45,17 @@ extern int of_dma_router_register(struct device_node *np,
> > > #define of_dma_router_free of_dma_controller_free
> > >
> > > extern struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
> > > - const char *name);
> > > + const char *name,
> > > + void *data);
> > > extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec,
> > > - struct of_dma *ofdma);
> > > + struct of_dma *ofdma, void *data);
> > > extern struct dma_chan *of_dma_xlate_by_chan_id(struct of_phandle_args *dma_spec,
> > > - struct of_dma *ofdma);
> > > + struct of_dma *ofdma, void *data);
> > >
> > > #else
> > > static inline int of_dma_controller_register(struct device_node *np,
> > > struct dma_chan *(*of_dma_xlate)
> > > - (struct of_phandle_args *, struct of_dma *),
> > > + (struct of_phandle_args *, struct of_dma *, void *),
> > > void *data)
> > > {
> > > return -ENODEV;
> > > @@ -75,7 +76,8 @@ static inline int of_dma_router_register(struct device_node *np,
> > > #define of_dma_router_free of_dma_controller_free
> > >
> > > static inline struct dma_chan *of_dma_request_slave_channel(struct device_node *np,
> > > - const char *name)
> > > + const char *name,
> > > + void *data)
> > > {
> > > return ERR_PTR(-ENODEV);
> > > }
> > > diff --git a/sound/soc/apple/mca.c b/sound/soc/apple/mca.c
> > > index 5dd24ab90d0f052bb48f451cf009dc2e9128014d..43d48e4ac8161ee9955120fe64f7b911bfdfe1ca 100644
> > > --- a/sound/soc/apple/mca.c
> > > +++ b/sound/soc/apple/mca.c
> > > @@ -926,7 +926,7 @@ static struct dma_chan *mca_request_dma_channel(struct mca_cluster *cl, unsigned
> > > char *name = devm_kasprintf(cl->host->dev, GFP_KERNEL,
> > > is_tx ? "tx%da" : "rx%db", cl->no);
> > > #endif
> > > - return of_dma_request_slave_channel(cl->host->dev->of_node, name);
> > > + return of_dma_request_slave_channel(cl->host->dev->of_node, name, NULL);
> > >
> > > }
> > >
> > > diff --git a/sound/soc/renesas/rcar/dma.c b/sound/soc/renesas/rcar/dma.c
> > > index 2035ce06fe4c4aeaa8620d817910a5319732fa58..dcbff2fc61a0472adea226371016a128563b3cd0 100644
> > > --- a/sound/soc/renesas/rcar/dma.c
> > > +++ b/sound/soc/renesas/rcar/dma.c
> > > @@ -204,7 +204,7 @@ struct dma_chan *rsnd_dma_request_channel(struct device_node *of_node, char *nam
> > > }
> > >
> > > if (i == rsnd_mod_id_raw(mod) && (!chan))
> > > - chan = of_dma_request_slave_channel(np, x);
> > > + chan = of_dma_request_slave_channel(np, x, NULL);
> > > i++;
> > > }
> > >
>
> --
> Regards,
>
> Laurent Pinchart
Powered by blists - more mailing lists