[PATCH v4 00/14] NVIDIA Tegra APB DMA driver fixes and improvements

[PATCH v4 00/14] NVIDIA Tegra APB DMA driver fixes and improvements

[Dmitry Osipenko]

Hello,

This series fixes some problems that I spotted recently, secondly the
driver's code gets a cleanup. Please review and apply, thanks in advance!

Changelog:

v4: - Addressed Jon's request to *not* remove the runtime PM usage, instead
      there is now new patch that makes RPM more practical:

        dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

    - Added new minor patch to clean up RPM's teardown:

        dmaengine: tegra-apb: Clean up runtime PM teardown

v3: - In the review comment to v1 Michał Mirosław suggested that "Prevent
      race conditions on channel's freeing" does changes that deserve to
      be separated into two patches. I factored out and improved tasklet
      releasing into this new patch:

        dmaengine: tegra-apb: Clean up tasklet releasing

    - The "Fix use-after-free" patch got an improved commit message.

v2: - I took another look at the driver and spotted few more things that
      could be improved, which resulted in these new patches:

        dmaengine: tegra-apb: Remove runtime PM usage
        dmaengine: tegra-apb: Clean up suspend-resume
        dmaengine: tegra-apb: Add missing of_dma_controller_free
        dmaengine: tegra-apb: Allow to compile as a loadable kernel module
        dmaengine: tegra-apb: Remove MODULE_ALIAS

Dmitry Osipenko (14):
  dmaengine: tegra-apb: Fix use-after-free
  dmaengine: tegra-apb: Implement synchronization callback
  dmaengine: tegra-apb: Prevent race conditions on channel's freeing
  dmaengine: tegra-apb: Clean up tasklet releasing
  dmaengine: tegra-apb: Prevent race conditions of tasklet vs free list
  dmaengine: tegra-apb: Use devm_platform_ioremap_resource
  dmaengine: tegra-apb: Use devm_request_irq
  dmaengine: tegra-apb: Fix coding style problems
  dmaengine: tegra-apb: Clean up runtime PM teardown
  dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer
  dmaengine: tegra-apb: Clean up suspend-resume
  dmaengine: tegra-apb: Add missing of_dma_controller_free
  dmaengine: tegra-apb: Allow to compile as a loadable kernel module
  dmaengine: tegra-apb: Remove MODULE_ALIAS

 drivers/dma/Kconfig           |   2 +-
 drivers/dma/tegra20-apb-dma.c | 509 +++++++++++++++++-----------------
 2 files changed, 263 insertions(+), 248 deletions(-)

-- 
2.24.0

[PATCH v4 01/14] dmaengine: tegra-apb: Fix use-after-free

[Dmitry Osipenko]

I was doing some experiments with I2C and noticed that Tegra APB DMA
driver crashes sometime after I2C DMA transfer termination. The crash
happens because tegra_dma_terminate_all() bails out immediately if pending
list is empty, thus it doesn't release the half-completed descriptors
which are getting re-used before ISR tasklet kicks-in.

 tegra-i2c 7000c400.i2c: DMA transfer timeout
 elants_i2c 0-0010: elants_i2c_irq: failed to read data: -110
 ------------[ cut here ]------------
 WARNING: CPU: 0 PID: 142 at lib/list_debug.c:45 __list_del_entry_valid+0x45/0xac
 list_del corruption, ddbaac44->next is LIST_POISON1 (00000100)
 Modules linked in:
 CPU: 0 PID: 142 Comm: kworker/0:2 Not tainted 5.5.0-rc2-next-20191220-00175-gc3605715758d-dirty #538
 Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
 Workqueue: events_freezable_power_ thermal_zone_device_check
 [<c010e5c5>] (unwind_backtrace) from [<c010a1c5>] (show_stack+0x11/0x14)
 [<c010a1c5>] (show_stack) from [<c0973925>] (dump_stack+0x85/0x94)
 [<c0973925>] (dump_stack) from [<c011f529>] (__warn+0xc1/0xc4)
 [<c011f529>] (__warn) from [<c011f7e9>] (warn_slowpath_fmt+0x61/0x78)
 [<c011f7e9>] (warn_slowpath_fmt) from [<c042497d>] (__list_del_entry_valid+0x45/0xac)
 [<c042497d>] (__list_del_entry_valid) from [<c047a87f>] (tegra_dma_tasklet+0x5b/0x154)
 [<c047a87f>] (tegra_dma_tasklet) from [<c0124799>] (tasklet_action_common.constprop.0+0x41/0x7c)
 [<c0124799>] (tasklet_action_common.constprop.0) from [<c01022ab>] (__do_softirq+0xd3/0x2a8)
 [<c01022ab>] (__do_softirq) from [<c0124683>] (irq_exit+0x7b/0x98)
 [<c0124683>] (irq_exit) from [<c0168c19>] (__handle_domain_irq+0x45/0x80)
 [<c0168c19>] (__handle_domain_irq) from [<c043e429>] (gic_handle_irq+0x45/0x7c)
 [<c043e429>] (gic_handle_irq) from [<c0101aa5>] (__irq_svc+0x65/0x94)
 Exception stack(0xde2ebb90 to 0xde2ebbd8)

Cc: <stable@vger.kernel.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/dma/tegra20-apb-dma.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 3a45079d11ec..319f31d27014 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -756,10 +756,6 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	bool was_busy;
 
 	spin_lock_irqsave(&tdc->lock, flags);
-	if (list_empty(&tdc->pending_sg_req)) {
-		spin_unlock_irqrestore(&tdc->lock, flags);
-		return 0;
-	}
 
 	if (!tdc->busy)
 		goto skip_dma_stop;
-- 
2.24.0

Re: [PATCH v4 01/14] dmaengine: tegra-apb: Fix use-after-free

[Jon Hunter]

On 12/01/2020 17:29, Dmitry Osipenko wrote:
> I was doing some experiments with I2C and noticed that Tegra APB DMA
> driver crashes sometime after I2C DMA transfer termination. The crash
> happens because tegra_dma_terminate_all() bails out immediately if pending
> list is empty, thus it doesn't release the half-completed descriptors
> which are getting re-used before ISR tasklet kicks-in.

Can you elaborate a bit more on how these are getting re-used? What is
the sequence of events which results in the panic? I believe that this
was also reported in the past [0] and so I don't doubt there is an issue
here, but would like to completely understand this.

Thanks!
Jon

[0] https://lore.kernel.org/patchwork/patch/675349/

-- 
nvpublic

Re: [PATCH v4 01/14] dmaengine: tegra-apb: Fix use-after-free

[Dmitry Osipenko]

14.01.2020 18:09, Jon Hunter пишет:
> 
> On 12/01/2020 17:29, Dmitry Osipenko wrote:
>> I was doing some experiments with I2C and noticed that Tegra APB DMA
>> driver crashes sometime after I2C DMA transfer termination. The crash
>> happens because tegra_dma_terminate_all() bails out immediately if pending
>> list is empty, thus it doesn't release the half-completed descriptors
>> which are getting re-used before ISR tasklet kicks-in.
> 
> Can you elaborate a bit more on how these are getting re-used? What is
> the sequence of events which results in the panic? I believe that this
> was also reported in the past [0] and so I don't doubt there is an issue
> here, but would like to completely understand this.
> 
> Thanks!
> Jon
> 
> [0] https://lore.kernel.org/patchwork/patch/675349/
> 

In my case it happens in the touchscreen driver during of the
touchscreen's interrupt handling (in a threaded IRQ handler) + CPU is
under load and there is other interrupts activity. So what happens here
is that the TS driver issues one I2C transfer, which fails with
(apparently bogus) timeout (because DMA descriptor is completed and
removed from the pending list, but tasklet not executed yet), and then
TS immediately issues another I2C transfer that re-uses the
yet-incompleted descriptor. That's my understanding.

Re: [PATCH v4 01/14] dmaengine: tegra-apb: Fix use-after-free

[Jon Hunter]

On 14/01/2020 20:33, Dmitry Osipenko wrote:
> 14.01.2020 18:09, Jon Hunter пишет:
>>
>> On 12/01/2020 17:29, Dmitry Osipenko wrote:
>>> I was doing some experiments with I2C and noticed that Tegra APB DMA
>>> driver crashes sometime after I2C DMA transfer termination. The crash
>>> happens because tegra_dma_terminate_all() bails out immediately if pending
>>> list is empty, thus it doesn't release the half-completed descriptors
>>> which are getting re-used before ISR tasklet kicks-in.
>>
>> Can you elaborate a bit more on how these are getting re-used? What is
>> the sequence of events which results in the panic? I believe that this
>> was also reported in the past [0] and so I don't doubt there is an issue
>> here, but would like to completely understand this.
>>
>> Thanks!
>> Jon
>>
>> [0] https://lore.kernel.org/patchwork/patch/675349/
>>
> 
> In my case it happens in the touchscreen driver during of the
> touchscreen's interrupt handling (in a threaded IRQ handler) + CPU is
> under load and there is other interrupts activity. So what happens here
> is that the TS driver issues one I2C transfer, which fails with
> (apparently bogus) timeout (because DMA descriptor is completed and
> removed from the pending list, but tasklet not executed yet), and then
> TS immediately issues another I2C transfer that re-uses the
> yet-incompleted descriptor. That's my understanding.

OK, but what is the exact sequence that it allowing it to re-use the
incompleted descriptor?

Thanks
Jon

-- 
nvpublic

Re: [PATCH v4 01/14] dmaengine: tegra-apb: Fix use-after-free

[Dmitry Osipenko]

15.01.2020 12:00, Jon Hunter пишет:
> 
> On 14/01/2020 20:33, Dmitry Osipenko wrote:
>> 14.01.2020 18:09, Jon Hunter пишет:
>>>
>>> On 12/01/2020 17:29, Dmitry Osipenko wrote:
>>>> I was doing some experiments with I2C and noticed that Tegra APB DMA
>>>> driver crashes sometime after I2C DMA transfer termination. The crash
>>>> happens because tegra_dma_terminate_all() bails out immediately if pending
>>>> list is empty, thus it doesn't release the half-completed descriptors
>>>> which are getting re-used before ISR tasklet kicks-in.
>>>
>>> Can you elaborate a bit more on how these are getting re-used? What is
>>> the sequence of events which results in the panic? I believe that this
>>> was also reported in the past [0] and so I don't doubt there is an issue
>>> here, but would like to completely understand this.
>>>
>>> Thanks!
>>> Jon
>>>
>>> [0] https://lore.kernel.org/patchwork/patch/675349/
>>>
>>
>> In my case it happens in the touchscreen driver during of the
>> touchscreen's interrupt handling (in a threaded IRQ handler) + CPU is
>> under load and there is other interrupts activity. So what happens here
>> is that the TS driver issues one I2C transfer, which fails with
>> (apparently bogus) timeout (because DMA descriptor is completed and
>> removed from the pending list, but tasklet not executed yet), and then
>> TS immediately issues another I2C transfer that re-uses the
>> yet-incompleted descriptor. That's my understanding.
> 
> OK, but what is the exact sequence that it allowing it to re-use the
> incompleted descriptor?

   TDMA driver                      DMA Client

1.
                                    dmaengine_prep()

2.
   tegra_dma_desc_get()
   dma_desc = kzalloc()
   ...
   tegra_dma_prep_slave_sg()
   INIT_LIST_HEAD(&dma_desc->tx_list);
   INIT_LIST_HEAD(&dma_desc->cb_node);
   list_add_tail(sgreq->node,
                 dma_desc->tx_list)

3.
                                    dma_async_issue_pending()

4.
   tegra_dma_tx_submit()
   list_splice_tail_init(dma_desc->tx_list,
                         tdc->pending_sg_req)

5.
   tegra_dma_isr()
   ...
   handle_once_dma_done()
   ...
   sgreq = list_first_entry(tdc->pending_sg_req)
   list_del(sgreq->node);
   ...
   list_add_tail(dma_desc->cb_node,
                 tdc->cb_desc);
   list_add_tail(dma_desc->node,
                 tdc->free_dma_desc);
   ...
   tasklet_schedule(&tdc->tasklet);
   ...

6.
                                    timeout
                                    dmaengine_terminate_async()

7.
   tegra_dma_terminate_all()
   if (list_empty(tdc->pending_sg_req))
       return 0;

8.
                                    dmaengine_prep()

9.
   tegra_dma_desc_get()
   list_for_each_entry(dma_desc,
                       tdc->free_dma_desc) {
       list_del(dma_desc->node);
       return dma_desc;
   }
   ...
   tegra_dma_prep_slave_sg()
   INIT_LIST_HEAD(&dma_desc->tx_list);
   INIT_LIST_HEAD(&dma_desc->cb_node);

   *** tdc->cb_desc list is wrecked now! ***

   list_add_tail(sgreq->node,
                 dma_desc->tx_list)
   ...

10.
   same actions as in #4 #5 ...

11.
   tegra_dma_tasklet()
   dma_desc = list_first_entry(tdc->cb_desc)
   list_del(dma_desc->cb_node);

   eventual woopsie

Re: [PATCH v4 01/14] dmaengine: tegra-apb: Fix use-after-free

[Jon Hunter]

On 16/01/2020 20:10, Dmitry Osipenko wrote:
> 15.01.2020 12:00, Jon Hunter пишет:
>>
>> On 14/01/2020 20:33, Dmitry Osipenko wrote:
>>> 14.01.2020 18:09, Jon Hunter пишет:
>>>>
>>>> On 12/01/2020 17:29, Dmitry Osipenko wrote:
>>>>> I was doing some experiments with I2C and noticed that Tegra APB DMA
>>>>> driver crashes sometime after I2C DMA transfer termination. The crash
>>>>> happens because tegra_dma_terminate_all() bails out immediately if pending
>>>>> list is empty, thus it doesn't release the half-completed descriptors
>>>>> which are getting re-used before ISR tasklet kicks-in.
>>>>
>>>> Can you elaborate a bit more on how these are getting re-used? What is
>>>> the sequence of events which results in the panic? I believe that this
>>>> was also reported in the past [0] and so I don't doubt there is an issue
>>>> here, but would like to completely understand this.
>>>>
>>>> Thanks!
>>>> Jon
>>>>
>>>> [0] https://lore.kernel.org/patchwork/patch/675349/
>>>>
>>>
>>> In my case it happens in the touchscreen driver during of the
>>> touchscreen's interrupt handling (in a threaded IRQ handler) + CPU is
>>> under load and there is other interrupts activity. So what happens here
>>> is that the TS driver issues one I2C transfer, which fails with
>>> (apparently bogus) timeout (because DMA descriptor is completed and
>>> removed from the pending list, but tasklet not executed yet), and then
>>> TS immediately issues another I2C transfer that re-uses the
>>> yet-incompleted descriptor. That's my understanding.
>>
>> OK, but what is the exact sequence that it allowing it to re-use the
>> incompleted descriptor?
> 
>    TDMA driver                      DMA Client
> 
> 1.
>                                     dmaengine_prep()
> 
> 2.
>    tegra_dma_desc_get()
>    dma_desc = kzalloc()
>    ...
>    tegra_dma_prep_slave_sg()
>    INIT_LIST_HEAD(&dma_desc->tx_list);
>    INIT_LIST_HEAD(&dma_desc->cb_node);
>    list_add_tail(sgreq->node,
>                  dma_desc->tx_list)
> 
> 3.
>                                     dma_async_issue_pending()
> 
> 4.
>    tegra_dma_tx_submit()
>    list_splice_tail_init(dma_desc->tx_list,
>                          tdc->pending_sg_req)
> 
> 5.
>    tegra_dma_isr()
>    ...
>    handle_once_dma_done()
>    ...
>    sgreq = list_first_entry(tdc->pending_sg_req)
>    list_del(sgreq->node);
>    ...
>    list_add_tail(dma_desc->cb_node,
>                  tdc->cb_desc);
>    list_add_tail(dma_desc->node,
>                  tdc->free_dma_desc);

Isn't this the problem here, that we have placed this on the free list
before we are actually done?

It seems to me that there could still be a potential race condition
between the ISR and the tasklet running.

Jon

-- 
nvpublic

Re: [PATCH v4 01/14] dmaengine: tegra-apb: Fix use-after-free

[Dmitry Osipenko]

28.01.2020 17:02, Jon Hunter пишет:
> 
> On 16/01/2020 20:10, Dmitry Osipenko wrote:
>> 15.01.2020 12:00, Jon Hunter пишет:
>>>
>>> On 14/01/2020 20:33, Dmitry Osipenko wrote:
>>>> 14.01.2020 18:09, Jon Hunter пишет:
>>>>>
>>>>> On 12/01/2020 17:29, Dmitry Osipenko wrote:
>>>>>> I was doing some experiments with I2C and noticed that Tegra APB DMA
>>>>>> driver crashes sometime after I2C DMA transfer termination. The crash
>>>>>> happens because tegra_dma_terminate_all() bails out immediately if pending
>>>>>> list is empty, thus it doesn't release the half-completed descriptors
>>>>>> which are getting re-used before ISR tasklet kicks-in.
>>>>>
>>>>> Can you elaborate a bit more on how these are getting re-used? What is
>>>>> the sequence of events which results in the panic? I believe that this
>>>>> was also reported in the past [0] and so I don't doubt there is an issue
>>>>> here, but would like to completely understand this.
>>>>>
>>>>> Thanks!
>>>>> Jon
>>>>>
>>>>> [0] https://lore.kernel.org/patchwork/patch/675349/
>>>>>
>>>>
>>>> In my case it happens in the touchscreen driver during of the
>>>> touchscreen's interrupt handling (in a threaded IRQ handler) + CPU is
>>>> under load and there is other interrupts activity. So what happens here
>>>> is that the TS driver issues one I2C transfer, which fails with
>>>> (apparently bogus) timeout (because DMA descriptor is completed and
>>>> removed from the pending list, but tasklet not executed yet), and then
>>>> TS immediately issues another I2C transfer that re-uses the
>>>> yet-incompleted descriptor. That's my understanding.
>>>
>>> OK, but what is the exact sequence that it allowing it to re-use the
>>> incompleted descriptor?
>>
>>    TDMA driver                      DMA Client
>>
>> 1.
>>                                     dmaengine_prep()
>>
>> 2.
>>    tegra_dma_desc_get()
>>    dma_desc = kzalloc()
>>    ...
>>    tegra_dma_prep_slave_sg()
>>    INIT_LIST_HEAD(&dma_desc->tx_list);
>>    INIT_LIST_HEAD(&dma_desc->cb_node);
>>    list_add_tail(sgreq->node,
>>                  dma_desc->tx_list)
>>
>> 3.
>>                                     dma_async_issue_pending()
>>
>> 4.
>>    tegra_dma_tx_submit()
>>    list_splice_tail_init(dma_desc->tx_list,
>>                          tdc->pending_sg_req)
>>
>> 5.
>>    tegra_dma_isr()
>>    ...
>>    handle_once_dma_done()
>>    ...
>>    sgreq = list_first_entry(tdc->pending_sg_req)
>>    list_del(sgreq->node);
>>    ...
>>    list_add_tail(dma_desc->cb_node,
>>                  tdc->cb_desc);
>>    list_add_tail(dma_desc->node,
>>                  tdc->free_dma_desc);
> 
> Isn't this the problem here, that we have placed this on the free list
> before we are actually done?
> 
> It seems to me that there could still be a potential race condition
> between the ISR and the tasklet running.

Yes, this should be addressed by the patch #3 "dmaengine: tegra-apb:
Prevent race conditions of tasklet vs free list".

Re: [PATCH v4 01/14] dmaengine: tegra-apb: Fix use-after-free

[Dmitry Osipenko]

28.01.2020 17:51, Dmitry Osipenko пишет:
> 28.01.2020 17:02, Jon Hunter пишет:
>>
>> On 16/01/2020 20:10, Dmitry Osipenko wrote:
>>> 15.01.2020 12:00, Jon Hunter пишет:
>>>>
>>>> On 14/01/2020 20:33, Dmitry Osipenko wrote:
>>>>> 14.01.2020 18:09, Jon Hunter пишет:
>>>>>>
>>>>>> On 12/01/2020 17:29, Dmitry Osipenko wrote:
>>>>>>> I was doing some experiments with I2C and noticed that Tegra APB DMA
>>>>>>> driver crashes sometime after I2C DMA transfer termination. The crash
>>>>>>> happens because tegra_dma_terminate_all() bails out immediately if pending
>>>>>>> list is empty, thus it doesn't release the half-completed descriptors
>>>>>>> which are getting re-used before ISR tasklet kicks-in.
>>>>>>
>>>>>> Can you elaborate a bit more on how these are getting re-used? What is
>>>>>> the sequence of events which results in the panic? I believe that this
>>>>>> was also reported in the past [0] and so I don't doubt there is an issue
>>>>>> here, but would like to completely understand this.
>>>>>>
>>>>>> Thanks!
>>>>>> Jon
>>>>>>
>>>>>> [0] https://lore.kernel.org/patchwork/patch/675349/
>>>>>>
>>>>>
>>>>> In my case it happens in the touchscreen driver during of the
>>>>> touchscreen's interrupt handling (in a threaded IRQ handler) + CPU is
>>>>> under load and there is other interrupts activity. So what happens here
>>>>> is that the TS driver issues one I2C transfer, which fails with
>>>>> (apparently bogus) timeout (because DMA descriptor is completed and
>>>>> removed from the pending list, but tasklet not executed yet), and then
>>>>> TS immediately issues another I2C transfer that re-uses the
>>>>> yet-incompleted descriptor. That's my understanding.
>>>>
>>>> OK, but what is the exact sequence that it allowing it to re-use the
>>>> incompleted descriptor?
>>>
>>>    TDMA driver                      DMA Client
>>>
>>> 1.
>>>                                     dmaengine_prep()
>>>
>>> 2.
>>>    tegra_dma_desc_get()
>>>    dma_desc = kzalloc()
>>>    ...
>>>    tegra_dma_prep_slave_sg()
>>>    INIT_LIST_HEAD(&dma_desc->tx_list);
>>>    INIT_LIST_HEAD(&dma_desc->cb_node);
>>>    list_add_tail(sgreq->node,
>>>                  dma_desc->tx_list)
>>>
>>> 3.
>>>                                     dma_async_issue_pending()
>>>
>>> 4.
>>>    tegra_dma_tx_submit()
>>>    list_splice_tail_init(dma_desc->tx_list,
>>>                          tdc->pending_sg_req)
>>>
>>> 5.
>>>    tegra_dma_isr()
>>>    ...
>>>    handle_once_dma_done()
>>>    ...
>>>    sgreq = list_first_entry(tdc->pending_sg_req)
>>>    list_del(sgreq->node);
>>>    ...
>>>    list_add_tail(dma_desc->cb_node,
>>>                  tdc->cb_desc);
>>>    list_add_tail(dma_desc->node,
>>>                  tdc->free_dma_desc);
>>
>> Isn't this the problem here, that we have placed this on the free list
>> before we are actually done?
>>
>> It seems to me that there could still be a potential race condition
>> between the ISR and the tasklet running.
> 
> Yes, this should be addressed by the patch #3 "dmaengine: tegra-apb:
> Prevent race conditions of tasklet vs free list".

correction (to avoid confusion): it's actually patch #5, my bad

Re: [PATCH v4 01/14] dmaengine: tegra-apb: Fix use-after-free

[Jon Hunter]

On 29/01/2020 00:12, Dmitry Osipenko wrote:
> 28.01.2020 17:51, Dmitry Osipenko пишет:
>> 28.01.2020 17:02, Jon Hunter пишет:
>>>
>>> On 16/01/2020 20:10, Dmitry Osipenko wrote:
>>>> 15.01.2020 12:00, Jon Hunter пишет:
>>>>>
>>>>> On 14/01/2020 20:33, Dmitry Osipenko wrote:
>>>>>> 14.01.2020 18:09, Jon Hunter пишет:
>>>>>>>
>>>>>>> On 12/01/2020 17:29, Dmitry Osipenko wrote:
>>>>>>>> I was doing some experiments with I2C and noticed that Tegra APB DMA
>>>>>>>> driver crashes sometime after I2C DMA transfer termination. The crash
>>>>>>>> happens because tegra_dma_terminate_all() bails out immediately if pending
>>>>>>>> list is empty, thus it doesn't release the half-completed descriptors
>>>>>>>> which are getting re-used before ISR tasklet kicks-in.
>>>>>>>
>>>>>>> Can you elaborate a bit more on how these are getting re-used? What is
>>>>>>> the sequence of events which results in the panic? I believe that this
>>>>>>> was also reported in the past [0] and so I don't doubt there is an issue
>>>>>>> here, but would like to completely understand this.
>>>>>>>
>>>>>>> Thanks!
>>>>>>> Jon
>>>>>>>
>>>>>>> [0] https://lore.kernel.org/patchwork/patch/675349/
>>>>>>>
>>>>>>
>>>>>> In my case it happens in the touchscreen driver during of the
>>>>>> touchscreen's interrupt handling (in a threaded IRQ handler) + CPU is
>>>>>> under load and there is other interrupts activity. So what happens here
>>>>>> is that the TS driver issues one I2C transfer, which fails with
>>>>>> (apparently bogus) timeout (because DMA descriptor is completed and
>>>>>> removed from the pending list, but tasklet not executed yet), and then
>>>>>> TS immediately issues another I2C transfer that re-uses the
>>>>>> yet-incompleted descriptor. That's my understanding.
>>>>>
>>>>> OK, but what is the exact sequence that it allowing it to re-use the
>>>>> incompleted descriptor?
>>>>
>>>>    TDMA driver                      DMA Client
>>>>
>>>> 1.
>>>>                                     dmaengine_prep()
>>>>
>>>> 2.
>>>>    tegra_dma_desc_get()
>>>>    dma_desc = kzalloc()
>>>>    ...
>>>>    tegra_dma_prep_slave_sg()
>>>>    INIT_LIST_HEAD(&dma_desc->tx_list);
>>>>    INIT_LIST_HEAD(&dma_desc->cb_node);
>>>>    list_add_tail(sgreq->node,
>>>>                  dma_desc->tx_list)
>>>>
>>>> 3.
>>>>                                     dma_async_issue_pending()
>>>>
>>>> 4.
>>>>    tegra_dma_tx_submit()
>>>>    list_splice_tail_init(dma_desc->tx_list,
>>>>                          tdc->pending_sg_req)
>>>>
>>>> 5.
>>>>    tegra_dma_isr()
>>>>    ...
>>>>    handle_once_dma_done()
>>>>    ...
>>>>    sgreq = list_first_entry(tdc->pending_sg_req)
>>>>    list_del(sgreq->node);
>>>>    ...
>>>>    list_add_tail(dma_desc->cb_node,
>>>>                  tdc->cb_desc);
>>>>    list_add_tail(dma_desc->node,
>>>>                  tdc->free_dma_desc);
>>>
>>> Isn't this the problem here, that we have placed this on the free list
>>> before we are actually done?
>>>
>>> It seems to me that there could still be a potential race condition
>>> between the ISR and the tasklet running.
>>
>> Yes, this should be addressed by the patch #3 "dmaengine: tegra-apb:
>> Prevent race conditions of tasklet vs free list".
> 
> correction (to avoid confusion): it's actually patch #5, my bad

Ah OK great! Could be worth making that patch #1 or #2 in the series as
theses are somewhat related.

Cheers
Jon

-- 
nvpublic

[PATCH v4 02/14] dmaengine: tegra-apb: Implement synchronization callback

[Dmitry Osipenko]

The ISR tasklet could be kept scheduled after DMA transfer termination,
let's add synchronization callback which blocks until tasklet is finished.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/dma/tegra20-apb-dma.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 319f31d27014..664e9c5df3ba 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -798,6 +798,13 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	return 0;
 }
 
+static void tegra_dma_synchronize(struct dma_chan *dc)
+{
+	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
+
+	tasklet_kill(&tdc->tasklet);
+}
+
 static unsigned int tegra_dma_sg_bytes_xferred(struct tegra_dma_channel *tdc,
 					       struct tegra_dma_sg_req *sg_req)
 {
@@ -1506,6 +1513,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	tdma->dma_dev.residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
 	tdma->dma_dev.device_config = tegra_dma_slave_config;
 	tdma->dma_dev.device_terminate_all = tegra_dma_terminate_all;
+	tdma->dma_dev.device_synchronize = tegra_dma_synchronize;
 	tdma->dma_dev.device_tx_status = tegra_dma_tx_status;
 	tdma->dma_dev.device_issue_pending = tegra_dma_issue_pending;
 
-- 
2.24.0

Re: [PATCH v4 02/14] dmaengine: tegra-apb: Implement synchronization callback

[Jon Hunter]

On 12/01/2020 17:29, Dmitry Osipenko wrote:
> The ISR tasklet could be kept scheduled after DMA transfer termination,
> let's add synchronization callback which blocks until tasklet is finished.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/dma/tegra20-apb-dma.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 319f31d27014..664e9c5df3ba 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -798,6 +798,13 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
>  	return 0;
>  }
>  
> +static void tegra_dma_synchronize(struct dma_chan *dc)
> +{
> +	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> +
> +	tasklet_kill(&tdc->tasklet);
> +}
> +

Wouldn't there need to be some clean-up here? If the tasklet is
scheduled, seems that there would be some other house-keeping that needs
to be done after killing it.

Jon

-- 
nvpublic

Re: [PATCH v4 02/14] dmaengine: tegra-apb: Implement synchronization callback

[Dmitry Osipenko]

14.01.2020 18:15, Jon Hunter пишет:
> 
> On 12/01/2020 17:29, Dmitry Osipenko wrote:
>> The ISR tasklet could be kept scheduled after DMA transfer termination,
>> let's add synchronization callback which blocks until tasklet is finished.
>>
>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/dma/tegra20-apb-dma.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>> index 319f31d27014..664e9c5df3ba 100644
>> --- a/drivers/dma/tegra20-apb-dma.c
>> +++ b/drivers/dma/tegra20-apb-dma.c
>> @@ -798,6 +798,13 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
>>  	return 0;
>>  }
>>  
>> +static void tegra_dma_synchronize(struct dma_chan *dc)
>> +{
>> +	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>> +
>> +	tasklet_kill(&tdc->tasklet);
>> +}
>> +
> 
> Wouldn't there need to be some clean-up here? If the tasklet is
> scheduled, seems that there would be some other house-keeping that needs
> to be done after killing it.

I'm not seeing anything to clean-up, could you please clarify?

Re: [PATCH v4 02/14] dmaengine: tegra-apb: Implement synchronization callback

[Jon Hunter]

On 14/01/2020 21:02, Dmitry Osipenko wrote:
> 14.01.2020 18:15, Jon Hunter пишет:
>>
>> On 12/01/2020 17:29, Dmitry Osipenko wrote:
>>> The ISR tasklet could be kept scheduled after DMA transfer termination,
>>> let's add synchronization callback which blocks until tasklet is finished.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>>  drivers/dma/tegra20-apb-dma.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>>> index 319f31d27014..664e9c5df3ba 100644
>>> --- a/drivers/dma/tegra20-apb-dma.c
>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>> @@ -798,6 +798,13 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
>>>  	return 0;
>>>  }
>>>  
>>> +static void tegra_dma_synchronize(struct dma_chan *dc)
>>> +{
>>> +	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>> +
>>> +	tasklet_kill(&tdc->tasklet);
>>> +}
>>> +
>>
>> Wouldn't there need to be some clean-up here? If the tasklet is
>> scheduled, seems that there would be some other house-keeping that needs
>> to be done after killing it.
> 
> I'm not seeing anything to clean-up, could you please clarify?

Clean-up with regard to the descriptors. I was concerned if you will the
tasklet the necessary clean-up of the descriptors is not handled.

Jon

-- 
nvpublic

Re: [PATCH v4 02/14] dmaengine: tegra-apb: Implement synchronization callback

[Jon Hunter]

On 15/01/2020 09:18, Jon Hunter wrote:
> 
> On 14/01/2020 21:02, Dmitry Osipenko wrote:
>> 14.01.2020 18:15, Jon Hunter пишет:
>>>
>>> On 12/01/2020 17:29, Dmitry Osipenko wrote:
>>>> The ISR tasklet could be kept scheduled after DMA transfer termination,
>>>> let's add synchronization callback which blocks until tasklet is finished.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  drivers/dma/tegra20-apb-dma.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>>>> index 319f31d27014..664e9c5df3ba 100644
>>>> --- a/drivers/dma/tegra20-apb-dma.c
>>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>>> @@ -798,6 +798,13 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static void tegra_dma_synchronize(struct dma_chan *dc)
>>>> +{
>>>> +	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>>> +
>>>> +	tasklet_kill(&tdc->tasklet);
>>>> +}
>>>> +
>>>
>>> Wouldn't there need to be some clean-up here? If the tasklet is
>>> scheduled, seems that there would be some other house-keeping that needs
>>> to be done after killing it.
>>
>> I'm not seeing anything to clean-up, could you please clarify?
> 
> Clean-up with regard to the descriptors. I was concerned if you will the
> tasklet the necessary clean-up of the descriptors is not handled.

Ah I see that tasklet_kill, unlike tasklet_kill_immediate, does wait for
the tasklet to run if scheduled. OK, then this should be fine.

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic

[PATCH v4 03/14] dmaengine: tegra-apb: Prevent race conditions on channel's freeing

[Dmitry Osipenko]

It's incorrect to check the channel's "busy" state without taking a lock.
That shouldn't cause any real troubles, nevertheless it's always better
not to have any race conditions in the code.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/dma/tegra20-apb-dma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 664e9c5df3ba..24ad3a5a04e3 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1294,8 +1294,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 
 	dev_dbg(tdc2dev(tdc), "Freeing channel %d\n", tdc->id);
 
-	if (tdc->busy)
-		tegra_dma_terminate_all(dc);
+	tegra_dma_terminate_all(dc);
 
 	spin_lock_irqsave(&tdc->lock, flags);
 	list_splice_init(&tdc->pending_sg_req, &sg_req_list);
-- 
2.24.0

Re: [PATCH v4 03/14] dmaengine: tegra-apb: Prevent race conditions on channel's freeing

[Jon Hunter]

On 12/01/2020 17:29, Dmitry Osipenko wrote:
> It's incorrect to check the channel's "busy" state without taking a lock.
> That shouldn't cause any real troubles, nevertheless it's always better
> not to have any race conditions in the code.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/dma/tegra20-apb-dma.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 664e9c5df3ba..24ad3a5a04e3 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -1294,8 +1294,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>  
>  	dev_dbg(tdc2dev(tdc), "Freeing channel %d\n", tdc->id);
>  
> -	if (tdc->busy)
> -		tegra_dma_terminate_all(dc);
> +	tegra_dma_terminate_all(dc);
>  
>  	spin_lock_irqsave(&tdc->lock, flags);
>  	list_splice_init(&tdc->pending_sg_req, &sg_req_list);

Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Cheers
Jon

-- 
nvpublic

[PATCH v4 04/14] dmaengine: tegra-apb: Clean up tasklet releasing

[Dmitry Osipenko]

There is no need to kill tasklet when driver's probe fails because tasklet
can't be scheduled at this time. It is also cleaner to kill tasklet on
channel's freeing rather than to kill it on driver's removal, otherwise
tasklet could perform a dummy execution after channel's releasing, which
isn't very nice.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/dma/tegra20-apb-dma.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 24ad3a5a04e3..1b8a11804962 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1287,7 +1287,6 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 	struct tegra_dma_sg_req *sg_req;
 	struct list_head dma_desc_list;
 	struct list_head sg_req_list;
-	unsigned long flags;
 
 	INIT_LIST_HEAD(&dma_desc_list);
 	INIT_LIST_HEAD(&sg_req_list);
@@ -1295,15 +1294,14 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 	dev_dbg(tdc2dev(tdc), "Freeing channel %d\n", tdc->id);
 
 	tegra_dma_terminate_all(dc);
+	tasklet_kill(&tdc->tasklet);
 
-	spin_lock_irqsave(&tdc->lock, flags);
 	list_splice_init(&tdc->pending_sg_req, &sg_req_list);
 	list_splice_init(&tdc->free_sg_req, &sg_req_list);
 	list_splice_init(&tdc->free_dma_desc, &dma_desc_list);
 	INIT_LIST_HEAD(&tdc->cb_desc);
 	tdc->config_init = false;
 	tdc->isr_handler = NULL;
-	spin_unlock_irqrestore(&tdc->lock, flags);
 
 	while (!list_empty(&dma_desc_list)) {
 		dma_desc = list_first_entry(&dma_desc_list,
@@ -1542,7 +1540,6 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
 
 		free_irq(tdc->irq, tdc);
-		tasklet_kill(&tdc->tasklet);
 	}
 
 	pm_runtime_disable(&pdev->dev);
@@ -1562,7 +1559,6 @@ static int tegra_dma_remove(struct platform_device *pdev)
 	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
 		tdc = &tdma->channels[i];
 		free_irq(tdc->irq, tdc);
-		tasklet_kill(&tdc->tasklet);
 	}
 
 	pm_runtime_disable(&pdev->dev);
-- 
2.24.0

Re: [PATCH v4 04/14] dmaengine: tegra-apb: Clean up tasklet releasing

[Jon Hunter]

On 12/01/2020 17:29, Dmitry Osipenko wrote:
> There is no need to kill tasklet when driver's probe fails because tasklet
> can't be scheduled at this time. It is also cleaner to kill tasklet on
> channel's freeing rather than to kill it on driver's removal, otherwise
> tasklet could perform a dummy execution after channel's releasing, which
> isn't very nice.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/dma/tegra20-apb-dma.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 24ad3a5a04e3..1b8a11804962 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -1287,7 +1287,6 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>  	struct tegra_dma_sg_req *sg_req;
>  	struct list_head dma_desc_list;
>  	struct list_head sg_req_list;
> -	unsigned long flags;
>  
>  	INIT_LIST_HEAD(&dma_desc_list);
>  	INIT_LIST_HEAD(&sg_req_list);
> @@ -1295,15 +1294,14 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>  	dev_dbg(tdc2dev(tdc), "Freeing channel %d\n", tdc->id);
>  
>  	tegra_dma_terminate_all(dc);
> +	tasklet_kill(&tdc->tasklet);
>  
> -	spin_lock_irqsave(&tdc->lock, flags);
>  	list_splice_init(&tdc->pending_sg_req, &sg_req_list);
>  	list_splice_init(&tdc->free_sg_req, &sg_req_list);
>  	list_splice_init(&tdc->free_dma_desc, &dma_desc_list);
>  	INIT_LIST_HEAD(&tdc->cb_desc);
>  	tdc->config_init = false;
>  	tdc->isr_handler = NULL;
> -	spin_unlock_irqrestore(&tdc->lock, flags);
>  
>  	while (!list_empty(&dma_desc_list)) {
>  		dma_desc = list_first_entry(&dma_desc_list,
> @@ -1542,7 +1540,6 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  		struct tegra_dma_channel *tdc = &tdma->channels[i];
>  
>  		free_irq(tdc->irq, tdc);
> -		tasklet_kill(&tdc->tasklet);
>  	}
>  
>  	pm_runtime_disable(&pdev->dev);
> @@ -1562,7 +1559,6 @@ static int tegra_dma_remove(struct platform_device *pdev)
>  	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
>  		tdc = &tdma->channels[i];
>  		free_irq(tdc->irq, tdc);
> -		tasklet_kill(&tdc->tasklet);
>  	}
>  
>  	pm_runtime_disable(&pdev->dev);

Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Cheers
Jon

-- 
nvpublic

[PATCH v4 05/14] dmaengine: tegra-apb: Prevent race conditions of tasklet vs free list

[Dmitry Osipenko]

The interrupt handler puts a half-completed DMA descriptor on a free list
and then schedules tasklet to process bottom half of the descriptor that
executes client's callback, this creates possibility to pick up the busy
descriptor from the free list. Thus let's disallow descriptor's re-use
until it is fully processed.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/dma/tegra20-apb-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 1b8a11804962..aafad50d075e 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -281,7 +281,7 @@ static struct tegra_dma_desc *tegra_dma_desc_get(
 
 	/* Do not allocate if desc are waiting for ack */
 	list_for_each_entry(dma_desc, &tdc->free_dma_desc, node) {
-		if (async_tx_test_ack(&dma_desc->txd)) {
+		if (async_tx_test_ack(&dma_desc->txd) && !dma_desc->cb_count) {
 			list_del(&dma_desc->node);
 			spin_unlock_irqrestore(&tdc->lock, flags);
 			dma_desc->txd.flags = 0;
-- 
2.24.0

Re: [PATCH v4 05/14] dmaengine: tegra-apb: Prevent race conditions of tasklet vs free list

[Jon Hunter]

On 12/01/2020 17:29, Dmitry Osipenko wrote:
> The interrupt handler puts a half-completed DMA descriptor on a free list
> and then schedules tasklet to process bottom half of the descriptor that
> executes client's callback, this creates possibility to pick up the busy
> descriptor from the free list. Thus let's disallow descriptor's re-use
> until it is fully processed.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/dma/tegra20-apb-dma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 1b8a11804962..aafad50d075e 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -281,7 +281,7 @@ static struct tegra_dma_desc *tegra_dma_desc_get(
>  
>  	/* Do not allocate if desc are waiting for ack */
>  	list_for_each_entry(dma_desc, &tdc->free_dma_desc, node) {
> -		if (async_tx_test_ack(&dma_desc->txd)) {
> +		if (async_tx_test_ack(&dma_desc->txd) && !dma_desc->cb_count) {
>  			list_del(&dma_desc->node);
>  			spin_unlock_irqrestore(&tdc->lock, flags);
>  			dma_desc->txd.flags = 0;
> 

Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Cheers
Jon

-- 
nvpublic

[PATCH v4 06/14] dmaengine: tegra-apb: Use devm_platform_ioremap_resource

[Dmitry Osipenko]

Use devm_platform_ioremap_resource to keep code cleaner a tad.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/dma/tegra20-apb-dma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index aafad50d075e..f44291207928 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1402,8 +1402,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	tdma->chip_data = cdata;
 	platform_set_drvdata(pdev, tdma);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	tdma->base_addr = devm_ioremap_resource(&pdev->dev, res);
+	tdma->base_addr = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(tdma->base_addr))
 		return PTR_ERR(tdma->base_addr);
 
-- 
2.24.0

Re: [PATCH v4 06/14] dmaengine: tegra-apb: Use devm_platform_ioremap_resource

[Jon Hunter]

On 12/01/2020 17:29, Dmitry Osipenko wrote:
> Use devm_platform_ioremap_resource to keep code cleaner a tad.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/dma/tegra20-apb-dma.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index aafad50d075e..f44291207928 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -1402,8 +1402,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  	tdma->chip_data = cdata;
>  	platform_set_drvdata(pdev, tdma);
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	tdma->base_addr = devm_ioremap_resource(&pdev->dev, res);
> +	tdma->base_addr = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(tdma->base_addr))
>  		return PTR_ERR(tdma->base_addr);
>  

Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Cheers
Jon

-- 
nvpublic

[PATCH v4 07/14] dmaengine: tegra-apb: Use devm_request_irq

[Dmitry Osipenko]

Use resource-managed variant of request_irq for brevity.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/dma/tegra20-apb-dma.c | 35 +++++++++++------------------------
 1 file changed, 11 insertions(+), 24 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index f44291207928..dff21e80ffa4 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -182,7 +182,6 @@ struct tegra_dma_channel {
 	char			name[12];
 	bool			config_init;
 	int			id;
-	int			irq;
 	void __iomem		*chan_addr;
 	spinlock_t		lock;
 	bool			busy;
@@ -1380,7 +1379,6 @@ static const struct tegra_dma_chip_data tegra148_dma_chip_data = {
 
 static int tegra_dma_probe(struct platform_device *pdev)
 {
-	struct resource *res;
 	struct tegra_dma *tdma;
 	int ret;
 	int i;
@@ -1446,25 +1444,27 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&tdma->dma_dev.channels);
 	for (i = 0; i < cdata->nr_channels; i++) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
+		int irq;
 
 		tdc->chan_addr = tdma->base_addr +
 				 TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET +
 				 (i * cdata->channel_reg_size);
 
-		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
-		if (!res) {
-			ret = -EINVAL;
+		irq = platform_get_irq(pdev, i);
+		if (irq < 0) {
+			ret = irq;
 			dev_err(&pdev->dev, "No irq resource for chan %d\n", i);
-			goto err_irq;
+			goto err_pm_disable;
 		}
-		tdc->irq = res->start;
+
 		snprintf(tdc->name, sizeof(tdc->name), "apbdma.%d", i);
-		ret = request_irq(tdc->irq, tegra_dma_isr, 0, tdc->name, tdc);
+		ret = devm_request_irq(&pdev->dev, irq, tegra_dma_isr, 0,
+				       tdc->name, tdc);
 		if (ret) {
 			dev_err(&pdev->dev,
 				"request_irq failed with err %d channel %d\n",
 				ret, i);
-			goto err_irq;
+			goto err_pm_disable;
 		}
 
 		tdc->dma_chan.device = &tdma->dma_dev;
@@ -1517,7 +1517,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	if (ret < 0) {
 		dev_err(&pdev->dev,
 			"Tegra20 APB DMA driver registration failed %d\n", ret);
-		goto err_irq;
+		goto err_pm_disable;
 	}
 
 	ret = of_dma_controller_register(pdev->dev.of_node,
@@ -1534,13 +1534,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 
 err_unregister_dma_dev:
 	dma_async_device_unregister(&tdma->dma_dev);
-err_irq:
-	while (--i >= 0) {
-		struct tegra_dma_channel *tdc = &tdma->channels[i];
-
-		free_irq(tdc->irq, tdc);
-	}
-
+err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		tegra_dma_runtime_suspend(&pdev->dev);
@@ -1550,16 +1544,9 @@ static int tegra_dma_probe(struct platform_device *pdev)
 static int tegra_dma_remove(struct platform_device *pdev)
 {
 	struct tegra_dma *tdma = platform_get_drvdata(pdev);
-	int i;
-	struct tegra_dma_channel *tdc;
 
 	dma_async_device_unregister(&tdma->dma_dev);
 
-	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
-		tdc = &tdma->channels[i];
-		free_irq(tdc->irq, tdc);
-	}
-
 	pm_runtime_disable(&pdev->dev);
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		tegra_dma_runtime_suspend(&pdev->dev);
-- 
2.24.0

Re: [PATCH v4 07/14] dmaengine: tegra-apb: Use devm_request_irq

[Jon Hunter]

On 12/01/2020 17:29, Dmitry Osipenko wrote:
> Use resource-managed variant of request_irq for brevity.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/dma/tegra20-apb-dma.c | 35 +++++++++++------------------------
>  1 file changed, 11 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index f44291207928..dff21e80ffa4 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -182,7 +182,6 @@ struct tegra_dma_channel {
>  	char			name[12];
>  	bool			config_init;
>  	int			id;
> -	int			irq;
>  	void __iomem		*chan_addr;
>  	spinlock_t		lock;
>  	bool			busy;
> @@ -1380,7 +1379,6 @@ static const struct tegra_dma_chip_data tegra148_dma_chip_data = {
>  
>  static int tegra_dma_probe(struct platform_device *pdev)
>  {
> -	struct resource *res;
>  	struct tegra_dma *tdma;
>  	int ret;
>  	int i;
> @@ -1446,25 +1444,27 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  	INIT_LIST_HEAD(&tdma->dma_dev.channels);
>  	for (i = 0; i < cdata->nr_channels; i++) {
>  		struct tegra_dma_channel *tdc = &tdma->channels[i];
> +		int irq;
>  
>  		tdc->chan_addr = tdma->base_addr +
>  				 TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET +
>  				 (i * cdata->channel_reg_size);
>  
> -		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
> -		if (!res) {
> -			ret = -EINVAL;
> +		irq = platform_get_irq(pdev, i);
> +		if (irq < 0) {
> +			ret = irq;
>  			dev_err(&pdev->dev, "No irq resource for chan %d\n", i);
> -			goto err_irq;
> +			goto err_pm_disable;
>  		}
> -		tdc->irq = res->start;
> +
>  		snprintf(tdc->name, sizeof(tdc->name), "apbdma.%d", i);
> -		ret = request_irq(tdc->irq, tegra_dma_isr, 0, tdc->name, tdc);
> +		ret = devm_request_irq(&pdev->dev, irq, tegra_dma_isr, 0,
> +				       tdc->name, tdc);
>  		if (ret) {
>  			dev_err(&pdev->dev,
>  				"request_irq failed with err %d channel %d\n",
>  				ret, i);
> -			goto err_irq;
> +			goto err_pm_disable;
>  		}
>  
>  		tdc->dma_chan.device = &tdma->dma_dev;
> @@ -1517,7 +1517,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  	if (ret < 0) {
>  		dev_err(&pdev->dev,
>  			"Tegra20 APB DMA driver registration failed %d\n", ret);
> -		goto err_irq;
> +		goto err_pm_disable;
>  	}
>  
>  	ret = of_dma_controller_register(pdev->dev.of_node,
> @@ -1534,13 +1534,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  
>  err_unregister_dma_dev:
>  	dma_async_device_unregister(&tdma->dma_dev);
> -err_irq:
> -	while (--i >= 0) {
> -		struct tegra_dma_channel *tdc = &tdma->channels[i];
> -
> -		free_irq(tdc->irq, tdc);
> -	}
> -
> +err_pm_disable:
>  	pm_runtime_disable(&pdev->dev);
>  	if (!pm_runtime_status_suspended(&pdev->dev))
>  		tegra_dma_runtime_suspend(&pdev->dev);
> @@ -1550,16 +1544,9 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  static int tegra_dma_remove(struct platform_device *pdev)
>  {
>  	struct tegra_dma *tdma = platform_get_drvdata(pdev);
> -	int i;
> -	struct tegra_dma_channel *tdc;
>  
>  	dma_async_device_unregister(&tdma->dma_dev);
>  
> -	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
> -		tdc = &tdma->channels[i];
> -		free_irq(tdc->irq, tdc);
> -	}
> -
>  	pm_runtime_disable(&pdev->dev);
>  	if (!pm_runtime_status_suspended(&pdev->dev))
>  		tegra_dma_runtime_suspend(&pdev->dev);
> 

Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Cheers
Jon

-- 
nvpublic

[PATCH v4 08/14] dmaengine: tegra-apb: Fix coding style problems

[Dmitry Osipenko]

This patch fixes few dozens of coding style problems reported by
checkpatch and prettifies code where makes sense.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/dma/tegra20-apb-dma.c | 276 ++++++++++++++++++----------------
 1 file changed, 144 insertions(+), 132 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index dff21e80ffa4..7158bd3145c4 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -59,7 +59,7 @@
 #define TEGRA_APBDMA_STATUS_COUNT_MASK		0xFFFC
 
 #define TEGRA_APBDMA_CHAN_CSRE			0x00C
-#define TEGRA_APBDMA_CHAN_CSRE_PAUSE		(1 << 31)
+#define TEGRA_APBDMA_CHAN_CSRE_PAUSE		BIT(31)
 
 /* AHB memory address */
 #define TEGRA_APBDMA_CHAN_AHBPTR		0x010
@@ -120,21 +120,21 @@ struct tegra_dma;
  * @support_separate_wcount_reg: Support separate word count register.
  */
 struct tegra_dma_chip_data {
-	int nr_channels;
-	int channel_reg_size;
-	int max_dma_count;
+	unsigned int nr_channels;
+	unsigned int channel_reg_size;
+	unsigned int max_dma_count;
 	bool support_channel_pause;
 	bool support_separate_wcount_reg;
 };
 
 /* DMA channel registers */
 struct tegra_dma_channel_regs {
-	unsigned long	csr;
-	unsigned long	ahb_ptr;
-	unsigned long	apb_ptr;
-	unsigned long	ahb_seq;
-	unsigned long	apb_seq;
-	unsigned long	wcount;
+	u32 csr;
+	u32 ahb_ptr;
+	u32 apb_ptr;
+	u32 ahb_seq;
+	u32 apb_seq;
+	u32 wcount;
 };
 
 /*
@@ -168,7 +168,7 @@ struct tegra_dma_desc {
 	struct list_head		node;
 	struct list_head		tx_list;
 	struct list_head		cb_node;
-	int				cb_count;
+	unsigned int			cb_count;
 };
 
 struct tegra_dma_channel;
@@ -181,7 +181,7 @@ struct tegra_dma_channel {
 	struct dma_chan		dma_chan;
 	char			name[12];
 	bool			config_init;
-	int			id;
+	unsigned int		id;
 	void __iomem		*chan_addr;
 	spinlock_t		lock;
 	bool			busy;
@@ -201,7 +201,7 @@ struct tegra_dma_channel {
 	/* Channel-slave specific configuration */
 	unsigned int slave_id;
 	struct dma_slave_config dma_sconfig;
-	struct tegra_dma_channel_regs	channel_reg;
+	struct tegra_dma_channel_regs channel_reg;
 };
 
 /* tegra_dma: Tegra DMA specific information */
@@ -239,7 +239,7 @@ static inline u32 tdma_read(struct tegra_dma *tdma, u32 reg)
 }
 
 static inline void tdc_write(struct tegra_dma_channel *tdc,
-		u32 reg, u32 val)
+			     u32 reg, u32 val)
 {
 	writel(val, tdc->chan_addr + reg);
 }
@@ -254,8 +254,8 @@ static inline struct tegra_dma_channel *to_tegra_dma_chan(struct dma_chan *dc)
 	return container_of(dc, struct tegra_dma_channel, dma_chan);
 }
 
-static inline struct tegra_dma_desc *txd_to_tegra_dma_desc(
-		struct dma_async_tx_descriptor *td)
+static inline struct tegra_dma_desc *
+txd_to_tegra_dma_desc(struct dma_async_tx_descriptor *td)
 {
 	return container_of(td, struct tegra_dma_desc, txd);
 }
@@ -270,8 +270,7 @@ static int tegra_dma_runtime_suspend(struct device *dev);
 static int tegra_dma_runtime_resume(struct device *dev);
 
 /* Get DMA desc from free list, if not there then allocate it.  */
-static struct tegra_dma_desc *tegra_dma_desc_get(
-		struct tegra_dma_channel *tdc)
+static struct tegra_dma_desc *tegra_dma_desc_get(struct tegra_dma_channel *tdc)
 {
 	struct tegra_dma_desc *dma_desc;
 	unsigned long flags;
@@ -298,11 +297,12 @@ static struct tegra_dma_desc *tegra_dma_desc_get(
 	dma_async_tx_descriptor_init(&dma_desc->txd, &tdc->dma_chan);
 	dma_desc->txd.tx_submit = tegra_dma_tx_submit;
 	dma_desc->txd.flags = 0;
+
 	return dma_desc;
 }
 
 static void tegra_dma_desc_put(struct tegra_dma_channel *tdc,
-		struct tegra_dma_desc *dma_desc)
+			       struct tegra_dma_desc *dma_desc)
 {
 	unsigned long flags;
 
@@ -313,29 +313,29 @@ static void tegra_dma_desc_put(struct tegra_dma_channel *tdc,
 	spin_unlock_irqrestore(&tdc->lock, flags);
 }
 
-static struct tegra_dma_sg_req *tegra_dma_sg_req_get(
-		struct tegra_dma_channel *tdc)
+static struct tegra_dma_sg_req *
+tegra_dma_sg_req_get(struct tegra_dma_channel *tdc)
 {
-	struct tegra_dma_sg_req *sg_req = NULL;
+	struct tegra_dma_sg_req *sg_req;
 	unsigned long flags;
 
 	spin_lock_irqsave(&tdc->lock, flags);
 	if (!list_empty(&tdc->free_sg_req)) {
-		sg_req = list_first_entry(&tdc->free_sg_req,
-					typeof(*sg_req), node);
+		sg_req = list_first_entry(&tdc->free_sg_req, typeof(*sg_req),
+					  node);
 		list_del(&sg_req->node);
 		spin_unlock_irqrestore(&tdc->lock, flags);
 		return sg_req;
 	}
 	spin_unlock_irqrestore(&tdc->lock, flags);
 
-	sg_req = kzalloc(sizeof(struct tegra_dma_sg_req), GFP_NOWAIT);
+	sg_req = kzalloc(sizeof(*sg_req), GFP_NOWAIT);
 
 	return sg_req;
 }
 
 static int tegra_dma_slave_config(struct dma_chan *dc,
-		struct dma_slave_config *sconfig)
+				  struct dma_slave_config *sconfig)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
 
@@ -352,11 +352,12 @@ static int tegra_dma_slave_config(struct dma_chan *dc,
 		tdc->slave_id = sconfig->slave_id;
 	}
 	tdc->config_init = true;
+
 	return 0;
 }
 
 static void tegra_dma_global_pause(struct tegra_dma_channel *tdc,
-	bool wait_for_burst_complete)
+				   bool wait_for_burst_complete)
 {
 	struct tegra_dma *tdma = tdc->tdma;
 
@@ -391,13 +392,13 @@ static void tegra_dma_global_resume(struct tegra_dma_channel *tdc)
 }
 
 static void tegra_dma_pause(struct tegra_dma_channel *tdc,
-	bool wait_for_burst_complete)
+			    bool wait_for_burst_complete)
 {
 	struct tegra_dma *tdma = tdc->tdma;
 
 	if (tdma->chip_data->support_channel_pause) {
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_CSRE,
-				TEGRA_APBDMA_CHAN_CSRE_PAUSE);
+			  TEGRA_APBDMA_CHAN_CSRE_PAUSE);
 		if (wait_for_burst_complete)
 			udelay(TEGRA_APBDMA_BURST_COMPLETE_TIME);
 	} else {
@@ -409,17 +410,15 @@ static void tegra_dma_resume(struct tegra_dma_channel *tdc)
 {
 	struct tegra_dma *tdma = tdc->tdma;
 
-	if (tdma->chip_data->support_channel_pause) {
+	if (tdma->chip_data->support_channel_pause)
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_CSRE, 0);
-	} else {
+	else
 		tegra_dma_global_resume(tdc);
-	}
 }
 
 static void tegra_dma_stop(struct tegra_dma_channel *tdc)
 {
-	u32 csr;
-	u32 status;
+	u32 csr, status;
 
 	/* Disable interrupts */
 	csr = tdc_read(tdc, TEGRA_APBDMA_CHAN_CSR);
@@ -440,7 +439,7 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc)
 }
 
 static void tegra_dma_start(struct tegra_dma_channel *tdc,
-		struct tegra_dma_sg_req *sg_req)
+			    struct tegra_dma_sg_req *sg_req)
 {
 	struct tegra_dma_channel_regs *ch_regs = &sg_req->ch_regs;
 
@@ -454,11 +453,11 @@ static void tegra_dma_start(struct tegra_dma_channel *tdc,
 
 	/* Start DMA */
 	tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR,
-				ch_regs->csr | TEGRA_APBDMA_CSR_ENB);
+		  ch_regs->csr | TEGRA_APBDMA_CSR_ENB);
 }
 
 static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc,
-		struct tegra_dma_sg_req *nsg_req)
+					 struct tegra_dma_sg_req *nsg_req)
 {
 	unsigned long status;
 
@@ -492,9 +491,9 @@ static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc,
 	tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBPTR, nsg_req->ch_regs.ahb_ptr);
 	if (tdc->tdma->chip_data->support_separate_wcount_reg)
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_WCOUNT,
-						nsg_req->ch_regs.wcount);
+			  nsg_req->ch_regs.wcount);
 	tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR,
-				nsg_req->ch_regs.csr | TEGRA_APBDMA_CSR_ENB);
+		  nsg_req->ch_regs.csr | TEGRA_APBDMA_CSR_ENB);
 	nsg_req->configured = true;
 	nsg_req->words_xferred = 0;
 
@@ -508,8 +507,7 @@ static void tdc_start_head_req(struct tegra_dma_channel *tdc)
 	if (list_empty(&tdc->pending_sg_req))
 		return;
 
-	sg_req = list_first_entry(&tdc->pending_sg_req,
-					typeof(*sg_req), node);
+	sg_req = list_first_entry(&tdc->pending_sg_req, typeof(*sg_req), node);
 	tegra_dma_start(tdc, sg_req);
 	sg_req->configured = true;
 	sg_req->words_xferred = 0;
@@ -518,34 +516,35 @@ static void tdc_start_head_req(struct tegra_dma_channel *tdc)
 
 static void tdc_configure_next_head_desc(struct tegra_dma_channel *tdc)
 {
-	struct tegra_dma_sg_req *hsgreq;
-	struct tegra_dma_sg_req *hnsgreq;
+	struct tegra_dma_sg_req *hsgreq, *hnsgreq;
 
 	if (list_empty(&tdc->pending_sg_req))
 		return;
 
 	hsgreq = list_first_entry(&tdc->pending_sg_req, typeof(*hsgreq), node);
 	if (!list_is_last(&hsgreq->node, &tdc->pending_sg_req)) {
-		hnsgreq = list_first_entry(&hsgreq->node,
-					typeof(*hnsgreq), node);
+		hnsgreq = list_first_entry(&hsgreq->node, typeof(*hnsgreq),
+					   node);
 		tegra_dma_configure_for_next(tdc, hnsgreq);
 	}
 }
 
-static inline int get_current_xferred_count(struct tegra_dma_channel *tdc,
-	struct tegra_dma_sg_req *sg_req, unsigned long status)
+static inline unsigned int
+get_current_xferred_count(struct tegra_dma_channel *tdc,
+			  struct tegra_dma_sg_req *sg_req,
+			  unsigned long status)
 {
 	return sg_req->req_len - (status & TEGRA_APBDMA_STATUS_COUNT_MASK) - 4;
 }
 
 static void tegra_dma_abort_all(struct tegra_dma_channel *tdc)
 {
-	struct tegra_dma_sg_req *sgreq;
 	struct tegra_dma_desc *dma_desc;
+	struct tegra_dma_sg_req *sgreq;
 
 	while (!list_empty(&tdc->pending_sg_req)) {
-		sgreq = list_first_entry(&tdc->pending_sg_req,
-						typeof(*sgreq), node);
+		sgreq = list_first_entry(&tdc->pending_sg_req, typeof(*sgreq),
+					 node);
 		list_move_tail(&sgreq->node, &tdc->free_sg_req);
 		if (sgreq->last_sg) {
 			dma_desc = sgreq->dma_desc;
@@ -555,7 +554,7 @@ static void tegra_dma_abort_all(struct tegra_dma_channel *tdc)
 			/* Add in cb list if it is not there. */
 			if (!dma_desc->cb_count)
 				list_add_tail(&dma_desc->cb_node,
-							&tdc->cb_desc);
+					      &tdc->cb_desc);
 			dma_desc->cb_count++;
 		}
 	}
@@ -563,9 +562,10 @@ static void tegra_dma_abort_all(struct tegra_dma_channel *tdc)
 }
 
 static bool handle_continuous_head_request(struct tegra_dma_channel *tdc,
-		struct tegra_dma_sg_req *last_sg_req, bool to_terminate)
+					   struct tegra_dma_sg_req *last_sg_req,
+					   bool to_terminate)
 {
-	struct tegra_dma_sg_req *hsgreq = NULL;
+	struct tegra_dma_sg_req *hsgreq;
 
 	if (list_empty(&tdc->pending_sg_req)) {
 		dev_err(tdc2dev(tdc), "DMA is running without req\n");
@@ -589,14 +589,15 @@ static bool handle_continuous_head_request(struct tegra_dma_channel *tdc,
 	/* Configure next request */
 	if (!to_terminate)
 		tdc_configure_next_head_desc(tdc);
+
 	return true;
 }
 
 static void handle_once_dma_done(struct tegra_dma_channel *tdc,
-	bool to_terminate)
+				 bool to_terminate)
 {
-	struct tegra_dma_sg_req *sgreq;
 	struct tegra_dma_desc *dma_desc;
+	struct tegra_dma_sg_req *sgreq;
 
 	tdc->busy = false;
 	sgreq = list_first_entry(&tdc->pending_sg_req, typeof(*sgreq), node);
@@ -622,10 +623,10 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
 }
 
 static void handle_cont_sngl_cycle_dma_done(struct tegra_dma_channel *tdc,
-		bool to_terminate)
+					    bool to_terminate)
 {
-	struct tegra_dma_sg_req *sgreq;
 	struct tegra_dma_desc *dma_desc;
+	struct tegra_dma_sg_req *sgreq;
 	bool st;
 
 	sgreq = list_first_entry(&tdc->pending_sg_req, typeof(*sgreq), node);
@@ -657,13 +658,13 @@ static void tegra_dma_tasklet(unsigned long data)
 	struct tegra_dma_channel *tdc = (struct tegra_dma_channel *)data;
 	struct dmaengine_desc_callback cb;
 	struct tegra_dma_desc *dma_desc;
+	unsigned int cb_count;
 	unsigned long flags;
-	int cb_count;
 
 	spin_lock_irqsave(&tdc->lock, flags);
 	while (!list_empty(&tdc->cb_desc)) {
-		dma_desc  = list_first_entry(&tdc->cb_desc,
-					typeof(*dma_desc), cb_node);
+		dma_desc = list_first_entry(&tdc->cb_desc, typeof(*dma_desc),
+					    cb_node);
 		list_del(&dma_desc->cb_node);
 		dmaengine_desc_get_callback(&dma_desc->txd, &cb);
 		cb_count = dma_desc->cb_count;
@@ -681,8 +682,8 @@ static void tegra_dma_tasklet(unsigned long data)
 static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
 {
 	struct tegra_dma_channel *tdc = dev_id;
-	unsigned long status;
 	unsigned long flags;
+	u32 status;
 
 	spin_lock_irqsave(&tdc->lock, flags);
 
@@ -697,8 +698,9 @@ static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
 	}
 
 	spin_unlock_irqrestore(&tdc->lock, flags);
-	dev_info(tdc2dev(tdc),
-		"Interrupt already served status 0x%08lx\n", status);
+	dev_info(tdc2dev(tdc), "Interrupt already served status 0x%08x\n",
+		 status);
+
 	return IRQ_NONE;
 }
 
@@ -714,6 +716,7 @@ static dma_cookie_t tegra_dma_tx_submit(struct dma_async_tx_descriptor *txd)
 	cookie = dma_cookie_assign(&dma_desc->txd);
 	list_splice_tail_init(&dma_desc->tx_list, &tdc->pending_sg_req);
 	spin_unlock_irqrestore(&tdc->lock, flags);
+
 	return cookie;
 }
 
@@ -747,11 +750,10 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
 static int tegra_dma_terminate_all(struct dma_chan *dc)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
-	struct tegra_dma_sg_req *sgreq;
 	struct tegra_dma_desc *dma_desc;
+	struct tegra_dma_sg_req *sgreq;
 	unsigned long flags;
-	unsigned long status;
-	unsigned long wcount;
+	u32 status, wcount;
 	bool was_busy;
 
 	spin_lock_irqsave(&tdc->lock, flags);
@@ -777,8 +779,8 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	tegra_dma_stop(tdc);
 
 	if (!list_empty(&tdc->pending_sg_req) && was_busy) {
-		sgreq = list_first_entry(&tdc->pending_sg_req,
-					typeof(*sgreq), node);
+		sgreq = list_first_entry(&tdc->pending_sg_req, typeof(*sgreq),
+					 node);
 		sgreq->dma_desc->bytes_transferred +=
 				get_current_xferred_count(tdc, sgreq, wcount);
 	}
@@ -788,12 +790,13 @@ static int tegra_dma_terminate_all(struct dma_chan *dc)
 	tegra_dma_abort_all(tdc);
 
 	while (!list_empty(&tdc->cb_desc)) {
-		dma_desc  = list_first_entry(&tdc->cb_desc,
-					typeof(*dma_desc), cb_node);
+		dma_desc = list_first_entry(&tdc->cb_desc, typeof(*dma_desc),
+					    cb_node);
 		list_del(&dma_desc->cb_node);
 		dma_desc->cb_count = 0;
 	}
 	spin_unlock_irqrestore(&tdc->lock, flags);
+
 	return 0;
 }
 
@@ -807,7 +810,7 @@ static void tegra_dma_synchronize(struct dma_chan *dc)
 static unsigned int tegra_dma_sg_bytes_xferred(struct tegra_dma_channel *tdc,
 					       struct tegra_dma_sg_req *sg_req)
 {
-	unsigned long status, wcount = 0;
+	u32 status, wcount = 0;
 
 	if (!list_is_first(&sg_req->node, &tdc->pending_sg_req))
 		return 0;
@@ -864,7 +867,8 @@ static unsigned int tegra_dma_sg_bytes_xferred(struct tegra_dma_channel *tdc,
 }
 
 static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
-	dma_cookie_t cookie, struct dma_tx_state *txstate)
+					   dma_cookie_t cookie,
+					   struct dma_tx_state *txstate)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
 	struct tegra_dma_desc *dma_desc;
@@ -911,11 +915,12 @@ static enum dma_status tegra_dma_tx_status(struct dma_chan *dc,
 
 	trace_tegra_dma_tx_status(&tdc->dma_chan, cookie, txstate);
 	spin_unlock_irqrestore(&tdc->lock, flags);
+
 	return ret;
 }
 
-static inline int get_bus_width(struct tegra_dma_channel *tdc,
-		enum dma_slave_buswidth slave_bw)
+static inline unsigned int get_bus_width(struct tegra_dma_channel *tdc,
+					 enum dma_slave_buswidth slave_bw)
 {
 	switch (slave_bw) {
 	case DMA_SLAVE_BUSWIDTH_1_BYTE:
@@ -928,16 +933,17 @@ static inline int get_bus_width(struct tegra_dma_channel *tdc,
 		return TEGRA_APBDMA_APBSEQ_BUS_WIDTH_64;
 	default:
 		dev_warn(tdc2dev(tdc),
-			"slave bw is not supported, using 32bits\n");
+			 "slave bw is not supported, using 32bits\n");
 		return TEGRA_APBDMA_APBSEQ_BUS_WIDTH_32;
 	}
 }
 
-static inline int get_burst_size(struct tegra_dma_channel *tdc,
-	u32 burst_size, enum dma_slave_buswidth slave_bw, int len)
+static inline unsigned int get_burst_size(struct tegra_dma_channel *tdc,
+					  u32 burst_size,
+					  enum dma_slave_buswidth slave_bw,
+					  u32 len)
 {
-	int burst_byte;
-	int burst_ahb_width;
+	unsigned int burst_byte, burst_ahb_width;
 
 	/*
 	 * burst_size from client is in terms of the bus_width.
@@ -964,9 +970,12 @@ static inline int get_burst_size(struct tegra_dma_channel *tdc,
 }
 
 static int get_transfer_param(struct tegra_dma_channel *tdc,
-	enum dma_transfer_direction direction, unsigned long *apb_addr,
-	unsigned long *apb_seq,	unsigned long *csr, unsigned int *burst_size,
-	enum dma_slave_buswidth *slave_bw)
+			      enum dma_transfer_direction direction,
+			      u32 *apb_addr,
+			      u32 *apb_seq,
+			      u32 *csr,
+			      unsigned int *burst_size,
+			      enum dma_slave_buswidth *slave_bw)
 {
 	switch (direction) {
 	case DMA_MEM_TO_DEV:
@@ -987,13 +996,15 @@ static int get_transfer_param(struct tegra_dma_channel *tdc,
 
 	default:
 		dev_err(tdc2dev(tdc), "DMA direction is not supported\n");
-		return -EINVAL;
+		break;
 	}
+
 	return -EINVAL;
 }
 
 static void tegra_dma_prep_wcount(struct tegra_dma_channel *tdc,
-	struct tegra_dma_channel_regs *ch_regs, u32 len)
+				  struct tegra_dma_channel_regs *ch_regs,
+				  u32 len)
 {
 	u32 len_field = (len - 4) & 0xFFFC;
 
@@ -1003,20 +1014,23 @@ static void tegra_dma_prep_wcount(struct tegra_dma_channel *tdc,
 		ch_regs->csr |= len_field;
 }
 
-static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
-	struct dma_chan *dc, struct scatterlist *sgl, unsigned int sg_len,
-	enum dma_transfer_direction direction, unsigned long flags,
-	void *context)
+static struct dma_async_tx_descriptor *
+tegra_dma_prep_slave_sg(struct dma_chan *dc,
+			struct scatterlist *sgl,
+			unsigned int sg_len,
+			enum dma_transfer_direction direction,
+			unsigned long flags,
+			void *context)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
+	struct tegra_dma_sg_req *sg_req = NULL;
+	u32 csr, ahb_seq, apb_ptr, apb_seq;
+	enum dma_slave_buswidth slave_bw;
 	struct tegra_dma_desc *dma_desc;
-	unsigned int i;
-	struct scatterlist *sg;
-	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
 	struct list_head req_list;
-	struct tegra_dma_sg_req  *sg_req = NULL;
-	u32 burst_size;
-	enum dma_slave_buswidth slave_bw;
+	struct scatterlist *sg;
+	unsigned int burst_size;
+	unsigned int i;
 
 	if (!tdc->config_init) {
 		dev_err(tdc2dev(tdc), "DMA channel is not configured\n");
@@ -1028,7 +1042,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
 	}
 
 	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
-				&burst_size, &slave_bw) < 0)
+			       &burst_size, &slave_bw) < 0)
 		return NULL;
 
 	INIT_LIST_HEAD(&req_list);
@@ -1074,7 +1088,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
 		len = sg_dma_len(sg);
 
 		if ((len & 3) || (mem & 3) ||
-				(len > tdc->tdma->chip_data->max_dma_count)) {
+		    len > tdc->tdma->chip_data->max_dma_count) {
 			dev_err(tdc2dev(tdc),
 				"DMA length/memory address is not supported\n");
 			tegra_dma_desc_put(tdc, dma_desc);
@@ -1126,20 +1140,21 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
 	return &dma_desc->txd;
 }
 
-static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
-	struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
-	size_t period_len, enum dma_transfer_direction direction,
-	unsigned long flags)
+static struct dma_async_tx_descriptor *
+tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr,
+			  size_t buf_len,
+			  size_t period_len,
+			  enum dma_transfer_direction direction,
+			  unsigned long flags)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
-	struct tegra_dma_desc *dma_desc = NULL;
 	struct tegra_dma_sg_req *sg_req = NULL;
-	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
-	int len;
-	size_t remain_len;
-	dma_addr_t mem = buf_addr;
-	u32 burst_size;
+	u32 csr, ahb_seq, apb_ptr, apb_seq;
 	enum dma_slave_buswidth slave_bw;
+	struct tegra_dma_desc *dma_desc;
+	dma_addr_t mem = buf_addr;
+	unsigned int burst_size;
+	size_t len, remain_len;
 
 	if (!buf_len || !period_len) {
 		dev_err(tdc2dev(tdc), "Invalid buffer/period len\n");
@@ -1173,13 +1188,13 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
 
 	len = period_len;
 	if ((len & 3) || (buf_addr & 3) ||
-			(len > tdc->tdma->chip_data->max_dma_count)) {
+	    len > tdc->tdma->chip_data->max_dma_count) {
 		dev_err(tdc2dev(tdc), "Req len/mem address is not correct\n");
 		return NULL;
 	}
 
 	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
-				&burst_size, &slave_bw) < 0)
+			       &burst_size, &slave_bw) < 0)
 		return NULL;
 
 	ahb_seq = TEGRA_APBDMA_AHBSEQ_INTR_ENB;
@@ -1269,7 +1284,6 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
 	int ret;
 
 	dma_cookie_init(&tdc->dma_chan);
-	tdc->config_init = false;
 
 	ret = pm_runtime_get_sync(tdma->dev);
 	if (ret < 0)
@@ -1303,8 +1317,8 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 	tdc->isr_handler = NULL;
 
 	while (!list_empty(&dma_desc_list)) {
-		dma_desc = list_first_entry(&dma_desc_list,
-					typeof(*dma_desc), node);
+		dma_desc = list_first_entry(&dma_desc_list, typeof(*dma_desc),
+					    node);
 		list_del(&dma_desc->node);
 		kfree(dma_desc);
 	}
@@ -1323,8 +1337,8 @@ static struct dma_chan *tegra_dma_of_xlate(struct of_phandle_args *dma_spec,
 					   struct of_dma *ofdma)
 {
 	struct tegra_dma *tdma = ofdma->of_dma_data;
-	struct dma_chan *chan;
 	struct tegra_dma_channel *tdc;
+	struct dma_chan *chan;
 
 	if (dma_spec->args[0] > TEGRA_APBDMA_CSR_REQ_SEL_MASK) {
 		dev_err(tdma->dev, "Invalid slave id: %d\n", dma_spec->args[0]);
@@ -1379,20 +1393,16 @@ static const struct tegra_dma_chip_data tegra148_dma_chip_data = {
 
 static int tegra_dma_probe(struct platform_device *pdev)
 {
+	const struct tegra_dma_chip_data *cdata;
 	struct tegra_dma *tdma;
+	unsigned int i;
+	size_t size;
 	int ret;
-	int i;
-	const struct tegra_dma_chip_data *cdata;
 
 	cdata = of_device_get_match_data(&pdev->dev);
-	if (!cdata) {
-		dev_err(&pdev->dev, "Error: No device match data found\n");
-		return -ENODEV;
-	}
+	size = struct_size(tdma, channels, cdata->nr_channels);
 
-	tdma = devm_kzalloc(&pdev->dev,
-			    struct_size(tdma, channels, cdata->nr_channels),
-			    GFP_KERNEL);
+	tdma = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
 	if (!tdma)
 		return -ENOMEM;
 
@@ -1424,10 +1434,8 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	else
 		ret = pm_runtime_get_sync(&pdev->dev);
 
-	if (ret < 0) {
-		pm_runtime_disable(&pdev->dev);
-		return ret;
-	}
+	if (ret < 0)
+		goto err_pm_disable;
 
 	/* Reset DMA controller */
 	reset_control_assert(tdma->rst);
@@ -1470,13 +1478,13 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		tdc->dma_chan.device = &tdma->dma_dev;
 		dma_cookie_init(&tdc->dma_chan);
 		list_add_tail(&tdc->dma_chan.device_node,
-				&tdma->dma_dev.channels);
+			      &tdma->dma_dev.channels);
 		tdc->tdma = tdma;
 		tdc->id = i;
 		tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
 
 		tasklet_init(&tdc->tasklet, tegra_dma_tasklet,
-				(unsigned long)tdc);
+			     (unsigned long)tdc);
 		spin_lock_init(&tdc->lock);
 
 		INIT_LIST_HEAD(&tdc->pending_sg_req);
@@ -1528,16 +1536,19 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		goto err_unregister_dma_dev;
 	}
 
-	dev_info(&pdev->dev, "Tegra20 APB DMA driver register %d channels\n",
-			cdata->nr_channels);
+	dev_info(&pdev->dev, "Tegra20 APB DMA driver registered %u channels\n",
+		 cdata->nr_channels);
+
 	return 0;
 
 err_unregister_dma_dev:
 	dma_async_device_unregister(&tdma->dma_dev);
+
 err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		tegra_dma_runtime_suspend(&pdev->dev);
+
 	return ret;
 }
 
@@ -1557,7 +1568,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
 static int tegra_dma_runtime_suspend(struct device *dev)
 {
 	struct tegra_dma *tdma = dev_get_drvdata(dev);
-	int i;
+	unsigned int i;
 
 	tdma->reg_gen = tdma_read(tdma, TEGRA_APBDMA_GENERAL);
 	for (i = 0; i < tdma->chip_data->nr_channels; i++) {
@@ -1586,7 +1597,8 @@ static int tegra_dma_runtime_suspend(struct device *dev)
 static int tegra_dma_runtime_resume(struct device *dev)
 {
 	struct tegra_dma *tdma = dev_get_drvdata(dev);
-	int i, ret;
+	unsigned int i;
+	int ret;
 
 	ret = clk_prepare_enable(tdma->dma_clk);
 	if (ret < 0) {
@@ -1614,7 +1626,7 @@ static int tegra_dma_runtime_resume(struct device *dev)
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBSEQ, ch_reg->ahb_seq);
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBPTR, ch_reg->ahb_ptr);
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR,
-			(ch_reg->csr & ~TEGRA_APBDMA_CSR_ENB));
+			  ch_reg->csr & ~TEGRA_APBDMA_CSR_ENB);
 	}
 
 	return 0;
-- 
2.24.0

Re: [PATCH v4 08/14] dmaengine: tegra-apb: Fix coding style problems

[Jon Hunter]

On 12/01/2020 17:30, Dmitry Osipenko wrote:
> This patch fixes few dozens of coding style problems reported by
> checkpatch and prettifies code where makes sense.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/dma/tegra20-apb-dma.c | 276 ++++++++++++++++++----------------
>  1 file changed, 144 insertions(+), 132 deletions(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index dff21e80ffa4..7158bd3145c4 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c

...

> @@ -1003,20 +1014,23 @@ static void tegra_dma_prep_wcount(struct tegra_dma_channel *tdc,
>  		ch_regs->csr |= len_field;
>  }
>  
> -static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
> -	struct dma_chan *dc, struct scatterlist *sgl, unsigned int sg_len,
> -	enum dma_transfer_direction direction, unsigned long flags,
> -	void *context)
> +static struct dma_async_tx_descriptor *
> +tegra_dma_prep_slave_sg(struct dma_chan *dc,
> +			struct scatterlist *sgl,
> +			unsigned int sg_len,
> +			enum dma_transfer_direction direction,
> +			unsigned long flags,
> +			void *context)
>  {
>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> +	struct tegra_dma_sg_req *sg_req = NULL;
> +	u32 csr, ahb_seq, apb_ptr, apb_seq;
> +	enum dma_slave_buswidth slave_bw;
>  	struct tegra_dma_desc *dma_desc;
> -	unsigned int i;
> -	struct scatterlist *sg;
> -	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
>  	struct list_head req_list;
> -	struct tegra_dma_sg_req  *sg_req = NULL;
> -	u32 burst_size;
> -	enum dma_slave_buswidth slave_bw;
> +	struct scatterlist *sg;
> +	unsigned int burst_size;
> +	unsigned int i;

This is not really consistent with the rest of the changes by having 'i'
and 'burst_size' on separate lines.

>  
>  	if (!tdc->config_init) {
>  		dev_err(tdc2dev(tdc), "DMA channel is not configured\n");
> @@ -1028,7 +1042,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>  	}
>  
>  	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
> -				&burst_size, &slave_bw) < 0)
> +			       &burst_size, &slave_bw) < 0)
>  		return NULL;
>  
>  	INIT_LIST_HEAD(&req_list);
> @@ -1074,7 +1088,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>  		len = sg_dma_len(sg);
>  
>  		if ((len & 3) || (mem & 3) ||
> -				(len > tdc->tdma->chip_data->max_dma_count)) {
> +		    len > tdc->tdma->chip_data->max_dma_count) {
>  			dev_err(tdc2dev(tdc),
>  				"DMA length/memory address is not supported\n");
>  			tegra_dma_desc_put(tdc, dma_desc);
> @@ -1126,20 +1140,21 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>  	return &dma_desc->txd;
>  }
>  
> -static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
> -	struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
> -	size_t period_len, enum dma_transfer_direction direction,
> -	unsigned long flags)
> +static struct dma_async_tx_descriptor *
> +tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr,
> +			  size_t buf_len,
> +			  size_t period_len,
> +			  enum dma_transfer_direction direction,
> +			  unsigned long flags)
>  {
>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> -	struct tegra_dma_desc *dma_desc = NULL;
>  	struct tegra_dma_sg_req *sg_req = NULL;
> -	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
> -	int len;
> -	size_t remain_len;
> -	dma_addr_t mem = buf_addr;
> -	u32 burst_size;
> +	u32 csr, ahb_seq, apb_ptr, apb_seq;
>  	enum dma_slave_buswidth slave_bw;
> +	struct tegra_dma_desc *dma_desc;
> +	dma_addr_t mem = buf_addr;
> +	unsigned int burst_size;
> +	size_t len, remain_len;
>  
>  	if (!buf_len || !period_len) {
>  		dev_err(tdc2dev(tdc), "Invalid buffer/period len\n");
> @@ -1173,13 +1188,13 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
>  
>  	len = period_len;
>  	if ((len & 3) || (buf_addr & 3) ||
> -			(len > tdc->tdma->chip_data->max_dma_count)) {
> +	    len > tdc->tdma->chip_data->max_dma_count) {
>  		dev_err(tdc2dev(tdc), "Req len/mem address is not correct\n");
>  		return NULL;
>  	}
>  
>  	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
> -				&burst_size, &slave_bw) < 0)
> +			       &burst_size, &slave_bw) < 0)
>  		return NULL;
>  
>  	ahb_seq = TEGRA_APBDMA_AHBSEQ_INTR_ENB;
> @@ -1269,7 +1284,6 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>  	int ret;
>  
>  	dma_cookie_init(&tdc->dma_chan);
> -	tdc->config_init = false;

Why is this removed? Does not seem to belong in this patch.

Cheers
Jon

-- 
nvpublic

Re: [PATCH v4 08/14] dmaengine: tegra-apb: Fix coding style problems

[Dmitry Osipenko]

15.01.2020 12:49, Jon Hunter пишет:
> 
> 
> On 12/01/2020 17:30, Dmitry Osipenko wrote:
>> This patch fixes few dozens of coding style problems reported by
>> checkpatch and prettifies code where makes sense.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/dma/tegra20-apb-dma.c | 276 ++++++++++++++++++----------------
>>  1 file changed, 144 insertions(+), 132 deletions(-)
>>
>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>> index dff21e80ffa4..7158bd3145c4 100644
>> --- a/drivers/dma/tegra20-apb-dma.c
>> +++ b/drivers/dma/tegra20-apb-dma.c
> 
> ...
> 
>> @@ -1003,20 +1014,23 @@ static void tegra_dma_prep_wcount(struct tegra_dma_channel *tdc,
>>  		ch_regs->csr |= len_field;
>>  }
>>  
>> -static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>> -	struct dma_chan *dc, struct scatterlist *sgl, unsigned int sg_len,
>> -	enum dma_transfer_direction direction, unsigned long flags,
>> -	void *context)
>> +static struct dma_async_tx_descriptor *
>> +tegra_dma_prep_slave_sg(struct dma_chan *dc,
>> +			struct scatterlist *sgl,
>> +			unsigned int sg_len,
>> +			enum dma_transfer_direction direction,
>> +			unsigned long flags,
>> +			void *context)
>>  {
>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>> +	struct tegra_dma_sg_req *sg_req = NULL;
>> +	u32 csr, ahb_seq, apb_ptr, apb_seq;
>> +	enum dma_slave_buswidth slave_bw;
>>  	struct tegra_dma_desc *dma_desc;
>> -	unsigned int i;
>> -	struct scatterlist *sg;
>> -	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
>>  	struct list_head req_list;
>> -	struct tegra_dma_sg_req  *sg_req = NULL;
>> -	u32 burst_size;
>> -	enum dma_slave_buswidth slave_bw;
>> +	struct scatterlist *sg;
>> +	unsigned int burst_size;
>> +	unsigned int i;
> 
> This is not really consistent with the rest of the changes by having 'i'
> and 'burst_size' on separate lines.

The goal wasn't to squash everything into a single line, but to make
code more readable. In this particular case the separated lines look
better to me.

>>  
>>  	if (!tdc->config_init) {
>>  		dev_err(tdc2dev(tdc), "DMA channel is not configured\n");
>> @@ -1028,7 +1042,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>  	}
>>  
>>  	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
>> -				&burst_size, &slave_bw) < 0)
>> +			       &burst_size, &slave_bw) < 0)
>>  		return NULL;
>>  
>>  	INIT_LIST_HEAD(&req_list);
>> @@ -1074,7 +1088,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>  		len = sg_dma_len(sg);
>>  
>>  		if ((len & 3) || (mem & 3) ||
>> -				(len > tdc->tdma->chip_data->max_dma_count)) {
>> +		    len > tdc->tdma->chip_data->max_dma_count) {
>>  			dev_err(tdc2dev(tdc),
>>  				"DMA length/memory address is not supported\n");
>>  			tegra_dma_desc_put(tdc, dma_desc);
>> @@ -1126,20 +1140,21 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>  	return &dma_desc->txd;
>>  }
>>  
>> -static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
>> -	struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
>> -	size_t period_len, enum dma_transfer_direction direction,
>> -	unsigned long flags)
>> +static struct dma_async_tx_descriptor *
>> +tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr,
>> +			  size_t buf_len,
>> +			  size_t period_len,
>> +			  enum dma_transfer_direction direction,
>> +			  unsigned long flags)
>>  {
>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>> -	struct tegra_dma_desc *dma_desc = NULL;
>>  	struct tegra_dma_sg_req *sg_req = NULL;
>> -	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
>> -	int len;
>> -	size_t remain_len;
>> -	dma_addr_t mem = buf_addr;
>> -	u32 burst_size;
>> +	u32 csr, ahb_seq, apb_ptr, apb_seq;
>>  	enum dma_slave_buswidth slave_bw;
>> +	struct tegra_dma_desc *dma_desc;
>> +	dma_addr_t mem = buf_addr;
>> +	unsigned int burst_size;
>> +	size_t len, remain_len;
>>  
>>  	if (!buf_len || !period_len) {
>>  		dev_err(tdc2dev(tdc), "Invalid buffer/period len\n");
>> @@ -1173,13 +1188,13 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
>>  
>>  	len = period_len;
>>  	if ((len & 3) || (buf_addr & 3) ||
>> -			(len > tdc->tdma->chip_data->max_dma_count)) {
>> +	    len > tdc->tdma->chip_data->max_dma_count) {
>>  		dev_err(tdc2dev(tdc), "Req len/mem address is not correct\n");
>>  		return NULL;
>>  	}
>>  
>>  	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
>> -				&burst_size, &slave_bw) < 0)
>> +			       &burst_size, &slave_bw) < 0)
>>  		return NULL;
>>  
>>  	ahb_seq = TEGRA_APBDMA_AHBSEQ_INTR_ENB;
>> @@ -1269,7 +1284,6 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>>  	int ret;
>>  
>>  	dma_cookie_init(&tdc->dma_chan);
>> -	tdc->config_init = false;
> 
> Why is this removed? Does not seem to belong in this patch.

Because initially, on driver's probe, the tdc->config_init is false for
all channels and then tegra_dma_free_chan_resources() also sets it to
false. Thus there is no need to re-initilize the already initialized
variable. It's not a very good coding style if variables are
unnecessarily initialized, you probably noticed that there are few other
cases of removing the unneeded initializations of local variables in
this patch.

Re: [PATCH v4 08/14] dmaengine: tegra-apb: Fix coding style problems

[Jon Hunter]

On 16/01/2020 17:37, Dmitry Osipenko wrote:
> 15.01.2020 12:49, Jon Hunter пишет:
>>
>>
>> On 12/01/2020 17:30, Dmitry Osipenko wrote:
>>> This patch fixes few dozens of coding style problems reported by
>>> checkpatch and prettifies code where makes sense.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>>  drivers/dma/tegra20-apb-dma.c | 276 ++++++++++++++++++----------------
>>>  1 file changed, 144 insertions(+), 132 deletions(-)
>>>
>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>>> index dff21e80ffa4..7158bd3145c4 100644
>>> --- a/drivers/dma/tegra20-apb-dma.c
>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>
>> ...
>>
>>> @@ -1003,20 +1014,23 @@ static void tegra_dma_prep_wcount(struct tegra_dma_channel *tdc,
>>>  		ch_regs->csr |= len_field;
>>>  }
>>>  
>>> -static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>> -	struct dma_chan *dc, struct scatterlist *sgl, unsigned int sg_len,
>>> -	enum dma_transfer_direction direction, unsigned long flags,
>>> -	void *context)
>>> +static struct dma_async_tx_descriptor *
>>> +tegra_dma_prep_slave_sg(struct dma_chan *dc,
>>> +			struct scatterlist *sgl,
>>> +			unsigned int sg_len,
>>> +			enum dma_transfer_direction direction,
>>> +			unsigned long flags,
>>> +			void *context)
>>>  {
>>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>> +	struct tegra_dma_sg_req *sg_req = NULL;
>>> +	u32 csr, ahb_seq, apb_ptr, apb_seq;
>>> +	enum dma_slave_buswidth slave_bw;
>>>  	struct tegra_dma_desc *dma_desc;
>>> -	unsigned int i;
>>> -	struct scatterlist *sg;
>>> -	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
>>>  	struct list_head req_list;
>>> -	struct tegra_dma_sg_req  *sg_req = NULL;
>>> -	u32 burst_size;
>>> -	enum dma_slave_buswidth slave_bw;
>>> +	struct scatterlist *sg;
>>> +	unsigned int burst_size;
>>> +	unsigned int i;
>>
>> This is not really consistent with the rest of the changes by having 'i'
>> and 'burst_size' on separate lines.
> 
> The goal wasn't to squash everything into a single line, but to make
> code more readable. In this particular case the separated lines look
> better to me.
> 
>>>  
>>>  	if (!tdc->config_init) {
>>>  		dev_err(tdc2dev(tdc), "DMA channel is not configured\n");
>>> @@ -1028,7 +1042,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>>  	}
>>>  
>>>  	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
>>> -				&burst_size, &slave_bw) < 0)
>>> +			       &burst_size, &slave_bw) < 0)
>>>  		return NULL;
>>>  
>>>  	INIT_LIST_HEAD(&req_list);
>>> @@ -1074,7 +1088,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>>  		len = sg_dma_len(sg);
>>>  
>>>  		if ((len & 3) || (mem & 3) ||
>>> -				(len > tdc->tdma->chip_data->max_dma_count)) {
>>> +		    len > tdc->tdma->chip_data->max_dma_count) {
>>>  			dev_err(tdc2dev(tdc),
>>>  				"DMA length/memory address is not supported\n");
>>>  			tegra_dma_desc_put(tdc, dma_desc);
>>> @@ -1126,20 +1140,21 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>>  	return &dma_desc->txd;
>>>  }
>>>  
>>> -static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
>>> -	struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
>>> -	size_t period_len, enum dma_transfer_direction direction,
>>> -	unsigned long flags)
>>> +static struct dma_async_tx_descriptor *
>>> +tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr,
>>> +			  size_t buf_len,
>>> +			  size_t period_len,
>>> +			  enum dma_transfer_direction direction,
>>> +			  unsigned long flags)
>>>  {
>>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>> -	struct tegra_dma_desc *dma_desc = NULL;
>>>  	struct tegra_dma_sg_req *sg_req = NULL;
>>> -	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
>>> -	int len;
>>> -	size_t remain_len;
>>> -	dma_addr_t mem = buf_addr;
>>> -	u32 burst_size;
>>> +	u32 csr, ahb_seq, apb_ptr, apb_seq;
>>>  	enum dma_slave_buswidth slave_bw;
>>> +	struct tegra_dma_desc *dma_desc;
>>> +	dma_addr_t mem = buf_addr;
>>> +	unsigned int burst_size;
>>> +	size_t len, remain_len;
>>>  
>>>  	if (!buf_len || !period_len) {
>>>  		dev_err(tdc2dev(tdc), "Invalid buffer/period len\n");
>>> @@ -1173,13 +1188,13 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
>>>  
>>>  	len = period_len;
>>>  	if ((len & 3) || (buf_addr & 3) ||
>>> -			(len > tdc->tdma->chip_data->max_dma_count)) {
>>> +	    len > tdc->tdma->chip_data->max_dma_count) {
>>>  		dev_err(tdc2dev(tdc), "Req len/mem address is not correct\n");
>>>  		return NULL;
>>>  	}
>>>  
>>>  	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
>>> -				&burst_size, &slave_bw) < 0)
>>> +			       &burst_size, &slave_bw) < 0)
>>>  		return NULL;
>>>  
>>>  	ahb_seq = TEGRA_APBDMA_AHBSEQ_INTR_ENB;
>>> @@ -1269,7 +1284,6 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>>>  	int ret;
>>>  
>>>  	dma_cookie_init(&tdc->dma_chan);
>>> -	tdc->config_init = false;
>>
>> Why is this removed? Does not seem to belong in this patch.
> 
> Because initially, on driver's probe, the tdc->config_init is false for
> all channels and then tegra_dma_free_chan_resources() also sets it to
> false. Thus there is no need to re-initilize the already initialized
> variable. It's not a very good coding style if variables are
> unnecessarily initialized, you probably noticed that there are few other
> cases of removing the unneeded initializations of local variables in
> this patch.

OK, but I don't really consider this coding-style and would prefer a
separate patch for this.

Jon

-- 
nvpublic

Re: [PATCH v4 08/14] dmaengine: tegra-apb: Fix coding style problems

[Dmitry Osipenko]

28.01.2020 17:05, Jon Hunter пишет:
> 
> On 16/01/2020 17:37, Dmitry Osipenko wrote:
>> 15.01.2020 12:49, Jon Hunter пишет:
>>>
>>>
>>> On 12/01/2020 17:30, Dmitry Osipenko wrote:
>>>> This patch fixes few dozens of coding style problems reported by
>>>> checkpatch and prettifies code where makes sense.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  drivers/dma/tegra20-apb-dma.c | 276 ++++++++++++++++++----------------
>>>>  1 file changed, 144 insertions(+), 132 deletions(-)
>>>>
>>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>>>> index dff21e80ffa4..7158bd3145c4 100644
>>>> --- a/drivers/dma/tegra20-apb-dma.c
>>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>>
>>> ...
>>>
>>>> @@ -1003,20 +1014,23 @@ static void tegra_dma_prep_wcount(struct tegra_dma_channel *tdc,
>>>>  		ch_regs->csr |= len_field;
>>>>  }
>>>>  
>>>> -static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>>> -	struct dma_chan *dc, struct scatterlist *sgl, unsigned int sg_len,
>>>> -	enum dma_transfer_direction direction, unsigned long flags,
>>>> -	void *context)
>>>> +static struct dma_async_tx_descriptor *
>>>> +tegra_dma_prep_slave_sg(struct dma_chan *dc,
>>>> +			struct scatterlist *sgl,
>>>> +			unsigned int sg_len,
>>>> +			enum dma_transfer_direction direction,
>>>> +			unsigned long flags,
>>>> +			void *context)
>>>>  {
>>>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>>> +	struct tegra_dma_sg_req *sg_req = NULL;
>>>> +	u32 csr, ahb_seq, apb_ptr, apb_seq;
>>>> +	enum dma_slave_buswidth slave_bw;
>>>>  	struct tegra_dma_desc *dma_desc;
>>>> -	unsigned int i;
>>>> -	struct scatterlist *sg;
>>>> -	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
>>>>  	struct list_head req_list;
>>>> -	struct tegra_dma_sg_req  *sg_req = NULL;
>>>> -	u32 burst_size;
>>>> -	enum dma_slave_buswidth slave_bw;
>>>> +	struct scatterlist *sg;
>>>> +	unsigned int burst_size;
>>>> +	unsigned int i;
>>>
>>> This is not really consistent with the rest of the changes by having 'i'
>>> and 'burst_size' on separate lines.
>>
>> The goal wasn't to squash everything into a single line, but to make
>> code more readable. In this particular case the separated lines look
>> better to me.
>>
>>>>  
>>>>  	if (!tdc->config_init) {
>>>>  		dev_err(tdc2dev(tdc), "DMA channel is not configured\n");
>>>> @@ -1028,7 +1042,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>>>  	}
>>>>  
>>>>  	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
>>>> -				&burst_size, &slave_bw) < 0)
>>>> +			       &burst_size, &slave_bw) < 0)
>>>>  		return NULL;
>>>>  
>>>>  	INIT_LIST_HEAD(&req_list);
>>>> @@ -1074,7 +1088,7 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>>>  		len = sg_dma_len(sg);
>>>>  
>>>>  		if ((len & 3) || (mem & 3) ||
>>>> -				(len > tdc->tdma->chip_data->max_dma_count)) {
>>>> +		    len > tdc->tdma->chip_data->max_dma_count) {
>>>>  			dev_err(tdc2dev(tdc),
>>>>  				"DMA length/memory address is not supported\n");
>>>>  			tegra_dma_desc_put(tdc, dma_desc);
>>>> @@ -1126,20 +1140,21 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
>>>>  	return &dma_desc->txd;
>>>>  }
>>>>  
>>>> -static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
>>>> -	struct dma_chan *dc, dma_addr_t buf_addr, size_t buf_len,
>>>> -	size_t period_len, enum dma_transfer_direction direction,
>>>> -	unsigned long flags)
>>>> +static struct dma_async_tx_descriptor *
>>>> +tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr,
>>>> +			  size_t buf_len,
>>>> +			  size_t period_len,
>>>> +			  enum dma_transfer_direction direction,
>>>> +			  unsigned long flags)
>>>>  {
>>>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>>> -	struct tegra_dma_desc *dma_desc = NULL;
>>>>  	struct tegra_dma_sg_req *sg_req = NULL;
>>>> -	unsigned long csr, ahb_seq, apb_ptr, apb_seq;
>>>> -	int len;
>>>> -	size_t remain_len;
>>>> -	dma_addr_t mem = buf_addr;
>>>> -	u32 burst_size;
>>>> +	u32 csr, ahb_seq, apb_ptr, apb_seq;
>>>>  	enum dma_slave_buswidth slave_bw;
>>>> +	struct tegra_dma_desc *dma_desc;
>>>> +	dma_addr_t mem = buf_addr;
>>>> +	unsigned int burst_size;
>>>> +	size_t len, remain_len;
>>>>  
>>>>  	if (!buf_len || !period_len) {
>>>>  		dev_err(tdc2dev(tdc), "Invalid buffer/period len\n");
>>>> @@ -1173,13 +1188,13 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
>>>>  
>>>>  	len = period_len;
>>>>  	if ((len & 3) || (buf_addr & 3) ||
>>>> -			(len > tdc->tdma->chip_data->max_dma_count)) {
>>>> +	    len > tdc->tdma->chip_data->max_dma_count) {
>>>>  		dev_err(tdc2dev(tdc), "Req len/mem address is not correct\n");
>>>>  		return NULL;
>>>>  	}
>>>>  
>>>>  	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
>>>> -				&burst_size, &slave_bw) < 0)
>>>> +			       &burst_size, &slave_bw) < 0)
>>>>  		return NULL;
>>>>  
>>>>  	ahb_seq = TEGRA_APBDMA_AHBSEQ_INTR_ENB;
>>>> @@ -1269,7 +1284,6 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>>>>  	int ret;
>>>>  
>>>>  	dma_cookie_init(&tdc->dma_chan);
>>>> -	tdc->config_init = false;
>>>
>>> Why is this removed? Does not seem to belong in this patch.
>>
>> Because initially, on driver's probe, the tdc->config_init is false for
>> all channels and then tegra_dma_free_chan_resources() also sets it to
>> false. Thus there is no need to re-initilize the already initialized
>> variable. It's not a very good coding style if variables are
>> unnecessarily initialized, you probably noticed that there are few other
>> cases of removing the unneeded initializations of local variables in
>> this patch.
> 
> OK, but I don't really consider this coding-style and would prefer a
> separate patch for this.

If if it's not a coding-style, then how would you name it? :)

[PATCH v4 09/14] dmaengine: tegra-apb: Clean up runtime PM teardown

[Dmitry Osipenko]

It's cleaner to teardown RPM by revering the enable sequence, which makes
code much easier to follow.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/dma/tegra20-apb-dma.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 7158bd3145c4..cc4a9ca20780 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1429,13 +1429,15 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	spin_lock_init(&tdma->global_lock);
 
 	pm_runtime_enable(&pdev->dev);
-	if (!pm_runtime_enabled(&pdev->dev))
+	if (!pm_runtime_enabled(&pdev->dev)) {
 		ret = tegra_dma_runtime_resume(&pdev->dev);
-	else
+		if (ret)
+			return ret;
+	} else {
 		ret = pm_runtime_get_sync(&pdev->dev);
-
-	if (ret < 0)
-		goto err_pm_disable;
+		if (ret < 0)
+			goto err_pm_disable;
+	}
 
 	/* Reset DMA controller */
 	reset_control_assert(tdma->rst);
@@ -1545,9 +1547,10 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	dma_async_device_unregister(&tdma->dma_dev);
 
 err_pm_disable:
-	pm_runtime_disable(&pdev->dev);
-	if (!pm_runtime_status_suspended(&pdev->dev))
+	if (!pm_runtime_enabled(&pdev->dev))
 		tegra_dma_runtime_suspend(&pdev->dev);
+	else
+		pm_runtime_disable(&pdev->dev);
 
 	return ret;
 }
@@ -1558,9 +1561,10 @@ static int tegra_dma_remove(struct platform_device *pdev)
 
 	dma_async_device_unregister(&tdma->dma_dev);
 
-	pm_runtime_disable(&pdev->dev);
-	if (!pm_runtime_status_suspended(&pdev->dev))
+	if (!pm_runtime_enabled(&pdev->dev))
 		tegra_dma_runtime_suspend(&pdev->dev);
+	else
+		pm_runtime_disable(&pdev->dev);
 
 	return 0;
 }
-- 
2.24.0

Re: [PATCH v4 09/14] dmaengine: tegra-apb: Clean up runtime PM teardown

[Jon Hunter]

On 12/01/2020 17:30, Dmitry Osipenko wrote:
> It's cleaner to teardown RPM by revering the enable sequence, which makes
> code much easier to follow.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/dma/tegra20-apb-dma.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 7158bd3145c4..cc4a9ca20780 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -1429,13 +1429,15 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  	spin_lock_init(&tdma->global_lock);
>  
>  	pm_runtime_enable(&pdev->dev);
> -	if (!pm_runtime_enabled(&pdev->dev))
> +	if (!pm_runtime_enabled(&pdev->dev)) {
>  		ret = tegra_dma_runtime_resume(&pdev->dev);
> -	else
> +		if (ret)
> +			return ret;
> +	} else {
>  		ret = pm_runtime_get_sync(&pdev->dev);
> -
> -	if (ret < 0)
> -		goto err_pm_disable;
> +		if (ret < 0)
> +			goto err_pm_disable;
> +	}
>  
>  	/* Reset DMA controller */
>  	reset_control_assert(tdma->rst);
> @@ -1545,9 +1547,10 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  	dma_async_device_unregister(&tdma->dma_dev);
>  
>  err_pm_disable:
> -	pm_runtime_disable(&pdev->dev);
> -	if (!pm_runtime_status_suspended(&pdev->dev))
> +	if (!pm_runtime_enabled(&pdev->dev))
>  		tegra_dma_runtime_suspend(&pdev->dev);
> +	else
> +		pm_runtime_disable(&pdev->dev);
>  
>  	return ret;
>  }
> @@ -1558,9 +1561,10 @@ static int tegra_dma_remove(struct platform_device *pdev)
>  
>  	dma_async_device_unregister(&tdma->dma_dev);
>  
> -	pm_runtime_disable(&pdev->dev);
> -	if (!pm_runtime_status_suspended(&pdev->dev))
> +	if (!pm_runtime_enabled(&pdev->dev))
>  		tegra_dma_runtime_suspend(&pdev->dev);
> +	else
> +		pm_runtime_disable(&pdev->dev);

Looks like dma_async_device_unregister() will warn if a client still has
a channel requested but does not prevent the unregister from completing.
So it could be possible that we could be leaving the controller active now.

Jon

-- 
nvpublic

Re: [PATCH v4 09/14] dmaengine: tegra-apb: Clean up runtime PM teardown

[Dmitry Osipenko]

15.01.2020 12:57, Jon Hunter пишет:
> 
> On 12/01/2020 17:30, Dmitry Osipenko wrote:
>> It's cleaner to teardown RPM by revering the enable sequence, which makes
>> code much easier to follow.
>>
>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/dma/tegra20-apb-dma.c | 22 +++++++++++++---------
>>  1 file changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>> index 7158bd3145c4..cc4a9ca20780 100644
>> --- a/drivers/dma/tegra20-apb-dma.c
>> +++ b/drivers/dma/tegra20-apb-dma.c
>> @@ -1429,13 +1429,15 @@ static int tegra_dma_probe(struct platform_device *pdev)
>>  	spin_lock_init(&tdma->global_lock);
>>  
>>  	pm_runtime_enable(&pdev->dev);
>> -	if (!pm_runtime_enabled(&pdev->dev))
>> +	if (!pm_runtime_enabled(&pdev->dev)) {
>>  		ret = tegra_dma_runtime_resume(&pdev->dev);
>> -	else
>> +		if (ret)
>> +			return ret;
>> +	} else {
>>  		ret = pm_runtime_get_sync(&pdev->dev);
>> -
>> -	if (ret < 0)
>> -		goto err_pm_disable;
>> +		if (ret < 0)
>> +			goto err_pm_disable;
>> +	}
>>  
>>  	/* Reset DMA controller */
>>  	reset_control_assert(tdma->rst);
>> @@ -1545,9 +1547,10 @@ static int tegra_dma_probe(struct platform_device *pdev)
>>  	dma_async_device_unregister(&tdma->dma_dev);
>>  
>>  err_pm_disable:
>> -	pm_runtime_disable(&pdev->dev);
>> -	if (!pm_runtime_status_suspended(&pdev->dev))
>> +	if (!pm_runtime_enabled(&pdev->dev))
>>  		tegra_dma_runtime_suspend(&pdev->dev);
>> +	else
>> +		pm_runtime_disable(&pdev->dev);
>>  
>>  	return ret;
>>  }
>> @@ -1558,9 +1561,10 @@ static int tegra_dma_remove(struct platform_device *pdev)
>>  
>>  	dma_async_device_unregister(&tdma->dma_dev);
>>  
>> -	pm_runtime_disable(&pdev->dev);
>> -	if (!pm_runtime_status_suspended(&pdev->dev))
>> +	if (!pm_runtime_enabled(&pdev->dev))
>>  		tegra_dma_runtime_suspend(&pdev->dev);
>> +	else
>> +		pm_runtime_disable(&pdev->dev);
> 
> Looks like dma_async_device_unregister() will warn if a client still has
> a channel requested but does not prevent the unregister from completing.
> So it could be possible that we could be leaving the controller active now.

It's a drivers dependency bug if DMA driver's module isn't properly
refcounted and thus could be removed while it has active users. Nothing
we can do about it here, the actual source of the bug needs to be fixed.

Perhaps Tegra DMA driver could inc/dec module's refcounf on channel's
request/free, but I think it should be responsibility of the DMA core to
care about the refcounting (if it doesn't do it already).

[PATCH v4 10/14] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

[Dmitry Osipenko]

It's a bit impractical to enable hardware's clock at the time of DMA
channel's allocation because most of DMA client drivers allocate DMA
channel at the time of the driver's probing and thus DMA clock is kept
always-enabled in practice, defeating the whole purpose of runtime PM.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/dma/tegra20-apb-dma.c | 43 ++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index cc4a9ca20780..b9d8e57eaf54 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -436,6 +436,8 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc)
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
 	}
 	tdc->busy = false;
+
+	pm_runtime_put(tdc->tdma->dev);
 }
 
 static void tegra_dma_start(struct tegra_dma_channel *tdc,
@@ -500,18 +502,25 @@ static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc,
 	tegra_dma_resume(tdc);
 }
 
-static void tdc_start_head_req(struct tegra_dma_channel *tdc)
+static bool tdc_start_head_req(struct tegra_dma_channel *tdc)
 {
 	struct tegra_dma_sg_req *sg_req;
+	int err;
 
 	if (list_empty(&tdc->pending_sg_req))
-		return;
+		return false;
+
+	err = pm_runtime_get_sync(tdc->tdma->dev);
+	if (WARN_ON_ONCE(err < 0))
+		return false;
 
 	sg_req = list_first_entry(&tdc->pending_sg_req, typeof(*sg_req), node);
 	tegra_dma_start(tdc, sg_req);
 	sg_req->configured = true;
 	sg_req->words_xferred = 0;
 	tdc->busy = true;
+
+	return true;
 }
 
 static void tdc_configure_next_head_desc(struct tegra_dma_channel *tdc)
@@ -615,6 +624,8 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
 	}
 	list_add_tail(&sgreq->node, &tdc->free_sg_req);
 
+	pm_runtime_put(tdc->tdma->dev);
+
 	/* Do not start DMA if it is going to be terminate */
 	if (to_terminate || list_empty(&tdc->pending_sg_req))
 		return;
@@ -730,9 +741,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
 		dev_err(tdc2dev(tdc), "No DMA request\n");
 		goto end;
 	}
-	if (!tdc->busy) {
-		tdc_start_head_req(tdc);
-
+	if (!tdc->busy && tdc_start_head_req(tdc)) {
 		/* Continuous single mode: Configure next req */
 		if (tdc->cyclic) {
 			/*
@@ -1280,22 +1289,15 @@ tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr,
 static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
-	struct tegra_dma *tdma = tdc->tdma;
-	int ret;
 
 	dma_cookie_init(&tdc->dma_chan);
 
-	ret = pm_runtime_get_sync(tdma->dev);
-	if (ret < 0)
-		return ret;
-
 	return 0;
 }
 
 static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
-	struct tegra_dma *tdma = tdc->tdma;
 	struct tegra_dma_desc *dma_desc;
 	struct tegra_dma_sg_req *sg_req;
 	struct list_head dma_desc_list;
@@ -1328,7 +1330,6 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 		list_del(&sg_req->node);
 		kfree(sg_req);
 	}
-	pm_runtime_put(tdma->dev);
 
 	tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
 }
@@ -1428,11 +1429,16 @@ static int tegra_dma_probe(struct platform_device *pdev)
 
 	spin_lock_init(&tdma->global_lock);
 
+	ret = clk_prepare(tdma->dma_clk);
+	if (ret)
+		return ret;
+
+	pm_runtime_irq_safe(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 	if (!pm_runtime_enabled(&pdev->dev)) {
 		ret = tegra_dma_runtime_resume(&pdev->dev);
 		if (ret)
-			return ret;
+			goto err_clk_unprepare;
 	} else {
 		ret = pm_runtime_get_sync(&pdev->dev);
 		if (ret < 0)
@@ -1552,6 +1558,9 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	else
 		pm_runtime_disable(&pdev->dev);
 
+err_clk_unprepare:
+	clk_unprepare(tdma->dma_clk);
+
 	return ret;
 }
 
@@ -1566,6 +1575,8 @@ static int tegra_dma_remove(struct platform_device *pdev)
 	else
 		pm_runtime_disable(&pdev->dev);
 
+	clk_unprepare(tdma->dma_clk);
+
 	return 0;
 }
 
@@ -1593,7 +1604,7 @@ static int tegra_dma_runtime_suspend(struct device *dev)
 						  TEGRA_APBDMA_CHAN_WCOUNT);
 	}
 
-	clk_disable_unprepare(tdma->dma_clk);
+	clk_disable(tdma->dma_clk);
 
 	return 0;
 }
@@ -1604,7 +1615,7 @@ static int tegra_dma_runtime_resume(struct device *dev)
 	unsigned int i;
 	int ret;
 
-	ret = clk_prepare_enable(tdma->dma_clk);
+	ret = clk_enable(tdma->dma_clk);
 	if (ret < 0) {
 		dev_err(dev, "clk_enable failed: %d\n", ret);
 		return ret;
-- 
2.24.0

Re: [PATCH v4 10/14] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

[Jon Hunter]

On 12/01/2020 17:30, Dmitry Osipenko wrote:
> It's a bit impractical to enable hardware's clock at the time of DMA
> channel's allocation because most of DMA client drivers allocate DMA
> channel at the time of the driver's probing and thus DMA clock is kept
> always-enabled in practice, defeating the whole purpose of runtime PM.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/dma/tegra20-apb-dma.c | 43 ++++++++++++++++++++++-------------
>  1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index cc4a9ca20780..b9d8e57eaf54 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -436,6 +436,8 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc)
>  		tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
>  	}
>  	tdc->busy = false;
> +
> +	pm_runtime_put(tdc->tdma->dev);

Is this the right place to call put? Seems that in terminate_all resume
is called after stop which will access the registers.

>  }
>  
>  static void tegra_dma_start(struct tegra_dma_channel *tdc,
> @@ -500,18 +502,25 @@ static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc,
>  	tegra_dma_resume(tdc);
>  }
>  
> -static void tdc_start_head_req(struct tegra_dma_channel *tdc)
> +static bool tdc_start_head_req(struct tegra_dma_channel *tdc)
>  {
>  	struct tegra_dma_sg_req *sg_req;
> +	int err;
>  
>  	if (list_empty(&tdc->pending_sg_req))
> -		return;
> +		return false;
> +
> +	err = pm_runtime_get_sync(tdc->tdma->dev);
> +	if (WARN_ON_ONCE(err < 0))
> +		return false;
>  
>  	sg_req = list_first_entry(&tdc->pending_sg_req, typeof(*sg_req), node);
>  	tegra_dma_start(tdc, sg_req);
>  	sg_req->configured = true;
>  	sg_req->words_xferred = 0;
>  	tdc->busy = true;
> +
> +	return true;
>  }
>  
>  static void tdc_configure_next_head_desc(struct tegra_dma_channel *tdc)
> @@ -615,6 +624,8 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
>  	}
>  	list_add_tail(&sgreq->node, &tdc->free_sg_req);
>  
> +	pm_runtime_put(tdc->tdma->dev);
> +
>  	/* Do not start DMA if it is going to be terminate */
>  	if (to_terminate || list_empty(&tdc->pending_sg_req))
>  		return;
> @@ -730,9 +741,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
>  		dev_err(tdc2dev(tdc), "No DMA request\n");
>  		goto end;
>  	}
> -	if (!tdc->busy) {
> -		tdc_start_head_req(tdc);
> -
> +	if (!tdc->busy && tdc_start_head_req(tdc)) {
>  		/* Continuous single mode: Configure next req */
>  		if (tdc->cyclic) {
>  			/*
> @@ -1280,22 +1289,15 @@ tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr,
>  static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>  {
>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> -	struct tegra_dma *tdma = tdc->tdma;
> -	int ret;
>  
>  	dma_cookie_init(&tdc->dma_chan);
>  
> -	ret = pm_runtime_get_sync(tdma->dev);
> -	if (ret < 0)
> -		return ret;
> -
>  	return 0;
>  }
>  
>  static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>  {
>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> -	struct tegra_dma *tdma = tdc->tdma;
>  	struct tegra_dma_desc *dma_desc;
>  	struct tegra_dma_sg_req *sg_req;
>  	struct list_head dma_desc_list;
> @@ -1328,7 +1330,6 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>  		list_del(&sg_req->node);
>  		kfree(sg_req);
>  	}
> -	pm_runtime_put(tdma->dev);
>  
>  	tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
>  }
> @@ -1428,11 +1429,16 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  
>  	spin_lock_init(&tdma->global_lock);
>  
> +	ret = clk_prepare(tdma->dma_clk);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_irq_safe(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
>  	if (!pm_runtime_enabled(&pdev->dev)) {
>  		ret = tegra_dma_runtime_resume(&pdev->dev);
>  		if (ret)
> -			return ret;
> +			goto err_clk_unprepare;
>  	} else {
>  		ret = pm_runtime_get_sync(&pdev->dev);

There is a get here but I don't see a put in probe.

Jon

-- 
nvpublic

Re: [PATCH v4 10/14] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

[Dmitry Osipenko]

15.01.2020 13:08, Jon Hunter пишет:
> 
> 
> On 12/01/2020 17:30, Dmitry Osipenko wrote:
>> It's a bit impractical to enable hardware's clock at the time of DMA
>> channel's allocation because most of DMA client drivers allocate DMA
>> channel at the time of the driver's probing and thus DMA clock is kept
>> always-enabled in practice, defeating the whole purpose of runtime PM.
>>
>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>  drivers/dma/tegra20-apb-dma.c | 43 ++++++++++++++++++++++-------------
>>  1 file changed, 27 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>> index cc4a9ca20780..b9d8e57eaf54 100644
>> --- a/drivers/dma/tegra20-apb-dma.c
>> +++ b/drivers/dma/tegra20-apb-dma.c
>> @@ -436,6 +436,8 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc)
>>  		tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status);
>>  	}
>>  	tdc->busy = false;
>> +
>> +	pm_runtime_put(tdc->tdma->dev);
> 
> Is this the right place to call put? Seems that in terminate_all resume
> is called after stop which will access the registers.

Indeed, there is a problem here. Looks like resume/pause should take
get/put the RPM. I'll correct it in v5, thanks.

>>  }
>>  
>>  static void tegra_dma_start(struct tegra_dma_channel *tdc,
>> @@ -500,18 +502,25 @@ static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc,
>>  	tegra_dma_resume(tdc);
>>  }
>>  
>> -static void tdc_start_head_req(struct tegra_dma_channel *tdc)
>> +static bool tdc_start_head_req(struct tegra_dma_channel *tdc)
>>  {
>>  	struct tegra_dma_sg_req *sg_req;
>> +	int err;
>>  
>>  	if (list_empty(&tdc->pending_sg_req))
>> -		return;
>> +		return false;
>> +
>> +	err = pm_runtime_get_sync(tdc->tdma->dev);
>> +	if (WARN_ON_ONCE(err < 0))
>> +		return false;
>>  
>>  	sg_req = list_first_entry(&tdc->pending_sg_req, typeof(*sg_req), node);
>>  	tegra_dma_start(tdc, sg_req);
>>  	sg_req->configured = true;
>>  	sg_req->words_xferred = 0;
>>  	tdc->busy = true;
>> +
>> +	return true;
>>  }
>>  
>>  static void tdc_configure_next_head_desc(struct tegra_dma_channel *tdc)
>> @@ -615,6 +624,8 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
>>  	}
>>  	list_add_tail(&sgreq->node, &tdc->free_sg_req);
>>  
>> +	pm_runtime_put(tdc->tdma->dev);
>> +
>>  	/* Do not start DMA if it is going to be terminate */
>>  	if (to_terminate || list_empty(&tdc->pending_sg_req))
>>  		return;
>> @@ -730,9 +741,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
>>  		dev_err(tdc2dev(tdc), "No DMA request\n");
>>  		goto end;
>>  	}
>> -	if (!tdc->busy) {
>> -		tdc_start_head_req(tdc);
>> -
>> +	if (!tdc->busy && tdc_start_head_req(tdc)) {
>>  		/* Continuous single mode: Configure next req */
>>  		if (tdc->cyclic) {
>>  			/*
>> @@ -1280,22 +1289,15 @@ tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr,
>>  static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>>  {
>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>> -	struct tegra_dma *tdma = tdc->tdma;
>> -	int ret;
>>  
>>  	dma_cookie_init(&tdc->dma_chan);
>>  
>> -	ret = pm_runtime_get_sync(tdma->dev);
>> -	if (ret < 0)
>> -		return ret;
>> -
>>  	return 0;
>>  }
>>  
>>  static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>>  {
>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>> -	struct tegra_dma *tdma = tdc->tdma;
>>  	struct tegra_dma_desc *dma_desc;
>>  	struct tegra_dma_sg_req *sg_req;
>>  	struct list_head dma_desc_list;
>> @@ -1328,7 +1330,6 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>>  		list_del(&sg_req->node);
>>  		kfree(sg_req);
>>  	}
>> -	pm_runtime_put(tdma->dev);
>>  
>>  	tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
>>  }
>> @@ -1428,11 +1429,16 @@ static int tegra_dma_probe(struct platform_device *pdev)
>>  
>>  	spin_lock_init(&tdma->global_lock);
>>  
>> +	ret = clk_prepare(tdma->dma_clk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	pm_runtime_irq_safe(&pdev->dev);
>>  	pm_runtime_enable(&pdev->dev);
>>  	if (!pm_runtime_enabled(&pdev->dev)) {
>>  		ret = tegra_dma_runtime_resume(&pdev->dev);
>>  		if (ret)
>> -			return ret;
>> +			goto err_clk_unprepare;
>>  	} else {
>>  		ret = pm_runtime_get_sync(&pdev->dev);
> 
> There is a get here but I don't see a put in probe.

Please see:
https://elixir.bootlin.com/linux/v5.5-rc6/source/drivers/dma/tegra20-apb-dma.c#L1445

[PATCH v4 11/14] dmaengine: tegra-apb: Clean up suspend-resume

[Dmitry Osipenko]

It is enough to check whether hardware is busy on suspend and to reset
it across of suspend-resume because channel's configuration is fully
re-programmed on each DMA transaction anyways and because save-restore
of an active channel won't end up well without pausing transfer prior to
saving of the state (note that all channels shall be idling at the time of
suspend, so save-restore is not needed at all).

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/dma/tegra20-apb-dma.c | 131 +++++++++++++++++-----------------
 1 file changed, 67 insertions(+), 64 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index b9d8e57eaf54..398a0e1d6506 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1392,6 +1392,36 @@ static const struct tegra_dma_chip_data tegra148_dma_chip_data = {
 	.support_separate_wcount_reg = true,
 };
 
+static int tegra_dma_init_hw(struct tegra_dma *tdma)
+{
+	int err;
+
+	err = reset_control_assert(tdma->rst);
+	if (err) {
+		dev_err(tdma->dev, "failed to assert reset: %d\n", err);
+		return err;
+	}
+
+	err = clk_enable(tdma->dma_clk);
+	if (err) {
+		dev_err(tdma->dev, "failed to enable clk: %d\n", err);
+		return err;
+	}
+
+	/* reset DMA controller */
+	udelay(2);
+	reset_control_deassert(tdma->rst);
+
+	/* enable global DMA registers */
+	tdma_write(tdma, TEGRA_APBDMA_GENERAL, TEGRA_APBDMA_GENERAL_ENABLE);
+	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
+	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFF);
+
+	clk_disable(tdma->dma_clk);
+
+	return 0;
+}
+
 static int tegra_dma_probe(struct platform_device *pdev)
 {
 	const struct tegra_dma_chip_data *cdata;
@@ -1433,30 +1463,18 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = tegra_dma_init_hw(tdma);
+	if (ret)
+		goto err_clk_unprepare;
+
 	pm_runtime_irq_safe(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 	if (!pm_runtime_enabled(&pdev->dev)) {
 		ret = tegra_dma_runtime_resume(&pdev->dev);
 		if (ret)
 			goto err_clk_unprepare;
-	} else {
-		ret = pm_runtime_get_sync(&pdev->dev);
-		if (ret < 0)
-			goto err_pm_disable;
 	}
 
-	/* Reset DMA controller */
-	reset_control_assert(tdma->rst);
-	udelay(2);
-	reset_control_deassert(tdma->rst);
-
-	/* Enable global DMA registers */
-	tdma_write(tdma, TEGRA_APBDMA_GENERAL, TEGRA_APBDMA_GENERAL_ENABLE);
-	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
-	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFFul);
-
-	pm_runtime_put(&pdev->dev);
-
 	INIT_LIST_HEAD(&tdma->dma_dev.channels);
 	for (i = 0; i < cdata->nr_channels; i++) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
@@ -1583,26 +1601,6 @@ static int tegra_dma_remove(struct platform_device *pdev)
 static int tegra_dma_runtime_suspend(struct device *dev)
 {
 	struct tegra_dma *tdma = dev_get_drvdata(dev);
-	unsigned int i;
-
-	tdma->reg_gen = tdma_read(tdma, TEGRA_APBDMA_GENERAL);
-	for (i = 0; i < tdma->chip_data->nr_channels; i++) {
-		struct tegra_dma_channel *tdc = &tdma->channels[i];
-		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
-
-		/* Only save the state of DMA channels that are in use */
-		if (!tdc->config_init)
-			continue;
-
-		ch_reg->csr = tdc_read(tdc, TEGRA_APBDMA_CHAN_CSR);
-		ch_reg->ahb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
-		ch_reg->apb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBPTR);
-		ch_reg->ahb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBSEQ);
-		ch_reg->apb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBSEQ);
-		if (tdma->chip_data->support_separate_wcount_reg)
-			ch_reg->wcount = tdc_read(tdc,
-						  TEGRA_APBDMA_CHAN_WCOUNT);
-	}
 
 	clk_disable(tdma->dma_clk);
 
@@ -1612,46 +1610,51 @@ static int tegra_dma_runtime_suspend(struct device *dev)
 static int tegra_dma_runtime_resume(struct device *dev)
 {
 	struct tegra_dma *tdma = dev_get_drvdata(dev);
-	unsigned int i;
-	int ret;
 
-	ret = clk_enable(tdma->dma_clk);
-	if (ret < 0) {
-		dev_err(dev, "clk_enable failed: %d\n", ret);
-		return ret;
-	}
+	return clk_enable(tdma->dma_clk);
+}
 
-	tdma_write(tdma, TEGRA_APBDMA_GENERAL, tdma->reg_gen);
-	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
-	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFFul);
+static int __maybe_unused tegra_dma_dev_suspend(struct device *dev)
+{
+	struct tegra_dma *tdma = dev_get_drvdata(dev);
+	unsigned long flags;
+	unsigned int i;
+	bool busy;
 
 	for (i = 0; i < tdma->chip_data->nr_channels; i++) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
-		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
-
-		/* Only restore the state of DMA channels that are in use */
-		if (!tdc->config_init)
-			continue;
-
-		if (tdma->chip_data->support_separate_wcount_reg)
-			tdc_write(tdc, TEGRA_APBDMA_CHAN_WCOUNT,
-				  ch_reg->wcount);
-		tdc_write(tdc, TEGRA_APBDMA_CHAN_APBSEQ, ch_reg->apb_seq);
-		tdc_write(tdc, TEGRA_APBDMA_CHAN_APBPTR, ch_reg->apb_ptr);
-		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBSEQ, ch_reg->ahb_seq);
-		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBPTR, ch_reg->ahb_ptr);
-		tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR,
-			  ch_reg->csr & ~TEGRA_APBDMA_CSR_ENB);
+
+		spin_lock_irqsave(&tdc->lock, flags);
+		busy = tdc->busy;
+		spin_unlock_irqrestore(&tdc->lock, flags);
+
+		if (busy) {
+			dev_err(tdma->dev, "channel %u busy\n", i);
+			return -EBUSY;
+		}
+
+		tasklet_kill(&tdc->tasklet);
 	}
 
-	return 0;
+	return pm_runtime_force_suspend(dev);
+}
+
+static int __maybe_unused tegra_dma_dev_resume(struct device *dev)
+{
+	struct tegra_dma *tdma = dev_get_drvdata(dev);
+	int err;
+
+	err = tegra_dma_init_hw(tdma);
+	if (err)
+		return err;
+
+	return pm_runtime_force_resume(dev);
 }
 
 static const struct dev_pm_ops tegra_dma_dev_pm_ops = {
 	SET_RUNTIME_PM_OPS(tegra_dma_runtime_suspend, tegra_dma_runtime_resume,
 			   NULL)
-	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
-				pm_runtime_force_resume)
+	SET_SYSTEM_SLEEP_PM_OPS(tegra_dma_dev_suspend, tegra_dma_dev_resume)
 };
 
 static const struct of_device_id tegra_dma_of_match[] = {
-- 
2.24.0

Re: [PATCH v4 11/14] dmaengine: tegra-apb: Clean up suspend-resume

[Dmitry Osipenko]

12.01.2020 20:30, Dmitry Osipenko пишет:
> It is enough to check whether hardware is busy on suspend and to reset
> it across of suspend-resume because channel's configuration is fully
> re-programmed on each DMA transaction anyways and because save-restore
> of an active channel won't end up well without pausing transfer prior to
> saving of the state (note that all channels shall be idling at the time of
> suspend, so save-restore is not needed at all).
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/dma/tegra20-apb-dma.c | 131 +++++++++++++++++-----------------
>  1 file changed, 67 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index b9d8e57eaf54..398a0e1d6506 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -1392,6 +1392,36 @@ static const struct tegra_dma_chip_data tegra148_dma_chip_data = {
>  	.support_separate_wcount_reg = true,
>  };
>  
> +static int tegra_dma_init_hw(struct tegra_dma *tdma)
> +{
> +	int err;
> +
> +	err = reset_control_assert(tdma->rst);
> +	if (err) {
> +		dev_err(tdma->dev, "failed to assert reset: %d\n", err);
> +		return err;
> +	}
> +
> +	err = clk_enable(tdma->dma_clk);
> +	if (err) {
> +		dev_err(tdma->dev, "failed to enable clk: %d\n", err);
> +		return err;
> +	}
> +
> +	/* reset DMA controller */
> +	udelay(2);
> +	reset_control_deassert(tdma->rst);
> +
> +	/* enable global DMA registers */
> +	tdma_write(tdma, TEGRA_APBDMA_GENERAL, TEGRA_APBDMA_GENERAL_ENABLE);
> +	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
> +	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFF);
> +
> +	clk_disable(tdma->dma_clk);
> +
> +	return 0;
> +}
> +
>  static int tegra_dma_probe(struct platform_device *pdev)
>  {
>  	const struct tegra_dma_chip_data *cdata;
> @@ -1433,30 +1463,18 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = tegra_dma_init_hw(tdma);
> +	if (ret)
> +		goto err_clk_unprepare;
> +
>  	pm_runtime_irq_safe(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
>  	if (!pm_runtime_enabled(&pdev->dev)) {
>  		ret = tegra_dma_runtime_resume(&pdev->dev);
>  		if (ret)
>  			goto err_clk_unprepare;

Jon, but isn't the RPM mandatory for all Tegra SoCs now and thus
guaranteed to be enabled? Maybe we should start to remove handling the
case of unavailable RPM from all Tegra drivers?

> -	} else {
> -		ret = pm_runtime_get_sync(&pdev->dev);
> -		if (ret < 0)
> -			goto err_pm_disable;
>  	}
>  
> -	/* Reset DMA controller */
> -	reset_control_assert(tdma->rst);
> -	udelay(2);
> -	reset_control_deassert(tdma->rst);
> -
> -	/* Enable global DMA registers */
> -	tdma_write(tdma, TEGRA_APBDMA_GENERAL, TEGRA_APBDMA_GENERAL_ENABLE);
> -	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
> -	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFFul);
> -
> -	pm_runtime_put(&pdev->dev);
> -
>  	INIT_LIST_HEAD(&tdma->dma_dev.channels);
>  	for (i = 0; i < cdata->nr_channels; i++) {
>  		struct tegra_dma_channel *tdc = &tdma->channels[i];
> @@ -1583,26 +1601,6 @@ static int tegra_dma_remove(struct platform_device *pdev)
>  static int tegra_dma_runtime_suspend(struct device *dev)
>  {
>  	struct tegra_dma *tdma = dev_get_drvdata(dev);
> -	unsigned int i;
> -
> -	tdma->reg_gen = tdma_read(tdma, TEGRA_APBDMA_GENERAL);
> -	for (i = 0; i < tdma->chip_data->nr_channels; i++) {
> -		struct tegra_dma_channel *tdc = &tdma->channels[i];
> -		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
> -
> -		/* Only save the state of DMA channels that are in use */
> -		if (!tdc->config_init)
> -			continue;
> -
> -		ch_reg->csr = tdc_read(tdc, TEGRA_APBDMA_CHAN_CSR);
> -		ch_reg->ahb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
> -		ch_reg->apb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBPTR);
> -		ch_reg->ahb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBSEQ);
> -		ch_reg->apb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBSEQ);
> -		if (tdma->chip_data->support_separate_wcount_reg)
> -			ch_reg->wcount = tdc_read(tdc,
> -						  TEGRA_APBDMA_CHAN_WCOUNT);
> -	}
>  
>  	clk_disable(tdma->dma_clk);
>  
> @@ -1612,46 +1610,51 @@ static int tegra_dma_runtime_suspend(struct device *dev)
>  static int tegra_dma_runtime_resume(struct device *dev)
>  {
>  	struct tegra_dma *tdma = dev_get_drvdata(dev);
> -	unsigned int i;
> -	int ret;
>  
> -	ret = clk_enable(tdma->dma_clk);
> -	if (ret < 0) {
> -		dev_err(dev, "clk_enable failed: %d\n", ret);
> -		return ret;
> -	}
> +	return clk_enable(tdma->dma_clk);
> +}
>  
> -	tdma_write(tdma, TEGRA_APBDMA_GENERAL, tdma->reg_gen);
> -	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
> -	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFFul);
> +static int __maybe_unused tegra_dma_dev_suspend(struct device *dev)
> +{
> +	struct tegra_dma *tdma = dev_get_drvdata(dev);
> +	unsigned long flags;
> +	unsigned int i;
> +	bool busy;
>  
>  	for (i = 0; i < tdma->chip_data->nr_channels; i++) {
>  		struct tegra_dma_channel *tdc = &tdma->channels[i];
> -		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
> -
> -		/* Only restore the state of DMA channels that are in use */
> -		if (!tdc->config_init)
> -			continue;
> -
> -		if (tdma->chip_data->support_separate_wcount_reg)
> -			tdc_write(tdc, TEGRA_APBDMA_CHAN_WCOUNT,
> -				  ch_reg->wcount);
> -		tdc_write(tdc, TEGRA_APBDMA_CHAN_APBSEQ, ch_reg->apb_seq);
> -		tdc_write(tdc, TEGRA_APBDMA_CHAN_APBPTR, ch_reg->apb_ptr);
> -		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBSEQ, ch_reg->ahb_seq);
> -		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBPTR, ch_reg->ahb_ptr);
> -		tdc_write(tdc, TEGRA_APBDMA_CHAN_CSR,
> -			  ch_reg->csr & ~TEGRA_APBDMA_CSR_ENB);
> +
> +		spin_lock_irqsave(&tdc->lock, flags);
> +		busy = tdc->busy;
> +		spin_unlock_irqrestore(&tdc->lock, flags);
> +
> +		if (busy) {
> +			dev_err(tdma->dev, "channel %u busy\n", i);
> +			return -EBUSY;
> +		}
> +
> +		tasklet_kill(&tdc->tasklet);

I realized that it will be more robust to kill the tasklet before
checking the busy state because technically tasklet could issue new DMA
transfer, will correct it in v5.

>  	}
>  
> -	return 0;
> +	return pm_runtime_force_suspend(dev);
> +}
> +
> +static int __maybe_unused tegra_dma_dev_resume(struct device *dev)
> +{
> +	struct tegra_dma *tdma = dev_get_drvdata(dev);
> +	int err;
> +
> +	err = tegra_dma_init_hw(tdma);
> +	if (err)
> +		return err;
> +
> +	return pm_runtime_force_resume(dev);
>  }
>  
>  static const struct dev_pm_ops tegra_dma_dev_pm_ops = {
>  	SET_RUNTIME_PM_OPS(tegra_dma_runtime_suspend, tegra_dma_runtime_resume,
>  			   NULL)
> -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> -				pm_runtime_force_resume)
> +	SET_SYSTEM_SLEEP_PM_OPS(tegra_dma_dev_suspend, tegra_dma_dev_resume)
>  };
>  
>  static const struct of_device_id tegra_dma_of_match[] = {
> 

Re: [PATCH v4 11/14] dmaengine: tegra-apb: Clean up suspend-resume

[Jon Hunter]

On 21/01/2020 21:23, Dmitry Osipenko wrote:
> 12.01.2020 20:30, Dmitry Osipenko пишет:
>> It is enough to check whether hardware is busy on suspend and to reset
>> it across of suspend-resume because channel's configuration is fully
>> re-programmed on each DMA transaction anyways and because save-restore
>> of an active channel won't end up well without pausing transfer prior to
>> saving of the state (note that all channels shall be idling at the time of
>> suspend, so save-restore is not needed at all).
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/dma/tegra20-apb-dma.c | 131 +++++++++++++++++-----------------
>>  1 file changed, 67 insertions(+), 64 deletions(-)
>>
>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>> index b9d8e57eaf54..398a0e1d6506 100644
>> --- a/drivers/dma/tegra20-apb-dma.c
>> +++ b/drivers/dma/tegra20-apb-dma.c
>> @@ -1392,6 +1392,36 @@ static const struct tegra_dma_chip_data tegra148_dma_chip_data = {
>>  	.support_separate_wcount_reg = true,
>>  };
>>  
>> +static int tegra_dma_init_hw(struct tegra_dma *tdma)
>> +{
>> +	int err;
>> +
>> +	err = reset_control_assert(tdma->rst);
>> +	if (err) {
>> +		dev_err(tdma->dev, "failed to assert reset: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	err = clk_enable(tdma->dma_clk);
>> +	if (err) {
>> +		dev_err(tdma->dev, "failed to enable clk: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	/* reset DMA controller */
>> +	udelay(2);
>> +	reset_control_deassert(tdma->rst);
>> +
>> +	/* enable global DMA registers */
>> +	tdma_write(tdma, TEGRA_APBDMA_GENERAL, TEGRA_APBDMA_GENERAL_ENABLE);
>> +	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
>> +	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFF);
>> +
>> +	clk_disable(tdma->dma_clk);
>> +
>> +	return 0;
>> +}
>> +
>>  static int tegra_dma_probe(struct platform_device *pdev)
>>  {
>>  	const struct tegra_dma_chip_data *cdata;
>> @@ -1433,30 +1463,18 @@ static int tegra_dma_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return ret;
>>  
>> +	ret = tegra_dma_init_hw(tdma);
>> +	if (ret)
>> +		goto err_clk_unprepare;
>> +
>>  	pm_runtime_irq_safe(&pdev->dev);
>>  	pm_runtime_enable(&pdev->dev);
>>  	if (!pm_runtime_enabled(&pdev->dev)) {
>>  		ret = tegra_dma_runtime_resume(&pdev->dev);
>>  		if (ret)
>>  			goto err_clk_unprepare;
> 
> Jon, but isn't the RPM mandatory for all Tegra SoCs now and thus
> guaranteed to be enabled? Maybe we should start to remove handling the
> case of unavailable RPM from all Tegra drivers?

Yes that's true, even ARCH_TEGRA selects PM now

Cheers
Jon

-- 
nvpublic

Re: [PATCH v4 11/14] dmaengine: tegra-apb: Clean up suspend-resume

[Dmitry Osipenko]

28.01.2020 17:10, Jon Hunter пишет:
> 
> On 21/01/2020 21:23, Dmitry Osipenko wrote:
>> 12.01.2020 20:30, Dmitry Osipenko пишет:
>>> It is enough to check whether hardware is busy on suspend and to reset
>>> it across of suspend-resume because channel's configuration is fully
>>> re-programmed on each DMA transaction anyways and because save-restore
>>> of an active channel won't end up well without pausing transfer prior to
>>> saving of the state (note that all channels shall be idling at the time of
>>> suspend, so save-restore is not needed at all).
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>> ---
>>>  drivers/dma/tegra20-apb-dma.c | 131 +++++++++++++++++-----------------
>>>  1 file changed, 67 insertions(+), 64 deletions(-)
>>>
>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>>> index b9d8e57eaf54..398a0e1d6506 100644
>>> --- a/drivers/dma/tegra20-apb-dma.c
>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>> @@ -1392,6 +1392,36 @@ static const struct tegra_dma_chip_data tegra148_dma_chip_data = {
>>>  	.support_separate_wcount_reg = true,
>>>  };
>>>  
>>> +static int tegra_dma_init_hw(struct tegra_dma *tdma)
>>> +{
>>> +	int err;
>>> +
>>> +	err = reset_control_assert(tdma->rst);
>>> +	if (err) {
>>> +		dev_err(tdma->dev, "failed to assert reset: %d\n", err);
>>> +		return err;
>>> +	}
>>> +
>>> +	err = clk_enable(tdma->dma_clk);
>>> +	if (err) {
>>> +		dev_err(tdma->dev, "failed to enable clk: %d\n", err);
>>> +		return err;
>>> +	}
>>> +
>>> +	/* reset DMA controller */
>>> +	udelay(2);
>>> +	reset_control_deassert(tdma->rst);
>>> +
>>> +	/* enable global DMA registers */
>>> +	tdma_write(tdma, TEGRA_APBDMA_GENERAL, TEGRA_APBDMA_GENERAL_ENABLE);
>>> +	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
>>> +	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFF);
>>> +
>>> +	clk_disable(tdma->dma_clk);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static int tegra_dma_probe(struct platform_device *pdev)
>>>  {
>>>  	const struct tegra_dma_chip_data *cdata;
>>> @@ -1433,30 +1463,18 @@ static int tegra_dma_probe(struct platform_device *pdev)
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> +	ret = tegra_dma_init_hw(tdma);
>>> +	if (ret)
>>> +		goto err_clk_unprepare;
>>> +
>>>  	pm_runtime_irq_safe(&pdev->dev);
>>>  	pm_runtime_enable(&pdev->dev);
>>>  	if (!pm_runtime_enabled(&pdev->dev)) {
>>>  		ret = tegra_dma_runtime_resume(&pdev->dev);
>>>  		if (ret)
>>>  			goto err_clk_unprepare;
>>
>> Jon, but isn't the RPM mandatory for all Tegra SoCs now and thus
>> guaranteed to be enabled? Maybe we should start to remove handling the
>> case of unavailable RPM from all Tegra drivers?
> 
> Yes that's true, even ARCH_TEGRA selects PM now

I already sent out v5 with the !RPM handling removed from the code,
please take a look at it.

[PATCH v4 12/14] dmaengine: tegra-apb: Add missing of_dma_controller_free

[Dmitry Osipenko]

The DMA controller shall be released on driver's removal.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/dma/tegra20-apb-dma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 398a0e1d6506..fbbb6a60901e 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1586,6 +1586,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
 {
 	struct tegra_dma *tdma = platform_get_drvdata(pdev);
 
+	of_dma_controller_free(pdev->dev.of_node);
 	dma_async_device_unregister(&tdma->dma_dev);
 
 	if (!pm_runtime_enabled(&pdev->dev))
-- 
2.24.0

Re: [PATCH v4 12/14] dmaengine: tegra-apb: Add missing of_dma_controller_free

[Jon Hunter]

On 12/01/2020 17:30, Dmitry Osipenko wrote:
> The DMA controller shall be released on driver's removal.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/dma/tegra20-apb-dma.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 398a0e1d6506..fbbb6a60901e 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -1586,6 +1586,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
>  {
>  	struct tegra_dma *tdma = platform_get_drvdata(pdev);
>  
> +	of_dma_controller_free(pdev->dev.of_node);
>  	dma_async_device_unregister(&tdma->dma_dev);
>  
>  	if (!pm_runtime_enabled(&pdev->dev))
> 

Acked-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Cheers
Jon

-- 
nvpublic

[PATCH v4 13/14] dmaengine: tegra-apb: Allow to compile as a loadable kernel module

[Dmitry Osipenko]

The driver's removal was fixed by a recent commit and module load/unload
is working well now, tested on Tegra30.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/dma/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 6fa1eba9d477..9f43e2cae8b4 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -586,7 +586,7 @@ config TXX9_DMAC
 	  integrated in chips such as the Toshiba TX4927/38/39.
 
 config TEGRA20_APB_DMA
-	bool "NVIDIA Tegra20 APB DMA support"
+	tristate "NVIDIA Tegra20 APB DMA support"
 	depends on ARCH_TEGRA
 	select DMA_ENGINE
 	help
-- 
2.24.0

Re: [PATCH v4 13/14] dmaengine: tegra-apb: Allow to compile as a loadable kernel module

[Jon Hunter]

On 12/01/2020 17:30, Dmitry Osipenko wrote:
> The driver's removal was fixed by a recent commit and module load/unload
> is working well now, tested on Tegra30.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/dma/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 6fa1eba9d477..9f43e2cae8b4 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -586,7 +586,7 @@ config TXX9_DMAC
>  	  integrated in chips such as the Toshiba TX4927/38/39.
>  
>  config TEGRA20_APB_DMA
> -	bool "NVIDIA Tegra20 APB DMA support"
> +	tristate "NVIDIA Tegra20 APB DMA support"
>  	depends on ARCH_TEGRA
>  	select DMA_ENGINE
>  	help

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon


-- 
nvpublic

[PATCH v4 14/14] dmaengine: tegra-apb: Remove MODULE_ALIAS

[Dmitry Osipenko]

Tegra APB DMA driver is an Open Firmware driver and thus it uses OF alias
naming scheme which overrides MODULE_ALIAS, meaning that MODULE_ALIAS does
nothing and could be removed safely.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/dma/tegra20-apb-dma.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index fbbb6a60901e..0a45dd77618c 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1688,7 +1688,6 @@ static struct platform_driver tegra_dmac_driver = {
 
 module_platform_driver(tegra_dmac_driver);
 
-MODULE_ALIAS("platform:tegra20-apbdma");
 MODULE_DESCRIPTION("NVIDIA Tegra APB DMA Controller driver");
 MODULE_AUTHOR("Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
 MODULE_LICENSE("GPL v2");
-- 
2.24.0

Re: [PATCH v4 14/14] dmaengine: tegra-apb: Remove MODULE_ALIAS

[Jon Hunter]

On 12/01/2020 17:30, Dmitry Osipenko wrote:
> Tegra APB DMA driver is an Open Firmware driver and thus it uses OF alias
> naming scheme which overrides MODULE_ALIAS, meaning that MODULE_ALIAS does
> nothing and could be removed safely.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/dma/tegra20-apb-dma.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index fbbb6a60901e..0a45dd77618c 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -1688,7 +1688,6 @@ static struct platform_driver tegra_dmac_driver = {
>  
>  module_platform_driver(tegra_dmac_driver);
>  
> -MODULE_ALIAS("platform:tegra20-apbdma");
>  MODULE_DESCRIPTION("NVIDIA Tegra APB DMA Controller driver");
>  MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
>  MODULE_LICENSE("GPL v2");

Acked-by: Jon Hunter <jonathanh@nvidia.com>

Cheers
Jon

-- 
nvpublic