Re: [PATCH 1/3] ata: sata_dwc_460ex: use "dmas" DT property to find dma channel

["Måns Rullgård" <mans@mansr.com>]

[-- Attachment #1: Type: text/plain, Size: 4373 bytes --]

Julian Margetson <runaway@candw.ms> writes:

> On 12/21/2015 9:24 AM, Måns Rullgård wrote:
>> Julian Margetson <runaway@candw.ms> writes:
>>
>>>>>> P.S. Anyway we have to ask Julian to try the kernel with
>>>>>> 8b3444852a2b58129 reverted.
>>>>>>
>>>>> git revert 8b3444852a2b58129
>>>>> error: could not revert 8b34448... sata_dwc_460ex: move to generic DMA driver
>>>>> hint: after resolving the conflicts, mark the corrected paths
>>>>> hint: with 'git add <paths>' or 'git rm <paths>'
>>>>> hint: and commit the result with 'git commit'
>>>> Yeah, that won't work since there are numerous changes afterward.  Just
>>>> revert the entire file back to 4.0 like this:
>>>>
>>>> $ git checkout v4.0 drivers/ata/sata_dwc_460ex.c
>>>>
>>>   CC [M]  drivers/ata/sata_dwc_460ex.o
>>> drivers/ata/sata_dwc_460ex.c:467:36: error: macro
>>> "dma_request_channel" requires 3 arguments, but only 1 given
>>>   static int dma_request_channel(void)
>>>                                      ^
>>> drivers/ata/sata_dwc_460ex.c:468:1: error: expected ‘=’, ‘,’,
>>> ‘;’, ‘asm’ or ‘__attribute__’ before ‘{’ token
>>>   {
>>>   ^
>>> drivers/ata/sata_dwc_460ex.c: In function ‘dma_dwc_xfer_setup’:
>>> drivers/ata/sata_dwc_460ex.c:758:31: error: macro
>>> "dma_request_channel" requires 3 arguments, but only 1 given
>>>    dma_ch = dma_request_channel();
>>>                                 ^
>>> drivers/ata/sata_dwc_460ex.c:758:11: error: ‘dma_request_channel’
>>> undeclared (first use in this function)
>>>    dma_ch = dma_request_channel();
>>>             ^
>>> drivers/ata/sata_dwc_460ex.c:758:11: note: each undeclared identifier
>>> is reported only once for each function it appears in
>>> drivers/ata/sata_dwc_460ex.c: In function ‘sata_dwc_dma_filter’:
>>> drivers/ata/sata_dwc_460ex.c:1282:35: error: ‘struct
>>> sata_dwc_device_port’ has no member named ‘dws’
>>>    struct dw_dma_slave *dws = hsdevp->dws;
>>>                                     ^
>>> drivers/ata/sata_dwc_460ex.c: In function ‘sata_dwc_port_start’:
>>> drivers/ata/sata_dwc_460ex.c:1325:17: warning: unused variable
>>> ‘mask’ [-Wunused-variable]
>>>    dma_cap_mask_t mask;
>>>                   ^
>>> drivers/ata/sata_dwc_460ex.c: At top level:
>>> drivers/ata/sata_dwc_460ex.c:345:28: warning: ‘sata_dwc_dma_dws’
>>> defined but not used [-Wunused-variable]
>>>   static struct dw_dma_slave sata_dwc_dma_dws = {
>>>                              ^
>>> drivers/ata/sata_dwc_460ex.c:1279:13: warning:
>>> ‘sata_dwc_dma_filter’ defined but not used [-Wunused-function]
>>>   static bool sata_dwc_dma_filter(struct dma_chan *chan, void *param)
>>>               ^
>> Those messages do not match the contents of the file from v4.0.
>> For your convenience, here's the file as it should be.
>>
>> $ sha1sum drivers/ata/sata_dwc_460ex.c
>> 0f54dfa3a91591101f5de434c3a631a5cd20ff1a  drivers/ata/sata_dwc_460ex.c
>
> [   16.119186] BUG: spinlock recursion on CPU#0, kworker/u2:1/85
> [   16.124935]  lock: 0xedd2f910, .magic: dead4ead, .owner: kworker/u2:1/85, .owner_cpu: 0
> [   16.132947] CPU: 0 PID: 85 Comm: kworker/u2:1 Not tainted 4.4.0-rc5-Sam460ex-dirty #3
> [   16.140793] Workqueue: events_unbound async_run_entry_fn
> [   16.146119] Call Trace:
> [   16.148582] [ee3cf8c0] [c0049238] do_raw_spin_lock+0x4c/0x100 (unreliable)
> [   16.155491] [ee3cf8e0] [c068af98] _raw_spin_lock_irqsave+0x2c/0x38
> [   16.161721] [ee3cf8f0] [f6a0fd98] sata_dwc_exec_command_by_tag.constprop.9+0x80/0xb4 [sata_dwc_460ex]
> [   16.170954] [ee3cf920] [f6a108c0] sata_dwc_qc_issue+0x6a4/0x6c4 [sata_dwc_460ex]
> [   16.178380] [ee3cf9d0] [c043bdf8] ata_qc_issue+0x338/0x3a0
> [   16.183883] [ee3cfa00] [c0440c84] ata_scsi_translate+0xf4/0x150
> [   16.189813] [ee3cfa20] [c0444080] ata_scsi_queuecmd+0x1e8/0x238
> [   16.195750] [ee3cfa40] [c042511c] scsi_dispatch_cmd+0xd4/0x110
> [   16.201602] [ee3cfa50] [c0427a9c] scsi_request_fn+0x52c/0x55c

Oh, that one again.  My patch still applies.  Here it is as applied to
that revision of the file.

>From what I can tell, that bug has always been there.  Probably nobody
ever tested the driver in a PREEMPT or SMP build, nor with lock
debugging enabled.

-- 
Måns Rullgård

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ata-sata_dwc_460ex-remove-incorrect-locking.patch --]
[-- Type: text/x-diff, Size: 1375 bytes --]

>From 53f3cf096f959a58fe6339f4f8b57b6e749b283e Mon Sep 17 00:00:00 2001
From: Mans Rullgard <mans@mansr.com>
Date: Sat, 19 Dec 2015 15:26:23 +0000
Subject: [PATCH] ata: sata_dwc_460ex: remove incorrect locking

This lock is already taken in ata_scsi_queuecmd() a few levels up the
call stack so attempting to take it here is an error.  Moreover, it is
pointless in the first place since it only protects a single, atomic
assignment.

Signed-off-by: Mans Rullgard <mans@mansr.com>
---
 drivers/ata/sata_dwc_460ex.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index fdb0f28..d404af8 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -1403,15 +1403,13 @@ static void sata_dwc_exec_command_by_tag(struct ata_port *ap,
 					 struct ata_taskfile *tf,
 					 u8 tag, u32 cmd_issued)
 {
-	unsigned long flags;
 	struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
 
 	dev_dbg(ap->dev, "%s cmd(0x%02x): %s tag=%d\n", __func__, tf->command,
 		ata_get_cmd_descript(tf->command), tag);
 
-	spin_lock_irqsave(&ap->host->lock, flags);
 	hsdevp->cmd_issued[tag] = cmd_issued;
-	spin_unlock_irqrestore(&ap->host->lock, flags);
+
 	/*
 	 * Clear SError before executing a new command.
 	 * sata_dwc_scr_write and read can not be used here. Clearing the PM
-- 
2.6.3