| Message ID | 20230407172402.103168-1-roman.gushchin@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: macb: fix a memory corruption in extended buffer descriptor mode | expand |
Friendly ping. Also cc'ing Dave. Thanks! On Fri, Apr 07, 2023 at 10:24:02AM -0700, Roman Gushchin wrote: > For quite some time we were chasing a bug which looked like a sudden > permanent failure of networking and mmc on some of our devices. > The bug was very sensitive to any software changes and even more to > any kernel debug options. > > Finally we got a setup where the problem was reproducible with > CONFIG_DMA_API_DEBUG=y and it revealed the issue with the rx dma: > > [ 16.992082] ------------[ cut here ]------------ > [ 16.996779] DMA-API: macb ff0b0000.ethernet: device driver tries to free DMA memory it has not allocated [device address=0x0000000875e3e244] [size=1536 bytes] > [ 17.011049] WARNING: CPU: 0 PID: 85 at kernel/dma/debug.c:1011 check_unmap+0x6a0/0x900 > [ 17.018977] Modules linked in: xxxxx > [ 17.038823] CPU: 0 PID: 85 Comm: irq/55-8000f000 Not tainted 5.4.0 #28 > [ 17.045345] Hardware name: xxxxx > [ 17.049528] pstate: 60000005 (nZCv daif -PAN -UAO) > [ 17.054322] pc : check_unmap+0x6a0/0x900 > [ 17.058243] lr : check_unmap+0x6a0/0x900 > [ 17.062163] sp : ffffffc010003c40 > [ 17.065470] x29: ffffffc010003c40 x28: 000000004000c03c > [ 17.070783] x27: ffffffc010da7048 x26: ffffff8878e38800 > [ 17.076095] x25: ffffff8879d22810 x24: ffffffc010003cc8 > [ 17.081407] x23: 0000000000000000 x22: ffffffc010a08750 > [ 17.086719] x21: ffffff8878e3c7c0 x20: ffffffc010acb000 > [ 17.092032] x19: 0000000875e3e244 x18: 0000000000000010 > [ 17.097343] x17: 0000000000000000 x16: 0000000000000000 > [ 17.102647] x15: ffffff8879e4a988 x14: 0720072007200720 > [ 17.107959] x13: 0720072007200720 x12: 0720072007200720 > [ 17.113261] x11: 0720072007200720 x10: 0720072007200720 > [ 17.118565] x9 : 0720072007200720 x8 : 000000000000022d > [ 17.123869] x7 : 0000000000000015 x6 : 0000000000000098 > [ 17.129173] x5 : 0000000000000000 x4 : 0000000000000000 > [ 17.134475] x3 : 00000000ffffffff x2 : ffffffc010a1d370 > [ 17.139778] x1 : b420c9d75d27bb00 x0 : 0000000000000000 > [ 17.145082] Call trace: > [ 17.147524] check_unmap+0x6a0/0x900 > [ 17.151091] debug_dma_unmap_page+0x88/0x90 > [ 17.155266] gem_rx+0x114/0x2f0 > [ 17.158396] macb_poll+0x58/0x100 > [ 17.161705] net_rx_action+0x118/0x400 > [ 17.165445] __do_softirq+0x138/0x36c > [ 17.169100] irq_exit+0x98/0xc0 > [ 17.172234] __handle_domain_irq+0x64/0xc0 > [ 17.176320] gic_handle_irq+0x5c/0xc0 > [ 17.179974] el1_irq+0xb8/0x140 > [ 17.183109] xiic_process+0x5c/0xe30 > [ 17.186677] irq_thread_fn+0x28/0x90 > [ 17.190244] irq_thread+0x208/0x2a0 > [ 17.193724] kthread+0x130/0x140 > [ 17.196945] ret_from_fork+0x10/0x20 > [ 17.200510] ---[ end trace 7240980785f81d6f ]--- > > [ 237.021490] ------------[ cut here ]------------ > [ 237.026129] DMA-API: exceeded 7 overlapping mappings of cacheline 0x0000000021d79e7b > [ 237.033886] WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:499 add_dma_entry+0x214/0x240 > [ 237.041802] Modules linked in: xxxxx > [ 237.061637] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 5.4.0 #28 > [ 237.068941] Hardware name: xxxxx > [ 237.073116] pstate: 80000085 (Nzcv daIf -PAN -UAO) > [ 237.077900] pc : add_dma_entry+0x214/0x240 > [ 237.081986] lr : add_dma_entry+0x214/0x240 > [ 237.086072] sp : ffffffc010003c30 > [ 237.089379] x29: ffffffc010003c30 x28: ffffff8878a0be00 > [ 237.094683] x27: 0000000000000180 x26: ffffff8878e387c0 > [ 237.099987] x25: 0000000000000002 x24: 0000000000000000 > [ 237.105290] x23: 000000000000003b x22: ffffffc010a0fa00 > [ 237.110594] x21: 0000000021d79e7b x20: ffffffc010abe600 > [ 237.115897] x19: 00000000ffffffef x18: 0000000000000010 > [ 237.121201] x17: 0000000000000000 x16: 0000000000000000 > [ 237.126504] x15: ffffffc010a0fdc8 x14: 0720072007200720 > [ 237.131807] x13: 0720072007200720 x12: 0720072007200720 > [ 237.137111] x11: 0720072007200720 x10: 0720072007200720 > [ 237.142415] x9 : 0720072007200720 x8 : 0000000000000259 > [ 237.147718] x7 : 0000000000000001 x6 : 0000000000000000 > [ 237.153022] x5 : ffffffc010003a20 x4 : 0000000000000001 > [ 237.158325] x3 : 0000000000000006 x2 : 0000000000000007 > [ 237.163628] x1 : 8ac721b3a7dc1c00 x0 : 0000000000000000 > [ 237.168932] Call trace: > [ 237.171373] add_dma_entry+0x214/0x240 > [ 237.175115] debug_dma_map_page+0xf8/0x120 > [ 237.179203] gem_rx_refill+0x190/0x280 > [ 237.182942] gem_rx+0x224/0x2f0 > [ 237.186075] macb_poll+0x58/0x100 > [ 237.189384] net_rx_action+0x118/0x400 > [ 237.193125] __do_softirq+0x138/0x36c > [ 237.196780] irq_exit+0x98/0xc0 > [ 237.199914] __handle_domain_irq+0x64/0xc0 > [ 237.204000] gic_handle_irq+0x5c/0xc0 > [ 237.207654] el1_irq+0xb8/0x140 > [ 237.210789] arch_cpu_idle+0x40/0x200 > [ 237.214444] default_idle_call+0x18/0x30 > [ 237.218359] do_idle+0x200/0x280 > [ 237.221578] cpu_startup_entry+0x20/0x30 > [ 237.225493] rest_init+0xe4/0xf0 > [ 237.228713] arch_call_rest_init+0xc/0x14 > [ 237.232714] start_kernel+0x47c/0x4a8 > [ 237.236367] ---[ end trace 7240980785f81d70 ]--- > > Lars was fast to find an explanation: according to the datasheet > bit 2 of the rx buffer descriptor entry has a different meaning in the > extended mode: > Address [2] of beginning of buffer, or > in extended buffer descriptor mode (DMA configuration register [28] = 1), > indicates a valid timestamp in the buffer descriptor entry. > > The macb driver didn't mask this bit while getting an address and it > eventually caused a memory corruption and a dma failure. > > The problem is resolved by extending the MACB_RX_WADDR_SIZE > in the extended mode. > > Fixes: 7b4296148066 ("net: macb: Add support for PTP timestamps in DMA descriptors") > Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev> > Co-developed-by: Lars-Peter Clausen <lars@metafoo.de> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > --- > drivers/net/ethernet/cadence/macb.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h > index c1fc91c97cee..1b330f7cfc09 100644 > --- a/drivers/net/ethernet/cadence/macb.h > +++ b/drivers/net/ethernet/cadence/macb.h > @@ -826,8 +826,13 @@ struct macb_dma_desc_ptp { > #define MACB_RX_USED_SIZE 1 > #define MACB_RX_WRAP_OFFSET 1 > #define MACB_RX_WRAP_SIZE 1 > +#ifdef MACB_EXT_DESC > +#define MACB_RX_WADDR_OFFSET 3 > +#define MACB_RX_WADDR_SIZE 29 > +#else > #define MACB_RX_WADDR_OFFSET 2 > #define MACB_RX_WADDR_SIZE 30 > +#endif > > #define MACB_RX_FRMLEN_OFFSET 0 > #define MACB_RX_FRMLEN_SIZE 12 > -- > 2.40.0 >
On Tue, Apr 11, 2023 at 11:20:34AM -0700, Roman Gushchin wrote: > Friendly ping. Nicolas and Claudiu look after the macb stuff, it's a good idea to CC the people that get_maintainer.pl says are supporters of the code! > Also cc'ing Dave. +CC Nicolas & Claudiu ;) > Thanks! > > On Fri, Apr 07, 2023 at 10:24:02AM -0700, Roman Gushchin wrote: > > For quite some time we were chasing a bug which looked like a sudden > > permanent failure of networking and mmc on some of our devices. > > The bug was very sensitive to any software changes and even more to > > any kernel debug options. > > > > Finally we got a setup where the problem was reproducible with > > CONFIG_DMA_API_DEBUG=y and it revealed the issue with the rx dma: > > > > [ 16.992082] ------------[ cut here ]------------ > > [ 16.996779] DMA-API: macb ff0b0000.ethernet: device driver tries to free DMA memory it has not allocated [device address=0x0000000875e3e244] [size=1536 bytes] > > [ 17.011049] WARNING: CPU: 0 PID: 85 at kernel/dma/debug.c:1011 check_unmap+0x6a0/0x900 > > [ 17.018977] Modules linked in: xxxxx > > [ 17.038823] CPU: 0 PID: 85 Comm: irq/55-8000f000 Not tainted 5.4.0 #28 > > [ 17.045345] Hardware name: xxxxx > > [ 17.049528] pstate: 60000005 (nZCv daif -PAN -UAO) > > [ 17.054322] pc : check_unmap+0x6a0/0x900 > > [ 17.058243] lr : check_unmap+0x6a0/0x900 > > [ 17.062163] sp : ffffffc010003c40 > > [ 17.065470] x29: ffffffc010003c40 x28: 000000004000c03c > > [ 17.070783] x27: ffffffc010da7048 x26: ffffff8878e38800 > > [ 17.076095] x25: ffffff8879d22810 x24: ffffffc010003cc8 > > [ 17.081407] x23: 0000000000000000 x22: ffffffc010a08750 > > [ 17.086719] x21: ffffff8878e3c7c0 x20: ffffffc010acb000 > > [ 17.092032] x19: 0000000875e3e244 x18: 0000000000000010 > > [ 17.097343] x17: 0000000000000000 x16: 0000000000000000 > > [ 17.102647] x15: ffffff8879e4a988 x14: 0720072007200720 > > [ 17.107959] x13: 0720072007200720 x12: 0720072007200720 > > [ 17.113261] x11: 0720072007200720 x10: 0720072007200720 > > [ 17.118565] x9 : 0720072007200720 x8 : 000000000000022d > > [ 17.123869] x7 : 0000000000000015 x6 : 0000000000000098 > > [ 17.129173] x5 : 0000000000000000 x4 : 0000000000000000 > > [ 17.134475] x3 : 00000000ffffffff x2 : ffffffc010a1d370 > > [ 17.139778] x1 : b420c9d75d27bb00 x0 : 0000000000000000 > > [ 17.145082] Call trace: > > [ 17.147524] check_unmap+0x6a0/0x900 > > [ 17.151091] debug_dma_unmap_page+0x88/0x90 > > [ 17.155266] gem_rx+0x114/0x2f0 > > [ 17.158396] macb_poll+0x58/0x100 > > [ 17.161705] net_rx_action+0x118/0x400 > > [ 17.165445] __do_softirq+0x138/0x36c > > [ 17.169100] irq_exit+0x98/0xc0 > > [ 17.172234] __handle_domain_irq+0x64/0xc0 > > [ 17.176320] gic_handle_irq+0x5c/0xc0 > > [ 17.179974] el1_irq+0xb8/0x140 > > [ 17.183109] xiic_process+0x5c/0xe30 > > [ 17.186677] irq_thread_fn+0x28/0x90 > > [ 17.190244] irq_thread+0x208/0x2a0 > > [ 17.193724] kthread+0x130/0x140 > > [ 17.196945] ret_from_fork+0x10/0x20 > > [ 17.200510] ---[ end trace 7240980785f81d6f ]--- > > > > [ 237.021490] ------------[ cut here ]------------ > > [ 237.026129] DMA-API: exceeded 7 overlapping mappings of cacheline 0x0000000021d79e7b > > [ 237.033886] WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:499 add_dma_entry+0x214/0x240 > > [ 237.041802] Modules linked in: xxxxx > > [ 237.061637] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 5.4.0 #28 > > [ 237.068941] Hardware name: xxxxx > > [ 237.073116] pstate: 80000085 (Nzcv daIf -PAN -UAO) > > [ 237.077900] pc : add_dma_entry+0x214/0x240 > > [ 237.081986] lr : add_dma_entry+0x214/0x240 > > [ 237.086072] sp : ffffffc010003c30 > > [ 237.089379] x29: ffffffc010003c30 x28: ffffff8878a0be00 > > [ 237.094683] x27: 0000000000000180 x26: ffffff8878e387c0 > > [ 237.099987] x25: 0000000000000002 x24: 0000000000000000 > > [ 237.105290] x23: 000000000000003b x22: ffffffc010a0fa00 > > [ 237.110594] x21: 0000000021d79e7b x20: ffffffc010abe600 > > [ 237.115897] x19: 00000000ffffffef x18: 0000000000000010 > > [ 237.121201] x17: 0000000000000000 x16: 0000000000000000 > > [ 237.126504] x15: ffffffc010a0fdc8 x14: 0720072007200720 > > [ 237.131807] x13: 0720072007200720 x12: 0720072007200720 > > [ 237.137111] x11: 0720072007200720 x10: 0720072007200720 > > [ 237.142415] x9 : 0720072007200720 x8 : 0000000000000259 > > [ 237.147718] x7 : 0000000000000001 x6 : 0000000000000000 > > [ 237.153022] x5 : ffffffc010003a20 x4 : 0000000000000001 > > [ 237.158325] x3 : 0000000000000006 x2 : 0000000000000007 > > [ 237.163628] x1 : 8ac721b3a7dc1c00 x0 : 0000000000000000 > > [ 237.168932] Call trace: > > [ 237.171373] add_dma_entry+0x214/0x240 > > [ 237.175115] debug_dma_map_page+0xf8/0x120 > > [ 237.179203] gem_rx_refill+0x190/0x280 > > [ 237.182942] gem_rx+0x224/0x2f0 > > [ 237.186075] macb_poll+0x58/0x100 > > [ 237.189384] net_rx_action+0x118/0x400 > > [ 237.193125] __do_softirq+0x138/0x36c > > [ 237.196780] irq_exit+0x98/0xc0 > > [ 237.199914] __handle_domain_irq+0x64/0xc0 > > [ 237.204000] gic_handle_irq+0x5c/0xc0 > > [ 237.207654] el1_irq+0xb8/0x140 > > [ 237.210789] arch_cpu_idle+0x40/0x200 > > [ 237.214444] default_idle_call+0x18/0x30 > > [ 237.218359] do_idle+0x200/0x280 > > [ 237.221578] cpu_startup_entry+0x20/0x30 > > [ 237.225493] rest_init+0xe4/0xf0 > > [ 237.228713] arch_call_rest_init+0xc/0x14 > > [ 237.232714] start_kernel+0x47c/0x4a8 > > [ 237.236367] ---[ end trace 7240980785f81d70 ]--- > > > > Lars was fast to find an explanation: according to the datasheet > > bit 2 of the rx buffer descriptor entry has a different meaning in the > > extended mode: > > Address [2] of beginning of buffer, or > > in extended buffer descriptor mode (DMA configuration register [28] = 1), > > indicates a valid timestamp in the buffer descriptor entry. > > > > The macb driver didn't mask this bit while getting an address and it > > eventually caused a memory corruption and a dma failure. > > > > The problem is resolved by extending the MACB_RX_WADDR_SIZE > > in the extended mode. > > > > Fixes: 7b4296148066 ("net: macb: Add support for PTP timestamps in DMA descriptors") > > Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev> > > Co-developed-by: Lars-Peter Clausen <lars@metafoo.de> > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > > --- > > drivers/net/ethernet/cadence/macb.h | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h > > index c1fc91c97cee..1b330f7cfc09 100644 > > --- a/drivers/net/ethernet/cadence/macb.h > > +++ b/drivers/net/ethernet/cadence/macb.h > > @@ -826,8 +826,13 @@ struct macb_dma_desc_ptp { > > #define MACB_RX_USED_SIZE 1 > > #define MACB_RX_WRAP_OFFSET 1 > > #define MACB_RX_WRAP_SIZE 1 > > +#ifdef MACB_EXT_DESC > > +#define MACB_RX_WADDR_OFFSET 3 > > +#define MACB_RX_WADDR_SIZE 29 > > +#else > > #define MACB_RX_WADDR_OFFSET 2 > > #define MACB_RX_WADDR_SIZE 30 > > +#endif > > > > #define MACB_RX_FRMLEN_OFFSET 0 > > #define MACB_RX_FRMLEN_SIZE 12 > > -- > > 2.40.0 > >
On Tue, Apr 11, 2023 at 07:30:00PM +0100, Conor Dooley wrote: > On Tue, Apr 11, 2023 at 11:20:34AM -0700, Roman Gushchin wrote: > > Friendly ping. > > Nicolas and Claudiu look after the macb stuff, it's a good idea to CC > the people that get_maintainer.pl says are supporters of the code! My fault, probably I was too happy to finally find it :) > > > Also cc'ing Dave. > > +CC Nicolas & Claudiu ;) Thank you, appreciate it!
On Fri, 7 Apr 2023 10:24:02 -0700 Roman Gushchin wrote: > The problem is resolved by extending the MACB_RX_WADDR_SIZE > in the extended mode. > > Fixes: 7b4296148066 ("net: macb: Add support for PTP timestamps in DMA descriptors") > Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev> > Co-developed-by: Lars-Peter Clausen <lars@metafoo.de> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > --- > drivers/net/ethernet/cadence/macb.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h > index c1fc91c97cee..1b330f7cfc09 100644 > --- a/drivers/net/ethernet/cadence/macb.h > +++ b/drivers/net/ethernet/cadence/macb.h > @@ -826,8 +826,13 @@ struct macb_dma_desc_ptp { > #define MACB_RX_USED_SIZE 1 > #define MACB_RX_WRAP_OFFSET 1 > #define MACB_RX_WRAP_SIZE 1 > +#ifdef MACB_EXT_DESC > +#define MACB_RX_WADDR_OFFSET 3 > +#define MACB_RX_WADDR_SIZE 29 > +#else > #define MACB_RX_WADDR_OFFSET 2 > #define MACB_RX_WADDR_SIZE 30 > +#endif Changing register definition based on Kconfig seems a bit old school. Where is the extended descriptor mode enabled? Is it always on if Kconfig is set or can it be off for some platforms based on other capabilities? Judging by macb_dma_desc_get_size() small descriptors can still be used even with EXT_DESC? If I'm grepping correctly thru the painful macro magic this register is only used in macb_get_addr(). It'd seem a bit more robust to me to open code the extraction of the address based on bp->hw_dma_cap in that one function. In addition to maintainers please also CC Harini Katakam <harini.katakam@xilinx.com> on v2.
On 4/11/23 18:48, Jakub Kicinski wrote: > On Fri, 7 Apr 2023 10:24:02 -0700 Roman Gushchin wrote: >> The problem is resolved by extending the MACB_RX_WADDR_SIZE >> in the extended mode. >> >> Fixes: 7b4296148066 ("net: macb: Add support for PTP timestamps in DMA descriptors") >> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev> >> Co-developed-by: Lars-Peter Clausen <lars@metafoo.de> >> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> >> --- >> drivers/net/ethernet/cadence/macb.h | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h >> index c1fc91c97cee..1b330f7cfc09 100644 >> --- a/drivers/net/ethernet/cadence/macb.h >> +++ b/drivers/net/ethernet/cadence/macb.h >> @@ -826,8 +826,13 @@ struct macb_dma_desc_ptp { >> #define MACB_RX_USED_SIZE 1 >> #define MACB_RX_WRAP_OFFSET 1 >> #define MACB_RX_WRAP_SIZE 1 >> +#ifdef MACB_EXT_DESC >> +#define MACB_RX_WADDR_OFFSET 3 >> +#define MACB_RX_WADDR_SIZE 29 >> +#else >> #define MACB_RX_WADDR_OFFSET 2 >> #define MACB_RX_WADDR_SIZE 30 >> +#endif > Changing register definition based on Kconfig seems a bit old school. > > Where is the extended descriptor mode enabled? Is it always on if > Kconfig is set or can it be off for some platforms based on other > capabilities? Judging by macb_dma_desc_get_size() small descriptors > can still be used even with EXT_DESC? > > If I'm grepping correctly thru the painful macro magic this register > is only used in macb_get_addr(). It'd seem a bit more robust to me > to open code the extraction of the address based on bp->hw_dma_cap > in that one function. > > In addition to maintainers please also CC Harini Katakam > <harini.katakam@xilinx.com> on v2. We had an alternative patch which fixes this based on runtime settings. But it didn't seem to be worth it considering the runtime overhead, even though it is small. The skb buffer address is guaranteed to be cacheline aligned, otherwise the DMA wouldn't work at all. So we know that the LSBs must always be 0. We could even unconditionally define MACB_RX_WADDR_OFFSET as 3. Alternative runtime base patch: diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index d13fb1d31821..1a40d5a26f36 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -1042,6 +1042,10 @@ static dma_addr_t macb_get_addr(struct macb *bp, struct macb_dma_desc *desc) } #endif addr |= MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, desc->addr)); +#ifdef CONFIG_MACB_USE_HWSTAMP + if (bp->hw_dma_cap & HW_DMA_CAP_PTP) + addr &= ~GEM_BIT(DMA_RXVALID_OFFSET); +#endif return addr; }
On Tue, Apr 11, 2023 at 08:13:51PM -0700, Lars-Peter Clausen wrote: > On 4/11/23 18:48, Jakub Kicinski wrote: > > On Fri, 7 Apr 2023 10:24:02 -0700 Roman Gushchin wrote: > > > The problem is resolved by extending the MACB_RX_WADDR_SIZE > > > in the extended mode. > > > > > > Fixes: 7b4296148066 ("net: macb: Add support for PTP timestamps in DMA descriptors") > > > Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev> > > > Co-developed-by: Lars-Peter Clausen <lars@metafoo.de> > > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > > > --- > > > drivers/net/ethernet/cadence/macb.h | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h > > > index c1fc91c97cee..1b330f7cfc09 100644 > > > --- a/drivers/net/ethernet/cadence/macb.h > > > +++ b/drivers/net/ethernet/cadence/macb.h > > > @@ -826,8 +826,13 @@ struct macb_dma_desc_ptp { > > > #define MACB_RX_USED_SIZE 1 > > > #define MACB_RX_WRAP_OFFSET 1 > > > #define MACB_RX_WRAP_SIZE 1 > > > +#ifdef MACB_EXT_DESC > > > +#define MACB_RX_WADDR_OFFSET 3 > > > +#define MACB_RX_WADDR_SIZE 29 > > > +#else > > > #define MACB_RX_WADDR_OFFSET 2 > > > #define MACB_RX_WADDR_SIZE 30 > > > +#endif > > Changing register definition based on Kconfig seems a bit old school. > > > > Where is the extended descriptor mode enabled? Is it always on if > > Kconfig is set or can it be off for some platforms based on other > > capabilities? Judging by macb_dma_desc_get_size() small descriptors > > can still be used even with EXT_DESC? > > > > If I'm grepping correctly thru the painful macro magic this register > > is only used in macb_get_addr(). It'd seem a bit more robust to me > > to open code the extraction of the address based on bp->hw_dma_cap > > in that one function. > > > > In addition to maintainers please also CC Harini Katakam > > <harini.katakam@xilinx.com> on v2. > > We had an alternative patch which fixes this based on runtime settings. But > it didn't seem to be worth it considering the runtime overhead, even though > it is small. The skb buffer address is guaranteed to be cacheline aligned, > otherwise the DMA wouldn't work at all. So we know that the LSBs must always > be 0. We could even unconditionally define MACB_RX_WADDR_OFFSET as 3. I'd personally prefer this. > > Alternative runtime base patch: > > diff --git a/drivers/net/ethernet/cadence/macb_main.c > b/drivers/net/ethernet/cadence/macb_main.c > index d13fb1d31821..1a40d5a26f36 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -1042,6 +1042,10 @@ static dma_addr_t macb_get_addr(struct macb *bp, > struct macb_dma_desc *desc) > } > #endif > addr |= MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, desc->addr)); > +#ifdef CONFIG_MACB_USE_HWSTAMP > + if (bp->hw_dma_cap & HW_DMA_CAP_PTP) > + addr &= ~GEM_BIT(DMA_RXVALID_OFFSET); > +#endif > return addr; > } > I think this version is slightly worse because it adds an unconditional if statement, which can be removed with certain config options. I can master a version with a helper function, if it's preferable. But if you like this one, it's fine too, let me know, I'll send an updated version. Thanks!
On Tue, 11 Apr 2023 20:48:50 -0700 Roman Gushchin wrote: > > diff --git a/drivers/net/ethernet/cadence/macb_main.c > > b/drivers/net/ethernet/cadence/macb_main.c > > index d13fb1d31821..1a40d5a26f36 100644 > > --- a/drivers/net/ethernet/cadence/macb_main.c > > +++ b/drivers/net/ethernet/cadence/macb_main.c > > @@ -1042,6 +1042,10 @@ static dma_addr_t macb_get_addr(struct macb *bp, > > struct macb_dma_desc *desc) > > } > > #endif > > addr |= MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, desc->addr)); > > +#ifdef CONFIG_MACB_USE_HWSTAMP > > + if (bp->hw_dma_cap & HW_DMA_CAP_PTP) > > + addr &= ~GEM_BIT(DMA_RXVALID_OFFSET); > > +#endif > > return addr; > > } > > I think this version is slightly worse because it adds an unconditional > if statement, which can be removed with certain config options. > I can master a version with a helper function, if it's preferable. > > But if you like this one, it's fine too, let me know, I'll send an updated > version. Yup, IMHO this looks better. More likely that someone reading the code will spot the trickiness. I suspect we could clear that bit unconditionally, if the branch is a concern. The code seems to assume that buffers it gets are 8B aligned already, regardless of CONFIG_MACB_USE_HWSTAMP. Drivers commonly save the DMA address to a SW ring (here I think rx_skbuff plays this role but only holds single ptr per entry) so that they don't have to access potentially uncached descriptor ring. But that'd be too large of a change for a fix.
Jakub, Roman, On 12/04/2023 at 06:13, Jakub Kicinski wrote: > On Tue, 11 Apr 2023 20:48:50 -0700 Roman Gushchin wrote: >>> diff --git a/drivers/net/ethernet/cadence/macb_main.c >>> b/drivers/net/ethernet/cadence/macb_main.c >>> index d13fb1d31821..1a40d5a26f36 100644 >>> --- a/drivers/net/ethernet/cadence/macb_main.c >>> +++ b/drivers/net/ethernet/cadence/macb_main.c >>> @@ -1042,6 +1042,10 @@ static dma_addr_t macb_get_addr(struct macb *bp, >>> struct macb_dma_desc *desc) >>> } >>> #endif >>> addr |= MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, desc->addr)); >>> +#ifdef CONFIG_MACB_USE_HWSTAMP >>> + if (bp->hw_dma_cap & HW_DMA_CAP_PTP) >>> + addr &= ~GEM_BIT(DMA_RXVALID_OFFSET); >>> +#endif >>> return addr; >>> } >> >> I think this version is slightly worse because it adds an unconditional >> if statement, which can be removed with certain config options. >> I can master a version with a helper function, if it's preferable. >> >> But if you like this one, it's fine too, let me know, I'll send an updated >> version. > > Yup, IMHO this looks better. More likely that someone reading the code > will spot the trickiness. Ok with this version, even if adding to the hot path is really a big concern for our low-end CPUs (ARM9 and Cortex-A5). > I suspect we could clear that bit unconditionally, if the branch is > a concern. The code seems to assume that buffers it gets are 8B aligned > already, regardless of CONFIG_MACB_USE_HWSTAMP. > > Drivers commonly save the DMA address to a SW ring (here I think > rx_skbuff plays this role but only holds single ptr per entry) > so that they don't have to access potentially uncached descriptor > ring. But that'd be too large of a change for a fix. Ok with that statement: Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com> Thanks, best regards, Nicolas
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h index c1fc91c97cee..1b330f7cfc09 100644 --- a/drivers/net/ethernet/cadence/macb.h +++ b/drivers/net/ethernet/cadence/macb.h @@ -826,8 +826,13 @@ struct macb_dma_desc_ptp { #define MACB_RX_USED_SIZE 1 #define MACB_RX_WRAP_OFFSET 1 #define MACB_RX_WRAP_SIZE 1 +#ifdef MACB_EXT_DESC +#define MACB_RX_WADDR_OFFSET 3 +#define MACB_RX_WADDR_SIZE 29 +#else #define MACB_RX_WADDR_OFFSET 2 #define MACB_RX_WADDR_SIZE 30 +#endif #define MACB_RX_FRMLEN_OFFSET 0 #define MACB_RX_FRMLEN_SIZE 12
For quite some time we were chasing a bug which looked like a sudden permanent failure of networking and mmc on some of our devices. The bug was very sensitive to any software changes and even more to any kernel debug options. Finally we got a setup where the problem was reproducible with CONFIG_DMA_API_DEBUG=y and it revealed the issue with the rx dma: [ 16.992082] ------------[ cut here ]------------ [ 16.996779] DMA-API: macb ff0b0000.ethernet: device driver tries to free DMA memory it has not allocated [device address=0x0000000875e3e244] [size=1536 bytes] [ 17.011049] WARNING: CPU: 0 PID: 85 at kernel/dma/debug.c:1011 check_unmap+0x6a0/0x900 [ 17.018977] Modules linked in: xxxxx [ 17.038823] CPU: 0 PID: 85 Comm: irq/55-8000f000 Not tainted 5.4.0 #28 [ 17.045345] Hardware name: xxxxx [ 17.049528] pstate: 60000005 (nZCv daif -PAN -UAO) [ 17.054322] pc : check_unmap+0x6a0/0x900 [ 17.058243] lr : check_unmap+0x6a0/0x900 [ 17.062163] sp : ffffffc010003c40 [ 17.065470] x29: ffffffc010003c40 x28: 000000004000c03c [ 17.070783] x27: ffffffc010da7048 x26: ffffff8878e38800 [ 17.076095] x25: ffffff8879d22810 x24: ffffffc010003cc8 [ 17.081407] x23: 0000000000000000 x22: ffffffc010a08750 [ 17.086719] x21: ffffff8878e3c7c0 x20: ffffffc010acb000 [ 17.092032] x19: 0000000875e3e244 x18: 0000000000000010 [ 17.097343] x17: 0000000000000000 x16: 0000000000000000 [ 17.102647] x15: ffffff8879e4a988 x14: 0720072007200720 [ 17.107959] x13: 0720072007200720 x12: 0720072007200720 [ 17.113261] x11: 0720072007200720 x10: 0720072007200720 [ 17.118565] x9 : 0720072007200720 x8 : 000000000000022d [ 17.123869] x7 : 0000000000000015 x6 : 0000000000000098 [ 17.129173] x5 : 0000000000000000 x4 : 0000000000000000 [ 17.134475] x3 : 00000000ffffffff x2 : ffffffc010a1d370 [ 17.139778] x1 : b420c9d75d27bb00 x0 : 0000000000000000 [ 17.145082] Call trace: [ 17.147524] check_unmap+0x6a0/0x900 [ 17.151091] debug_dma_unmap_page+0x88/0x90 [ 17.155266] gem_rx+0x114/0x2f0 [ 17.158396] macb_poll+0x58/0x100 [ 17.161705] net_rx_action+0x118/0x400 [ 17.165445] __do_softirq+0x138/0x36c [ 17.169100] irq_exit+0x98/0xc0 [ 17.172234] __handle_domain_irq+0x64/0xc0 [ 17.176320] gic_handle_irq+0x5c/0xc0 [ 17.179974] el1_irq+0xb8/0x140 [ 17.183109] xiic_process+0x5c/0xe30 [ 17.186677] irq_thread_fn+0x28/0x90 [ 17.190244] irq_thread+0x208/0x2a0 [ 17.193724] kthread+0x130/0x140 [ 17.196945] ret_from_fork+0x10/0x20 [ 17.200510] ---[ end trace 7240980785f81d6f ]--- [ 237.021490] ------------[ cut here ]------------ [ 237.026129] DMA-API: exceeded 7 overlapping mappings of cacheline 0x0000000021d79e7b [ 237.033886] WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:499 add_dma_entry+0x214/0x240 [ 237.041802] Modules linked in: xxxxx [ 237.061637] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 5.4.0 #28 [ 237.068941] Hardware name: xxxxx [ 237.073116] pstate: 80000085 (Nzcv daIf -PAN -UAO) [ 237.077900] pc : add_dma_entry+0x214/0x240 [ 237.081986] lr : add_dma_entry+0x214/0x240 [ 237.086072] sp : ffffffc010003c30 [ 237.089379] x29: ffffffc010003c30 x28: ffffff8878a0be00 [ 237.094683] x27: 0000000000000180 x26: ffffff8878e387c0 [ 237.099987] x25: 0000000000000002 x24: 0000000000000000 [ 237.105290] x23: 000000000000003b x22: ffffffc010a0fa00 [ 237.110594] x21: 0000000021d79e7b x20: ffffffc010abe600 [ 237.115897] x19: 00000000ffffffef x18: 0000000000000010 [ 237.121201] x17: 0000000000000000 x16: 0000000000000000 [ 237.126504] x15: ffffffc010a0fdc8 x14: 0720072007200720 [ 237.131807] x13: 0720072007200720 x12: 0720072007200720 [ 237.137111] x11: 0720072007200720 x10: 0720072007200720 [ 237.142415] x9 : 0720072007200720 x8 : 0000000000000259 [ 237.147718] x7 : 0000000000000001 x6 : 0000000000000000 [ 237.153022] x5 : ffffffc010003a20 x4 : 0000000000000001 [ 237.158325] x3 : 0000000000000006 x2 : 0000000000000007 [ 237.163628] x1 : 8ac721b3a7dc1c00 x0 : 0000000000000000 [ 237.168932] Call trace: [ 237.171373] add_dma_entry+0x214/0x240 [ 237.175115] debug_dma_map_page+0xf8/0x120 [ 237.179203] gem_rx_refill+0x190/0x280 [ 237.182942] gem_rx+0x224/0x2f0 [ 237.186075] macb_poll+0x58/0x100 [ 237.189384] net_rx_action+0x118/0x400 [ 237.193125] __do_softirq+0x138/0x36c [ 237.196780] irq_exit+0x98/0xc0 [ 237.199914] __handle_domain_irq+0x64/0xc0 [ 237.204000] gic_handle_irq+0x5c/0xc0 [ 237.207654] el1_irq+0xb8/0x140 [ 237.210789] arch_cpu_idle+0x40/0x200 [ 237.214444] default_idle_call+0x18/0x30 [ 237.218359] do_idle+0x200/0x280 [ 237.221578] cpu_startup_entry+0x20/0x30 [ 237.225493] rest_init+0xe4/0xf0 [ 237.228713] arch_call_rest_init+0xc/0x14 [ 237.232714] start_kernel+0x47c/0x4a8 [ 237.236367] ---[ end trace 7240980785f81d70 ]--- Lars was fast to find an explanation: according to the datasheet bit 2 of the rx buffer descriptor entry has a different meaning in the extended mode: Address [2] of beginning of buffer, or in extended buffer descriptor mode (DMA configuration register [28] = 1), indicates a valid timestamp in the buffer descriptor entry. The macb driver didn't mask this bit while getting an address and it eventually caused a memory corruption and a dma failure. The problem is resolved by extending the MACB_RX_WADDR_SIZE in the extended mode. Fixes: 7b4296148066 ("net: macb: Add support for PTP timestamps in DMA descriptors") Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev> Co-developed-by: Lars-Peter Clausen <lars@metafoo.de> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> --- drivers/net/ethernet/cadence/macb.h | 5 +++++ 1 file changed, 5 insertions(+)