* [PATCH v2] net: ethernet: ti: davinci_cpdma: fix warning "device driver frees DMA memory with different size"
@ 2019-12-04 16:50 Grygorii Strashko
2019-12-04 20:37 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Grygorii Strashko @ 2019-12-04 16:50 UTC (permalink / raw)
To: David S. Miller, netdev, Ivan Khoronzhuk
Cc: Sekhar Nori, linux-kernel, Grygorii Strashko
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 <ivan.khoronzhuk@linaro.org>
Fixes: 6670acacd59e ("net: ethernet: ti: davinci_cpdma: add dma mapped submit")
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] net: ethernet: ti: davinci_cpdma: fix warning "device driver frees DMA memory with different size"
2019-12-04 16:50 [PATCH v2] net: ethernet: ti: davinci_cpdma: fix warning "device driver frees DMA memory with different size" Grygorii Strashko
@ 2019-12-04 20:37 ` David Miller
2019-12-05 10:48 ` Grygorii Strashko
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2019-12-04 20:37 UTC (permalink / raw)
To: grygorii.strashko; +Cc: netdev, ivan.khoronzhuk, nsekhar, linux-kernel
From: Grygorii Strashko <grygorii.strashko@ti.com>
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] net: ethernet: ti: davinci_cpdma: fix warning "device driver frees DMA memory with different size"
2019-12-04 20:37 ` David Miller
@ 2019-12-05 10:48 ` Grygorii Strashko
2019-12-07 11:44 ` Ivan Khoronzhuk
0 siblings, 1 reply; 6+ messages in thread
From: Grygorii Strashko @ 2019-12-05 10:48 UTC (permalink / raw)
To: David Miller; +Cc: netdev, ivan.khoronzhuk, nsekhar, linux-kernel
On 04/12/2019 22:37, David Miller wrote:
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] net: ethernet: ti: davinci_cpdma: fix warning "device driver frees DMA memory with different size"
2019-12-05 10:48 ` Grygorii Strashko
@ 2019-12-07 11:44 ` Ivan Khoronzhuk
2019-12-07 12:02 ` Ivan Khoronzhuk
0 siblings, 1 reply; 6+ messages in thread
From: Ivan Khoronzhuk @ 2019-12-07 11:44 UTC (permalink / raw)
To: Grygorii Strashko; +Cc: David Miller, netdev, nsekhar, linux-kernel
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 <grygorii.strashko@ti.com>
>>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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] net: ethernet: ti: davinci_cpdma: fix warning "device driver frees DMA memory with different size"
2019-12-07 11:44 ` Ivan Khoronzhuk
@ 2019-12-07 12:02 ` Ivan Khoronzhuk
2019-12-07 19:50 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: Ivan Khoronzhuk @ 2019-12-07 12:02 UTC (permalink / raw)
To: Grygorii Strashko, David Miller, netdev, nsekhar, linux-kernel
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 <grygorii.strashko@ti.com>
>>>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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] net: ethernet: ti: davinci_cpdma: fix warning "device driver frees DMA memory with different size"
2019-12-07 12:02 ` Ivan Khoronzhuk
@ 2019-12-07 19:50 ` David Miller
0 siblings, 0 replies; 6+ messages in thread
From: David Miller @ 2019-12-07 19:50 UTC (permalink / raw)
To: ivan.khoronzhuk; +Cc: grygorii.strashko, netdev, nsekhar, linux-kernel
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
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....
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-07 19:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-04 16:50 [PATCH v2] net: ethernet: ti: davinci_cpdma: fix warning "device driver frees DMA memory with different size" Grygorii Strashko
2019-12-04 20:37 ` David Miller
2019-12-05 10:48 ` Grygorii Strashko
2019-12-07 11:44 ` Ivan Khoronzhuk
2019-12-07 12:02 ` Ivan Khoronzhuk
2019-12-07 19:50 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.