Messages in this thread | | | Date | Tue, 11 Apr 2023 20:48:50 -0700 | From | Roman Gushchin <> | Subject | Re: [PATCH net] net: macb: fix a memory corruption in extended buffer descriptor mode |
| |
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!
| |