Re: [PATCH] dmaengine: cppi41: Fix cppi41_dma_prep_slave_sg() when idle

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

On 10/23/19 8:16 PM, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [191023 17:04]:
>> On 10/23/19 6:31 PM, Tony Lindgren wrote:
>>> diff --git a/drivers/dma/ti/cppi41.c b/drivers/dma/ti/cppi41.c
>>> --- a/drivers/dma/ti/cppi41.c
>>> +++ b/drivers/dma/ti/cppi41.c
>>> @@ -586,9 +586,22 @@ static struct dma_async_tx_descriptor *cppi41_dma_prep_slave_sg(
>>>  	enum dma_transfer_direction dir, unsigned long tx_flags, void *context)
>>>  {
>>>  	struct cppi41_channel *c = to_cpp41_chan(chan);
>>> +	struct dma_async_tx_descriptor *txd = NULL;
>>> +	struct cppi41_dd *cdd = c->cdd;
>>>  	struct cppi41_desc *d;
>>>  	struct scatterlist *sg;
>>>  	unsigned int i;
>>> +	int error;
>>> +
>>> +	error = pm_runtime_get(cdd->ddev.dev);
>>
>> If pm_runtime_get()
>> pm_runtime_mark_last_busy()+pm_runtime_put_autosuspend() around a code
>> which updates a descriptor in _memory_ helps then this best described as
>> works by luck ;)
> 
> It also checks the cpp41 state for cdd->is_suspended
> though.

Which is cleared/set in the suspend/resume callbacks and they are called
from a work (the driver uses async runtime_get).

> AFAIK we do not currently have any other place
> to tell the driver a DMA request is about to start at
> some point soon.

True, but still.

>> I have a feeling that if you put enough delay between prepare_sg and
>> issue_pending in the usb driver then it will keep failing, no?
> 
> Nope, it will just queue it and run the queue when awake.

the autosuspend_delay is set 100 ms, so if you put a udelay(101) between
prep_sg and issue_pending in the usb driver this trickery will be for
nothing, right?
If the usb driver is preempted for longer than 100ms between the two
calls, same issue.
Not sure, but if for some reason the transfer would take longer than
100ms than pm_runtime will bring down the dma, no?

>> fwiw, in the cppi41_dma_issue_pending() the driver does:
>>
>> 	error = pm_runtime_get(cdd->ddev.dev);
>> ...
>> 	if (!cdd->is_suspended)
>> 		cppi41_run_queue(cdd);
>> ...
>> 	pm_runtime_mark_last_busy(cdd->ddev.dev);
>> 	pm_runtime_put_autosuspend(cdd->ddev.dev);
>>
>> Without waiting for the transfer to complete?
> 
> The queue gets run when cpp41 is awake, runtime PM
> reference is not released until completed.
> 
>> If issue_pending is not starting the transfer right away then the whole
>> pm handling is broken in there. imho.
> 
> AFAIK there is no other way to do this without tagging
> devices with pm_runtime_irq_safe(), which is nasty as
> it takes a permanent use count on the parent device.
> 
> But yeah, some dmaengine API that can sleep to tell
> a request is about to come would simplify things.

any of the prep callbacks kind of indicates that a client is preparing a
transfer so in a perfect world it is going to want to execute it..

> I don't think we have anything like that available
> right now?

Well, it would have the same issues. If the time between
dmaengine_be_warned_i_m_going_to_call_issue_pending_soon and
issue_pending is more than the autosuspend_delay then it is not going to
help.

On the other hand: if the usb driver assumes that the dma transfer is
already finished when issue_pending returned and carry on with
subsequent request, that is also a problematic assumption. One can only
consider a transfer to be done if the completion callback is called or
you have polled for the completion and it tells you the same.
This is problematic if you are in atomic context as the DMA completion
interrupt might not come while you are there.

imho, this fix is working by lucky constellation of the stars ;)
Or we can assume that there will never be more than 100ms delay between
prepare_sg and issue_pending...

- Peter

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