* [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.