Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220419211658.11403-1-apais@linux.microsoft.com>
 <20220419211658.11403-2-apais@linux.microsoft.com> <CACRpkdZ2DFZRPHS1x0=M3_8zYvU-jpCG5Tm3863dXv51EhY+BA@mail.gmail.com>
 <CAK8P3a0j_rziihsgHnG5bHMxmPbOkAhT6_+CCE4iFZy7HzQrLw@mail.gmail.com> <YpCGePbo9B/Z7slV@matsya>
In-Reply-To: <YpCGePbo9B/Z7slV@matsya>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 27 May 2022 12:59:14 +0200
Message-ID: <CAK8P3a2wD7=hgvqyS14X5p-eP+7Ajk4dFJOXgbOo8Z0r5UNYmg@mail.gmail.com>
Subject: Re: [RFC 1/1] drivers/dma/*: replace tasklets with workqueue
To:     Vinod Koul <vkoul@kernel.org>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Linus Walleij <linus.walleij@linaro.org>,
        Allen Pais <apais@linux.microsoft.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        olivier.dautricourt@orolia.com, Stefan Roese <sr@denx.de>,
        Kees Cook <keescook@chromium.org>,
        linux-hardening@vger.kernel.org,
        Ludovic Desroches <ludovic.desroches@microchip.com>,
        Tudor Ambarus <tudor.ambarus@microchip.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Ray Jui <rjui@broadcom.com>,
        Scott Branden <sbranden@broadcom.com>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>,
        Nicolas Saenz Julienne <nsaenz@kernel.org>,
        Paul Cercueil <paul@crapouillou.net>,
        Eugeniy.Paltsev@synopsys.com,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        Viresh Kumar <vireshk@kernel.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Leo Li <leoyang.li@nxp.com>, zw@zh-kernel.org,
        Zhou Wang <wangzhou1@hisilicon.com>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Sean Wang <sean.wang@mediatek.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        =?UTF-8?Q?Andreas_F=C3=A4rber?= <afaerber@suse.de>,
        Manivannan Sadhasivam <mani@kernel.org>,
        Logan Gunthorpe <logang@deltatee.com>,
        Sanjay R Mehta <sanju.mehta@amd.com>,
        Daniel Mack <daniel@zonque.org>,
        Haojian Zhuang <haojian.zhuang@gmail.com>,
        Robert Jarzmik <robert.jarzmik@free.fr>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        green.wan@sifive.com, Orson Zhai <orsonzhai@gmail.com>,
        Baolin Wang <baolin.wang7@gmail.com>,
        Lyra Zhang <zhang.lyra@gmail.com>,
        Patrice CHOTARD <patrice.chotard@foss.st.com>,
        Chen-Yu Tsai <wens@csie.org>,
        =?UTF-8?Q?Jernej_=C5=A0krabec?= <jernej.skrabec@gmail.com>,
        Samuel Holland <samuel@sholland.org>,
        dmaengine@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, May 27, 2022 at 10:06 AM Vinod Koul <vkoul@kernel.org> wrote:
> On 25-05-22, 13:03, Arnd Bergmann wrote:
> > What might work better in the case of the dmaengine API would
> > be an approach like:
> >
> > 1. add helper functions to call the callback functions from a
> >     tasklet locally defined in drivers/dma/dmaengine.c to allow
> >     deferring it from hardirq context
> >
> > 2. Change all  tasklets that are not part of the callback
> >     mechanism to work queue functions, I only see
> >     xilinx_dpdma_chan_err_task in the patch, but there
> >     may be more
> >
> > 3. change all drivers to move their custom tasklets back into
> >     hardirq context and instead call the new helper for deferring
> >     the callback.
> >
> > 4. Extend the dmaengine callback API to let slave drivers
> >     pick hardirq, tasklet or task context for the callback.
> >     task context can mean either a workqueue, or a threaded
> >     IRQ here, with the default remaining the tasklet version.
>
> That does sound a good idea, but I dont know who will use the workqueue
> or a threaded context here, it might be that most would default to
> hardirq or tasklet context for obvious reasons...

If the idea is to remove tasklets from the kernel for good, then the
choice is only between workqueue and hardirq at this point. The
workqueue version is the one that would make sense for any driver
that just defers execution from the callback down into task context.
If that gets called in task context already, the driver can be simpler.

I took a brief look at the roughly 150 slave drivers, and it does
seem like very few of them actually want task context:

* Over Half the drivers just do a complete(), which could
  probably be pulled into the dmaengine layer and done from
  hardirq, avoiding the callback entirely

* A lot of the remaining drivers have interrupts disabled for
  the entire callback, which means they might as well use
  hardirqs, regardless of what they want

* drivers/crypto/* and drivers/mmc/* tend to call another tasklet
  to do the real work.

* drivers/ata/sata_dwc_460ex.c and drivers/ntb/ntb_transport.c
   probably want task context

* Some drivers like sound/soc/sh/siu_pcm.c start a new DMA
  from the callback. Is that allowed from hardirq?

If we do the first three steps above, and then add a 'struct
completion' pointer to dma_async_tx_descriptor as an alternative
to the callback, that would already reduce the number of drivers
that end up in a tasklet significantly and should be completely
safe.

Unfortunately we can't just move the rest into hardirq
context because that breaks anything using spin_lock_bh
to protect against concurrent execution of the tasklet.

A possible alternative might be to then replace the global
dmaengine tasklet with a custom softirq. Obviously those
are not so hot either,  but dmaengine could be considered
special enough to fit in the same category as net_rx/tx
and block with their global softirqs.

       Arnd