The TI CPSW(s) driver produces warning with DMA API debug options enabled:
WARNING: CPU: 0 PID: 1033 at kernel/dma/debug.c:1025 check_unmap+0x4a8/0x968
DMA-API: cpsw 48484000.ethernet: device driver frees DMA memory with different size
[device address=0x00000000abc6aa02] [map size=64 bytes] [unmap size=42 bytes]
CPU: 0 PID: 1033 Comm: ping Not tainted 5.3.0-dirty #41
Hardware name: Generic DRA72X (Flattened Device Tree)
[<c0112c60>] (unwind_backtrace) from [<c010d270>] (show_stack+0x10/0x14)
[<c010d270>] (show_stack) from [<c09bc564>] (dump_stack+0xd8/0x110)
[<c09bc564>] (dump_stack) from [<c013b93c>] (__warn+0xe0/0x10c)
[<c013b93c>] (__warn) from [<c013b9ac>] (warn_slowpath_fmt+0x44/0x6c)
[<c013b9ac>] (warn_slowpath_fmt) from [<c01e0368>] (check_unmap+0x4a8/0x968)
[<c01e0368>] (check_unmap) from [<c01e08a8>] (debug_dma_unmap_page+0x80/0x90)
[<c01e08a8>] (debug_dma_unmap_page) from [<c0752414>] (__cpdma_chan_free+0x114/0x16c)
[<c0752414>] (__cpdma_chan_free) from [<c07525c4>] (__cpdma_chan_process+0x158/0x17c)
[<c07525c4>] (__cpdma_chan_process) from [<c0753690>] (cpdma_chan_process+0x3c/0x5c)
[<c0753690>] (cpdma_chan_process) from [<c0758660>] (cpsw_tx_mq_poll+0x48/0x94)
[<c0758660>] (cpsw_tx_mq_poll) from [<c0803018>] (net_rx_action+0x108/0x4e4)
[<c0803018>] (net_rx_action) from [<c010230c>] (__do_softirq+0xec/0x598)
[<c010230c>] (__do_softirq) from [<c0143914>] (do_softirq.part.4+0x68/0x74)
[<c0143914>] (do_softirq.part.4) from [<c0143a44>] (__local_bh_enable_ip+0x124/0x17c)
[<c0143a44>] (__local_bh_enable_ip) from [<c0871590>] (ip_finish_output2+0x294/0xb7c)
[<c0871590>] (ip_finish_output2) from [<c0875440>] (ip_output+0x210/0x364)
[<c0875440>] (ip_output) from [<c0875e2c>] (ip_send_skb+0x1c/0xf8)
[<c0875e2c>] (ip_send_skb) from [<c08a7fd4>] (raw_sendmsg+0x9a8/0xc74)
[<c08a7fd4>] (raw_sendmsg) from [<c07d6b90>] (sock_sendmsg+0x14/0x24)
[<c07d6b90>] (sock_sendmsg) from [<c07d8260>] (__sys_sendto+0xbc/0x100)
[<c07d8260>] (__sys_sendto) from [<c01011ac>] (__sys_trace_return+0x0/0x14)
Exception stack(0xea9a7fa8 to 0xea9a7ff0)
...
The reason is that cpdma_chan_submit_si() now stores original buffer length
(sw_len) in CPDMA descriptor instead of adjusted buffer length (hw_len)
used to map the buffer.
Hence, fix an issue by passing correct buffer length in CPDMA descriptor.
Cc: Ivan Khoronzhuk <[email protected]>
Fixes: 6670acacd59e ("net: ethernet: ti: davinci_cpdma: add dma mapped submit")
Signed-off-by: Grygorii Strashko <[email protected]>
---
drivers/net/ethernet/ti/davinci_cpdma.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
index 37ba708ac781..f30a9b9c78a7 100644
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -1018,7 +1018,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
struct cpdma_chan *chan = si->chan;
struct cpdma_ctlr *ctlr = chan->ctlr;
int len = si->len;
- int swlen = len;
+ int swlen;
struct cpdma_desc __iomem *desc;
dma_addr_t buffer;
u32 mode;
@@ -1040,6 +1040,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
chan->stats.runt_transmit_buff++;
}
+ swlen = len;
mode = CPDMA_DESC_OWNER | CPDMA_DESC_SOP | CPDMA_DESC_EOP;
cpdma_desc_to_port(chan, mode, si->directed);
--
2.17.1
From: Grygorii Strashko <[email protected]>
Date: Wed, 4 Dec 2019 18:50:29 +0200
> @@ -1018,7 +1018,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
> struct cpdma_chan *chan = si->chan;
> struct cpdma_ctlr *ctlr = chan->ctlr;
> int len = si->len;
> - int swlen = len;
> + int swlen;
> struct cpdma_desc __iomem *desc;
> dma_addr_t buffer;
> u32 mode;
> @@ -1040,6 +1040,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
> chan->stats.runt_transmit_buff++;
> }
>
> + swlen = len;
> mode = CPDMA_DESC_OWNER | CPDMA_DESC_SOP | CPDMA_DESC_EOP;
> cpdma_desc_to_port(chan, mode, si->directed);
>
> --
> 2.17.1
>
Now there is no reason to keep a separate swlen variable.
The integral value is always consumed as the length before the descriptor bits
are added to it.
Therefore you can just use 'len' everywhere in this function now.
On 04/12/2019 22:37, David Miller wrote:
> From: Grygorii Strashko <[email protected]>
> Date: Wed, 4 Dec 2019 18:50:29 +0200
>
>> @@ -1018,7 +1018,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
>> struct cpdma_chan *chan = si->chan;
>> struct cpdma_ctlr *ctlr = chan->ctlr;
>> int len = si->len;
>> - int swlen = len;
>> + int swlen;
>> struct cpdma_desc __iomem *desc;
>> dma_addr_t buffer;
>> u32 mode;
>> @@ -1040,6 +1040,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
>> chan->stats.runt_transmit_buff++;
>> }
>>
>> + swlen = len;
>> mode = CPDMA_DESC_OWNER | CPDMA_DESC_SOP | CPDMA_DESC_EOP;
>> cpdma_desc_to_port(chan, mode, si->directed);
>>
>> --
>> 2.17.1
>>
>
> Now there is no reason to keep a separate swlen variable.
>
> The integral value is always consumed as the length before the descriptor bits
> are added to it.
>
> Therefore you can just use 'len' everywhere in this function now.
>
Sry, but seems i can't, at least i can't just drop swlen.
Below in this function:
writel_relaxed(0, &desc->hw_next);
writel_relaxed(buffer, &desc->hw_buffer);
writel_relaxed(len, &desc->hw_len);
writel_relaxed(mode | len, &desc->hw_mode);
^^ here the "len" should be use
writel_relaxed((uintptr_t)si->token, &desc->sw_token);
writel_relaxed(buffer, &desc->sw_buffer);
writel_relaxed(swlen, &desc->sw_len);
^^ and here "len"|CPDMA_DMA_EXT_MAP if (si->data_dma) [1]
desc_read(desc, sw_len);
so additional if statement has to be added at [1] if "swlen" is dropped
--
Best regards,
grygorii
On Thu, Dec 05, 2019 at 12:48:46PM +0200, Grygorii Strashko wrote:
>
>
>On 04/12/2019 22:37, David Miller wrote:
>>From: Grygorii Strashko <[email protected]>
>>Date: Wed, 4 Dec 2019 18:50:29 +0200
>>
>>>@@ -1018,7 +1018,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
>>> struct cpdma_chan *chan = si->chan;
>>> struct cpdma_ctlr *ctlr = chan->ctlr;
>>> int len = si->len;
>>>- int swlen = len;
>>>+ int swlen;
>>> struct cpdma_desc __iomem *desc;
>>> dma_addr_t buffer;
>>> u32 mode;
>>>@@ -1040,6 +1040,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
>>> chan->stats.runt_transmit_buff++;
>>> }
>>>+ swlen = len;
>>> mode = CPDMA_DESC_OWNER | CPDMA_DESC_SOP | CPDMA_DESC_EOP;
>>> cpdma_desc_to_port(chan, mode, si->directed);
>>>--
>>>2.17.1
>>>
>>
>>Now there is no reason to keep a separate swlen variable.
>>
>>The integral value is always consumed as the length before the descriptor bits
>>are added to it.
>>
>>Therefore you can just use 'len' everywhere in this function now.
>>
>
>Sry, but seems i can't, at least i can't just drop swlen.
>
>Below in this function:
> writel_relaxed(0, &desc->hw_next);
> writel_relaxed(buffer, &desc->hw_buffer);
> writel_relaxed(len, &desc->hw_len);
> writel_relaxed(mode | len, &desc->hw_mode);
>^^ here the "len" should be use
>
> writel_relaxed((uintptr_t)si->token, &desc->sw_token);
> writel_relaxed(buffer, &desc->sw_buffer);
> writel_relaxed(swlen, &desc->sw_len);
>^^ and here "len"|CPDMA_DMA_EXT_MAP if (si->data_dma) [1]
>
> desc_read(desc, sw_len);
>
>so additional if statement has to be added at [1] if "swlen" is dropped
>
>--
>Best regards,
>grygorii
Seems like yes,
And the "swlen" can be avoided like this:
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -1018,7 +1018,6 @@ static int cpdma_chan_submit_si(struct submit_info *si)
struct cpdma_chan *chan = si->chan;
struct cpdma_ctlr *ctlr = chan->ctlr;
int len = si->len;
- int swlen = len;
struct cpdma_desc __iomem *desc;
dma_addr_t buffer;
u32 mode;
@@ -1046,7 +1045,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
if (si->data_dma) {
buffer = si->data_dma;
dma_sync_single_for_device(ctlr->dev, buffer, len, chan->dir);
- swlen |= CPDMA_DMA_EXT_MAP;
+ writel_relaxed(len | CPDMA_DMA_EXT_MAP, &desc->sw_len);
} else {
buffer = dma_map_single(ctlr->dev, si->data_virt, len, chan->dir);
ret = dma_mapping_error(ctlr->dev, buffer);
@@ -1054,6 +1053,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
cpdma_desc_free(ctlr->pool, desc, 1);
return -EINVAL;
}
+ writel_relaxed(len, &desc->sw_len);
}
/* Relaxed IO accessors can be used here as there is read barrier
@@ -1065,7 +1065,6 @@ static int cpdma_chan_submit_si(struct submit_info *si)
writel_relaxed(mode | len, &desc->hw_mode);
writel_relaxed((uintptr_t)si->token, &desc->sw_token);
writel_relaxed(buffer, &desc->sw_buffer);
- writel_relaxed(swlen, &desc->sw_len);
desc_read(desc, sw_len);
__cpdma_chan_submit(chan, desc);
But not sure what is better.
--
Regards,
Ivan Khoronzhuk
On Sat, Dec 07, 2019 at 01:44:20PM +0200, Ivan Khoronzhuk wrote:
>On Thu, Dec 05, 2019 at 12:48:46PM +0200, Grygorii Strashko wrote:
>>
>>
>>On 04/12/2019 22:37, David Miller wrote:
>>>From: Grygorii Strashko <[email protected]>
>>>Date: Wed, 4 Dec 2019 18:50:29 +0200
>>>
>>>>@@ -1018,7 +1018,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
>>>> struct cpdma_chan *chan = si->chan;
>>>> struct cpdma_ctlr *ctlr = chan->ctlr;
>>>> int len = si->len;
>>>>- int swlen = len;
>>>>+ int swlen;
>>>> struct cpdma_desc __iomem *desc;
>>>> dma_addr_t buffer;
>>>> u32 mode;
>>>>@@ -1040,6 +1040,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
>>>> chan->stats.runt_transmit_buff++;
>>>> }
>>>>+ swlen = len;
>>>> mode = CPDMA_DESC_OWNER | CPDMA_DESC_SOP | CPDMA_DESC_EOP;
>>>> cpdma_desc_to_port(chan, mode, si->directed);
>>>>--
>>>>2.17.1
>>>>
>>>
>>>Now there is no reason to keep a separate swlen variable.
>>>
>>>The integral value is always consumed as the length before the descriptor bits
>>>are added to it.
>>>
>>>Therefore you can just use 'len' everywhere in this function now.
>>>
>>
>>Sry, but seems i can't, at least i can't just drop swlen.
>>
>>Below in this function:
>> writel_relaxed(0, &desc->hw_next);
>> writel_relaxed(buffer, &desc->hw_buffer);
>> writel_relaxed(len, &desc->hw_len);
>> writel_relaxed(mode | len, &desc->hw_mode);
>>^^ here the "len" should be use
>>
>> writel_relaxed((uintptr_t)si->token, &desc->sw_token);
>> writel_relaxed(buffer, &desc->sw_buffer);
>> writel_relaxed(swlen, &desc->sw_len);
>>^^ and here "len"|CPDMA_DMA_EXT_MAP if (si->data_dma) [1]
>>
>> desc_read(desc, sw_len);
>>
>>so additional if statement has to be added at [1] if "swlen" is dropped
>>
>>--
>>Best regards,
>>grygorii
>
>Seems like yes,
>
>And the "swlen" can be avoided like this:
>
>--- a/drivers/net/ethernet/ti/davinci_cpdma.c
>+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>@@ -1018,7 +1018,6 @@ static int cpdma_chan_submit_si(struct submit_info *si)
> struct cpdma_chan *chan = si->chan;
> struct cpdma_ctlr *ctlr = chan->ctlr;
> int len = si->len;
>- int swlen = len;
> struct cpdma_desc __iomem *desc;
> dma_addr_t buffer;
> u32 mode;
>@@ -1046,7 +1045,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
> if (si->data_dma) {
> buffer = si->data_dma;
> dma_sync_single_for_device(ctlr->dev, buffer, len, chan->dir);
>- swlen |= CPDMA_DMA_EXT_MAP;
>+ writel_relaxed(len | CPDMA_DMA_EXT_MAP, &desc->sw_len);
> } else {
> buffer = dma_map_single(ctlr->dev, si->data_virt, len, chan->dir);
> ret = dma_mapping_error(ctlr->dev, buffer);
>@@ -1054,6 +1053,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
> cpdma_desc_free(ctlr->pool, desc, 1);
> return -EINVAL;
> }
>+ writel_relaxed(len, &desc->sw_len);
> }
>
> /* Relaxed IO accessors can be used here as there is read barrier
>@@ -1065,7 +1065,6 @@ static int cpdma_chan_submit_si(struct submit_info *si)
> writel_relaxed(mode | len, &desc->hw_mode);
> writel_relaxed((uintptr_t)si->token, &desc->sw_token);
> writel_relaxed(buffer, &desc->sw_buffer);
>- writel_relaxed(swlen, &desc->sw_len);
> desc_read(desc, sw_len);
>
> __cpdma_chan_submit(chan, desc);
>
>But not sure what is better.
>
Or like this:
--- a/drivers/net/ethernet/ti/davinci_cpdma.c
+++ b/drivers/net/ethernet/ti/davinci_cpdma.c
@@ -1018,7 +1018,6 @@ static int cpdma_chan_submit_si(struct submit_info *si)
struct cpdma_chan *chan = si->chan;
struct cpdma_ctlr *ctlr = chan->ctlr;
int len = si->len;
- int swlen = len;
struct cpdma_desc __iomem *desc;
dma_addr_t buffer;
u32 mode;
@@ -1046,7 +1045,6 @@ static int cpdma_chan_submit_si(struct submit_info *si)
if (si->data_dma) {
buffer = si->data_dma;
dma_sync_single_for_device(ctlr->dev, buffer, len, chan->dir);
- swlen |= CPDMA_DMA_EXT_MAP;
} else {
buffer = dma_map_single(ctlr->dev, si->data_virt, len, chan->dir);
ret = dma_mapping_error(ctlr->dev, buffer);
@@ -1065,7 +1063,7 @@ static int cpdma_chan_submit_si(struct submit_info *si)
writel_relaxed(mode | len, &desc->hw_mode);
writel_relaxed((uintptr_t)si->token, &desc->sw_token);
writel_relaxed(buffer, &desc->sw_buffer);
- writel_relaxed(swlen, &desc->sw_len);
+ writel_relaxed(si->data_dma ? len | CPDMA_DMA_EXT_MAP : len, &desc->sw_len);
desc_read(desc, sw_len);
__cpdma_chan_submit(chan, desc);
But it's branched twice then.
--
Regards,
Ivan Khoronzhuk
From: Ivan Khoronzhuk <[email protected]>
Date: Sat, 7 Dec 2019 14:02:39 +0200
> @@ -1046,7 +1045,6 @@ static int cpdma_chan_submit_si(struct
> submit_info *si)
> if (si->data_dma) {
> buffer = si->data_dma;
> dma_sync_single_for_device(ctlr->dev, buffer, len, chan->dir);
> - swlen |= CPDMA_DMA_EXT_MAP;
> } else {
> buffer = dma_map_single(ctlr->dev, si->data_virt, len, chan->dir);
> ret = dma_mapping_error(ctlr->dev, buffer);
> @@ -1065,7 +1063,7 @@ static int cpdma_chan_submit_si(struct
> submit_info *si)
> writel_relaxed(mode | len, &desc->hw_mode);
> writel_relaxed((uintptr_t)si->token, &desc->sw_token);
> writel_relaxed(buffer, &desc->sw_buffer);
> - writel_relaxed(swlen, &desc->sw_len);
> + writel_relaxed(si->data_dma ? len | CPDMA_DMA_EXT_MAP : len,
> &desc->sw_len);
> desc_read(desc, sw_len);
>
> __cpdma_chan_submit(chan, desc);
>
> But it's branched twice then.
It's a conditional move rather than a branch....