diff mbox series

[v2] net: ethernet: ti: davinci_cpdma: fix warning "device driver frees DMA memory with different size"

Message ID 20191204165029.9264-1-grygorii.strashko@ti.com
State New
Headers show
Series [v2] net: ethernet: ti: davinci_cpdma: fix warning "device driver frees DMA memory with different size" | expand

Commit Message

Grygorii Strashko Dec. 4, 2019, 4:50 p.m. UTC 
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(-)

-- 
2.17.1

Comments

David Miller Dec. 4, 2019, 8:37 p.m. UTC | #1 
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.
Grygorii Strashko Dec. 5, 2019, 10:48 a.m. UTC | #2 
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
Ivan Khoronzhuk Dec. 7, 2019, 12:02 p.m. UTC | #3 
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
diff mbox series

Patch

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);