dmaengine Archive on lore.kernel.org
 help / color / Atom feed
From: Peter Ujfalusi <peter.ujfalusi@ti.com>
To: Sascha Hauer <s.hauer@pengutronix.de>, <dmaengine@vger.kernel.org>
Cc: Robert Jarzmik <robert.jarzmik@free.fr>,
	Vinod Koul <vkoul@kernel.org>, <kernel@pengutronix.de>,
	Russell King <linux@armlinux.org.uk>
Subject: Re: vchan helper broken wrt locking
Date: Wed, 4 Dec 2019 13:47:03 +0200
Message-ID: <12ec3499-ac9a-2722-2052-02d77975c26c@ti.com> (raw)
In-Reply-To: <20191203115050.yvpaehsrck6zydmk@pengutronix.de>

Hi Sascha,

On 03/12/2019 13.50, Sascha Hauer wrote:
> Hi All,
> 
> vc->desc_free() used to be called in non atomic context which makes
> sense to me. This changed over time and now vc->desc_free() is sometimes
> called in atomic context and sometimes not.
> 
> The story starts with 13bb26ae8850 ("dmaengine: virt-dma: don't always
> free descriptor upon completion"). This introduced a vc->desc_allocated
> list which is mostly handled with the lock held, except in vchan_complete().
> vchan_complete() moves the completed descs onto a separate list for the sake
> of iterating over that list without the lock held allowing to call
> vc->desc_free() without lock. 13bb26ae8850 changes this to:
> 
> @@ -83,8 +110,10 @@ static void vchan_complete(unsigned long arg)
>                 cb_data = vd->tx.callback_param;
>  
>                 list_del(&vd->node);
> -
> -               vc->desc_free(vd);
> +               if (dmaengine_desc_test_reuse(&vd->tx))
> +                       list_add(&vd->node, &vc->desc_allocated);
> +               else
> +                       vc->desc_free(vd);
> 
> vc->desc_free() is still called without lock, but the list operation is done
> without locking as well which is wrong.

Hrm, yes all list operation against desc_* should be protected by the
lock, it is a miss.

> Now with 6af149d2b142 ("dmaengine: virt-dma: Add helper to free/reuse a
> descriptor") the hunk above was moved to a separate function
> (vchan_vdesc_fini()). With 1c7f072d94e8 ("dmaengine: virt-dma: Support for
> race free transfer termination") the helper is started to be called with
> lock held resulting in vc->desc_free() being called under the lock as
> well. It is still called from vchan_complete() without lock.

Right.
I think the most elegant way to fix this would be to introduce a new
list_head in virt_dma_chan, let's name it desc_terminated.

We would add the descriptor to this within vchan_terminate_vdesc() (lock
is held).
In vchan_synchronize() we would
list_splice_tail_init(&vc->desc_terminated, &head);
with the lock held and outside of the lock we free them up.

So we would put the terminated descs to the new list and free them up in
synchronize.

This way the vchan_vdesc_fini() would be only called without the lock held.

> I think vc->desc_free() being called under a spin_lock is unfortunate as
> the i.MX SDMA driver does a dma_free_coherent() there which is required
> to be called with interrupts enabled.

In the in review k3-udma driver I use dma_pool or dma_alloc_coherent in
mixed mode depending on the type of the channel.

I did also see the same issue and what I ended up doing is to have
desc_to_purge list and udma_purge_desc_work()
in udma_desc_free() if the descriptor is from the dma_pool, I free it
right away, if it needs dma_free_coherent() then I put it to the
desc_to_purge list and schedule the purge worker to deal with them at a
later time.

In this driver I don't use vchan_terminate_vdesc() because of this.

> I am not sure where to go from here hence I'm writing this mail. Do we
> agree that vc->desc_free() should be called without lock?

I think it should be called without the lock held.

> 
> Sascha
> 
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

  parent reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-03 11:50 Sascha Hauer
2019-12-04  8:45 ` Robert Jarzmik
2019-12-04 11:47 ` Peter Ujfalusi [this message]
2019-12-04 14:01   ` Vinod Koul
2019-12-04 15:51   ` Sascha Hauer

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=12ec3499-ac9a-2722-2052-02d77975c26c@ti.com \
    --to=peter.ujfalusi@ti.com \
    --cc=dmaengine@vger.kernel.org \
    --cc=kernel@pengutronix.de \
    --cc=linux@armlinux.org.uk \
    --cc=robert.jarzmik@free.fr \
    --cc=s.hauer@pengutronix.de \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

dmaengine Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dmaengine/0 dmaengine/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dmaengine dmaengine/ https://lore.kernel.org/dmaengine \
		dmaengine@vger.kernel.org
	public-inbox-index dmaengine

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.dmaengine


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git