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
