| Message ID | 1460879338-7671-2-git-send-email-b.galvani@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On 04/17/2016 09:48 AM, Beniamino Galvani wrote: > All members of the DMA descriptor must be 32-bit, even on 64-bit > architectures: change the type to u32 to ensure this. Also, fix > other warnings. > > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> > --- > drivers/net/designware.c | 59 ++++++++++++++++++++++++++---------------------- > drivers/net/designware.h | 4 ++-- > 2 files changed, 34 insertions(+), 29 deletions(-) > > diff --git a/drivers/net/designware.c b/drivers/net/designware.c > index ca58f34..2eda461 100644 > --- a/drivers/net/designware.c > +++ b/drivers/net/designware.c > @@ -98,8 +98,8 @@ static void tx_descs_init(struct dw_eth_dev *priv) > > for (idx = 0; idx < CONFIG_TX_DESCR_NUM; idx++) { > desc_p = &desc_table_p[idx]; > - desc_p->dmamac_addr = &txbuffs[idx * CONFIG_ETH_BUFSIZE]; > - desc_p->dmamac_next = &desc_table_p[idx + 1]; > + desc_p->dmamac_addr = (ulong)&txbuffs[idx * CONFIG_ETH_BUFSIZE]; > + desc_p->dmamac_next = (ulong)&desc_table_p[idx + 1]; Why don't you use u32 instead of ulong ? The u32 is well defined. DTTO all over the place. btw just curious, but what will happen if the descriptors get allocated in area above 4GiB ? Will the code silently corrupt memory by discarding the top bits in the descriptor pointer? [...] > diff --git a/drivers/net/designware.h b/drivers/net/designware.h > index ed6344c..d48df7b 100644 > --- a/drivers/net/designware.h > +++ b/drivers/net/designware.h > @@ -110,8 +110,8 @@ struct eth_dma_regs { > struct dmamacdescr { > u32 txrx_status; > u32 dmamac_cntl; > - void *dmamac_addr; > - struct dmamacdescr *dmamac_next; > + u32 dmamac_addr; > + u32 dmamac_next; > } __aligned(ARCH_DMA_MINALIGN); > > /* >
On Sun, Apr 17, 2016 at 11:56:58AM +0200, Marek Vasut wrote: > > - desc_p->dmamac_addr = &txbuffs[idx * CONFIG_ETH_BUFSIZE]; > > - desc_p->dmamac_next = &desc_table_p[idx + 1]; > > + desc_p->dmamac_addr = (ulong)&txbuffs[idx * CONFIG_ETH_BUFSIZE]; > > + desc_p->dmamac_next = (ulong)&desc_table_p[idx + 1]; > > Why don't you use u32 instead of ulong ? The u32 is well defined. > DTTO all over the place. &txbuffs[idx * CONFIG_ETH_BUFSIZE] is a pointer (and hence has the size of a ulong) and casting it to u32 would give a warning on 64 bit archs ("cast from pointer to integer of different size"). > btw just curious, but what will happen if the descriptors get allocated > in area above 4GiB ? Will the code silently corrupt memory by discarding > the top bits in the descriptor pointer? No, if the driver private structure (which contains buffers and descriptors) is above 4GiB, designware_initialize() will complain and return an error. Beniamino
On 04/17/2016 01:14 PM, Beniamino Galvani wrote: > On Sun, Apr 17, 2016 at 11:56:58AM +0200, Marek Vasut wrote: >>> - desc_p->dmamac_addr = &txbuffs[idx * CONFIG_ETH_BUFSIZE]; >>> - desc_p->dmamac_next = &desc_table_p[idx + 1]; >>> + desc_p->dmamac_addr = (ulong)&txbuffs[idx * CONFIG_ETH_BUFSIZE]; >>> + desc_p->dmamac_next = (ulong)&desc_table_p[idx + 1]; >> >> Why don't you use u32 instead of ulong ? The u32 is well defined. >> DTTO all over the place. > > &txbuffs[idx * CONFIG_ETH_BUFSIZE] is a pointer (and hence has the > size of a ulong) and casting it to u32 would give a warning on 64 bit > archs ("cast from pointer to integer of different size"). Will cast to uintptr_t and then to u32 help ? It's just a feeling, but casting to ulong just to circumvent compiler warning does not sound right. >> btw just curious, but what will happen if the descriptors get allocated >> in area above 4GiB ? Will the code silently corrupt memory by discarding >> the top bits in the descriptor pointer? > > No, if the driver private structure (which contains buffers and > descriptors) is above 4GiB, designware_initialize() will complain and > return an error. Which code checks that ? > Beniamino >
On Sun, Apr 17, 2016 at 10:59:11PM +0200, Marek Vasut wrote: > On 04/17/2016 01:14 PM, Beniamino Galvani wrote: > > On Sun, Apr 17, 2016 at 11:56:58AM +0200, Marek Vasut wrote: > >>> - desc_p->dmamac_addr = &txbuffs[idx * CONFIG_ETH_BUFSIZE]; > >>> - desc_p->dmamac_next = &desc_table_p[idx + 1]; > >>> + desc_p->dmamac_addr = (ulong)&txbuffs[idx * CONFIG_ETH_BUFSIZE]; > >>> + desc_p->dmamac_next = (ulong)&desc_table_p[idx + 1]; > >> > >> Why don't you use u32 instead of ulong ? The u32 is well defined. > >> DTTO all over the place. > > > > &txbuffs[idx * CONFIG_ETH_BUFSIZE] is a pointer (and hence has the > > size of a ulong) and casting it to u32 would give a warning on 64 bit > > archs ("cast from pointer to integer of different size"). > > Will cast to uintptr_t and then to u32 help ? Note that uintptr_t is defined as ulong and the second cast to u32 is not needed because C does not require casts between arithmetic types. So I don't see much difference. > It's just a feeling, but casting to ulong just to circumvent compiler > warning does not sound right. It seems fine to me, the (ulong) is needed to cast the pointer to an arithmetic type of equivalent size which then can be assigned to an u32 variable. > >> btw just curious, but what will happen if the descriptors get allocated > >> in area above 4GiB ? Will the code silently corrupt memory by discarding > >> the top bits in the descriptor pointer? > > > > No, if the driver private structure (which contains buffers and > > descriptors) is above 4GiB, designware_initialize() will complain and > > return an error. > > Which code checks that ? + if ((unsigned long long)priv + sizeof(*priv) > (1ULL << 32)) { + printf("designware: buffers are outside DMA memory\n"); + return -EINVAL; + } + Beniamino
On 04/18/2016 12:57 PM, Beniamino Galvani wrote: > On Sun, Apr 17, 2016 at 10:59:11PM +0200, Marek Vasut wrote: >> On 04/17/2016 01:14 PM, Beniamino Galvani wrote: >>> On Sun, Apr 17, 2016 at 11:56:58AM +0200, Marek Vasut wrote: >>>>> - desc_p->dmamac_addr = &txbuffs[idx * CONFIG_ETH_BUFSIZE]; >>>>> - desc_p->dmamac_next = &desc_table_p[idx + 1]; >>>>> + desc_p->dmamac_addr = (ulong)&txbuffs[idx * CONFIG_ETH_BUFSIZE]; >>>>> + desc_p->dmamac_next = (ulong)&desc_table_p[idx + 1]; >>>> Why don't you use u32 instead of ulong ? The u32 is well defined. >>>> DTTO all over the place. >>> &txbuffs[idx * CONFIG_ETH_BUFSIZE] is a pointer (and hence has the >>> size of a ulong) and casting it to u32 would give a warning on 64 bit >>> archs ("cast from pointer to integer of different size"). >> Will cast to uintptr_t and then to u32 help ? > Note that uintptr_t is defined as ulong and the second cast to u32 is > not needed because C does not require casts between arithmetic > types. So I don't see much difference. > >> It's just a feeling, but casting to ulong just to circumvent compiler >> warning does not sound right. > It seems fine to me, the (ulong) is needed to cast the pointer to an > arithmetic type of equivalent size which then can be assigned to an > u32 variable. > >>>> btw just curious, but what will happen if the descriptors get allocated >>>> in area above 4GiB ? Will the code silently corrupt memory by discarding >>>> the top bits in the descriptor pointer? >>> No, if the driver private structure (which contains buffers and >>> descriptors) is above 4GiB, designware_initialize() will complain and >>> return an error. >> Which code checks that ? > + if ((unsigned long long)priv + sizeof(*priv) > (1ULL << 32)) { > + printf("designware: buffers are outside DMA memory\n"); > + return -EINVAL; > + } > + Hmm, this is going to get very interesting with efi_loader support. By default we allocate memory at the highest possible free address, so payloads will probably (unless they specify limits) have their buffers above 32bit on this platform. If we now deny any DMA to them, we basically break I/O access. Could you by any chance just use a bounce buffer? Alex
Am 17.04.2016 um 09:48 schrieb Beniamino Galvani: > All members of the DMA descriptor must be 32-bit, even on 64-bit > architectures: change the type to u32 to ensure this. Also, fix > other warnings. > > Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> > --- > drivers/net/designware.c | 59 ++++++++++++++++++++++++++---------------------- > drivers/net/designware.h | 4 ++-- > 2 files changed, 34 insertions(+), 29 deletions(-) > > diff --git a/drivers/net/designware.c b/drivers/net/designware.c > index ca58f34..2eda461 100644 > --- a/drivers/net/designware.c > +++ b/drivers/net/designware.c [...] > @@ -488,6 +486,11 @@ int designware_initialize(ulong base_addr, u32 interface) > return -ENOMEM; > } > > + if ((unsigned long long)priv + sizeof(*priv) > (1ULL << 32)) { >=? Regards, Andreas > + printf("designware: buffers are outside DMA memory\n"); > + return -EINVAL; > + } > + > memset(dev, 0, sizeof(struct eth_device)); > memset(priv, 0, sizeof(struct dw_eth_dev)); > [snip]
On Mon, Apr 18, 2016 at 01:06:37PM +0200, Alexander Graf wrote: > Hmm, this is going to get very interesting with efi_loader support. By > default we allocate memory at the highest possible free address, so payloads > will probably (unless they specify limits) have their buffers above 32bit on > this platform. If we now deny any DMA to them, we basically break I/O > access. I'm not familiar with efi_loader, but on this platform the physical RAM is within the 32bit memory range, so I don't think a workaround is needed. And I guess probably it's the same for the other 64bit ARM SoC using this driver. BTW, I see that another driver (sunxi_mmc) also truncates the upper 32 bits of addresses on 64bit platforms. Maybe this issue should be addresses in a generic way? > Could you by any chance just use a bounce buffer? Do you have any suggestions on how to do it? Are there any primitives in u-boot to request memory from low addresses? Beniamino
On 18.04.16 23:38, Beniamino Galvani wrote: > On Mon, Apr 18, 2016 at 01:06:37PM +0200, Alexander Graf wrote: >> Hmm, this is going to get very interesting with efi_loader support. By >> default we allocate memory at the highest possible free address, so payloads >> will probably (unless they specify limits) have their buffers above 32bit on >> this platform. If we now deny any DMA to them, we basically break I/O >> access. > > I'm not familiar with efi_loader, but on this platform the physical > RAM is within the 32bit memory range, so I don't think a workaround is > needed. And I guess probably it's the same for the other 64bit ARM SoC > using this driver. So if RAM is always within the lower 32bits, then we don't have a problem. > BTW, I see that another driver (sunxi_mmc) also truncates the upper 32 > bits of addresses on 64bit platforms. Maybe this issue should be > addresses in a generic way? The only 64bit sunxi platform (A64) also only has 32bit physical RAM addresses. >> Could you by any chance just use a bounce buffer? > > Do you have any suggestions on how to do it? Are there any primitives > in u-boot to request memory from low addresses? Thinking about this I don't think we have the memory reservation logistics to maintain a good bounce buffer. You could create a global array in bss that you read to / write from, but I'm not sure it's really worth it. At the end of the day, if you know that your platform can only ever do 32bit DMA to a physical address range that's only 32bits, it's perfectly ok IMHO. We only have a problem if you have a platform that has RAM above 4G and can only do DMA to 32bit addresses. Alex
On Mon, Apr 18, 2016 at 01:55:55PM +0200, Andreas Färber wrote: > > + if ((unsigned long long)priv + sizeof(*priv) > (1ULL << 32)) { > > >=? I think ">" is correct, the (unfortunate) case priv + size == (1 << 32) is still acceptable because the last byte used by the structure would be (priv + size - 1). Beniamino
On Mon, Apr 18, 2016 at 5:57 AM, Beniamino Galvani <b.galvani@gmail.com> wrote: > On Sun, Apr 17, 2016 at 10:59:11PM +0200, Marek Vasut wrote: >> On 04/17/2016 01:14 PM, Beniamino Galvani wrote: >> > On Sun, Apr 17, 2016 at 11:56:58AM +0200, Marek Vasut wrote: >> >>> - desc_p->dmamac_addr = &txbuffs[idx * CONFIG_ETH_BUFSIZE]; >> >>> - desc_p->dmamac_next = &desc_table_p[idx + 1]; >> >>> + desc_p->dmamac_addr = (ulong)&txbuffs[idx * CONFIG_ETH_BUFSIZE]; >> >>> + desc_p->dmamac_next = (ulong)&desc_table_p[idx + 1]; >> >> >> >> Why don't you use u32 instead of ulong ? The u32 is well defined. >> >> DTTO all over the place. >> > >> > &txbuffs[idx * CONFIG_ETH_BUFSIZE] is a pointer (and hence has the >> > size of a ulong) and casting it to u32 would give a warning on 64 bit >> > archs ("cast from pointer to integer of different size"). >> >> Will cast to uintptr_t and then to u32 help ? > > Note that uintptr_t is defined as ulong and the second cast to u32 is > not needed because C does not require casts between arithmetic > types. So I don't see much difference. > >> It's just a feeling, but casting to ulong just to circumvent compiler >> warning does not sound right. > > It seems fine to me, the (ulong) is needed to cast the pointer to an > arithmetic type of equivalent size which then can be assigned to an > u32 variable. Sorry, didn't notice that you already had this conversation in a previous version. >> >> btw just curious, but what will happen if the descriptors get allocated >> >> in area above 4GiB ? Will the code silently corrupt memory by discarding >> >> the top bits in the descriptor pointer? >> > >> > No, if the driver private structure (which contains buffers and >> > descriptors) is above 4GiB, designware_initialize() will complain and >> > return an error. >> >> Which code checks that ? > > + if ((unsigned long long)priv + sizeof(*priv) > (1ULL << 32)) { > + printf("designware: buffers are outside DMA memory\n"); > + return -EINVAL; > + } > + > > Beniamino > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
diff --git a/drivers/net/designware.c b/drivers/net/designware.c index ca58f34..2eda461 100644 --- a/drivers/net/designware.c +++ b/drivers/net/designware.c @@ -98,8 +98,8 @@ static void tx_descs_init(struct dw_eth_dev *priv) for (idx = 0; idx < CONFIG_TX_DESCR_NUM; idx++) { desc_p = &desc_table_p[idx]; - desc_p->dmamac_addr = &txbuffs[idx * CONFIG_ETH_BUFSIZE]; - desc_p->dmamac_next = &desc_table_p[idx + 1]; + desc_p->dmamac_addr = (ulong)&txbuffs[idx * CONFIG_ETH_BUFSIZE]; + desc_p->dmamac_next = (ulong)&desc_table_p[idx + 1]; #if defined(CONFIG_DW_ALTDESCRIPTOR) desc_p->txrx_status &= ~(DESC_TXSTS_TXINT | DESC_TXSTS_TXLAST | @@ -117,11 +117,11 @@ static void tx_descs_init(struct dw_eth_dev *priv) } /* Correcting the last pointer of the chain */ - desc_p->dmamac_next = &desc_table_p[0]; + desc_p->dmamac_next = (ulong)&desc_table_p[0]; /* Flush all Tx buffer descriptors at once */ - flush_dcache_range((unsigned int)priv->tx_mac_descrtable, - (unsigned int)priv->tx_mac_descrtable + + flush_dcache_range((ulong)priv->tx_mac_descrtable, + (ulong)priv->tx_mac_descrtable + sizeof(priv->tx_mac_descrtable)); writel((ulong)&desc_table_p[0], &dma_p->txdesclistaddr); @@ -142,13 +142,12 @@ static void rx_descs_init(struct dw_eth_dev *priv) * Otherwise there's a chance to get some of them flushed in RAM when * GMAC is already pushing data to RAM via DMA. This way incoming from * GMAC data will be corrupted. */ - flush_dcache_range((unsigned int)rxbuffs, (unsigned int)rxbuffs + - RX_TOTAL_BUFSIZE); + flush_dcache_range((ulong)rxbuffs, (ulong)rxbuffs + RX_TOTAL_BUFSIZE); for (idx = 0; idx < CONFIG_RX_DESCR_NUM; idx++) { desc_p = &desc_table_p[idx]; - desc_p->dmamac_addr = &rxbuffs[idx * CONFIG_ETH_BUFSIZE]; - desc_p->dmamac_next = &desc_table_p[idx + 1]; + desc_p->dmamac_addr = (ulong)&rxbuffs[idx * CONFIG_ETH_BUFSIZE]; + desc_p->dmamac_next = (ulong)&desc_table_p[idx + 1]; desc_p->dmamac_cntl = (MAC_MAX_FRAME_SZ & DESC_RXCTRL_SIZE1MASK) | @@ -158,11 +157,11 @@ static void rx_descs_init(struct dw_eth_dev *priv) } /* Correcting the last pointer of the chain */ - desc_p->dmamac_next = &desc_table_p[0]; + desc_p->dmamac_next = (ulong)&desc_table_p[0]; /* Flush all Rx buffer descriptors at once */ - flush_dcache_range((unsigned int)priv->rx_mac_descrtable, - (unsigned int)priv->rx_mac_descrtable + + flush_dcache_range((ulong)priv->rx_mac_descrtable, + (ulong)priv->rx_mac_descrtable + sizeof(priv->rx_mac_descrtable)); writel((ulong)&desc_table_p[0], &dma_p->rxdesclistaddr); @@ -290,12 +289,11 @@ static int _dw_eth_send(struct dw_eth_dev *priv, void *packet, int length) struct eth_dma_regs *dma_p = priv->dma_regs_p; u32 desc_num = priv->tx_currdescnum; struct dmamacdescr *desc_p = &priv->tx_mac_descrtable[desc_num]; - uint32_t desc_start = (uint32_t)desc_p; - uint32_t desc_end = desc_start + + ulong desc_start = (ulong)desc_p; + ulong desc_end = desc_start + roundup(sizeof(*desc_p), ARCH_DMA_MINALIGN); - uint32_t data_start = (uint32_t)desc_p->dmamac_addr; - uint32_t data_end = data_start + - roundup(length, ARCH_DMA_MINALIGN); + ulong data_start = desc_p->dmamac_addr; + ulong data_end = data_start + roundup(length, ARCH_DMA_MINALIGN); /* * Strictly we only need to invalidate the "txrx_status" field * for the following check, but on some platforms we cannot @@ -312,7 +310,7 @@ static int _dw_eth_send(struct dw_eth_dev *priv, void *packet, int length) return -EPERM; } - memcpy(desc_p->dmamac_addr, packet, length); + memcpy((void *)data_start, packet, length); /* Flush data to be sent */ flush_dcache_range(data_start, data_end); @@ -352,11 +350,11 @@ static int _dw_eth_recv(struct dw_eth_dev *priv, uchar **packetp) u32 status, desc_num = priv->rx_currdescnum; struct dmamacdescr *desc_p = &priv->rx_mac_descrtable[desc_num]; int length = -EAGAIN; - uint32_t desc_start = (uint32_t)desc_p; - uint32_t desc_end = desc_start + + ulong desc_start = (ulong)desc_p; + ulong desc_end = desc_start + roundup(sizeof(*desc_p), ARCH_DMA_MINALIGN); - uint32_t data_start = (uint32_t)desc_p->dmamac_addr; - uint32_t data_end; + ulong data_start = desc_p->dmamac_addr; + ulong data_end; /* Invalidate entire buffer descriptor */ invalidate_dcache_range(desc_start, desc_end); @@ -372,7 +370,7 @@ static int _dw_eth_recv(struct dw_eth_dev *priv, uchar **packetp) /* Invalidate received data */ data_end = data_start + roundup(length, ARCH_DMA_MINALIGN); invalidate_dcache_range(data_start, data_end); - *packetp = desc_p->dmamac_addr; + *packetp = (uchar *)(ulong)desc_p->dmamac_addr; } return length; @@ -382,8 +380,8 @@ static int _dw_free_pkt(struct dw_eth_dev *priv) { u32 desc_num = priv->rx_currdescnum; struct dmamacdescr *desc_p = &priv->rx_mac_descrtable[desc_num]; - uint32_t desc_start = (uint32_t)desc_p; - uint32_t desc_end = desc_start + + ulong desc_start = (ulong)desc_p; + ulong desc_end = desc_start + roundup(sizeof(*desc_p), ARCH_DMA_MINALIGN); /* @@ -488,6 +486,11 @@ int designware_initialize(ulong base_addr, u32 interface) return -ENOMEM; } + if ((unsigned long long)priv + sizeof(*priv) > (1ULL << 32)) { + printf("designware: buffers are outside DMA memory\n"); + return -EINVAL; + } + memset(dev, 0, sizeof(struct eth_device)); memset(priv, 0, sizeof(struct dw_eth_dev)); @@ -583,6 +586,7 @@ static int designware_eth_probe(struct udevice *dev) struct eth_pdata *pdata = dev_get_platdata(dev); struct dw_eth_dev *priv = dev_get_priv(dev); u32 iobase = pdata->iobase; + ulong ioaddr; int ret; #ifdef CONFIG_DM_PCI @@ -601,8 +605,9 @@ static int designware_eth_probe(struct udevice *dev) #endif debug("%s, iobase=%x, priv=%p\n", __func__, iobase, priv); - priv->mac_regs_p = (struct eth_mac_regs *)iobase; - priv->dma_regs_p = (struct eth_dma_regs *)(iobase + DW_DMA_BASE_OFFSET); + ioaddr = iobase; + priv->mac_regs_p = (struct eth_mac_regs *)ioaddr; + priv->dma_regs_p = (struct eth_dma_regs *)(ioaddr + DW_DMA_BASE_OFFSET); priv->interface = pdata->phy_interface; priv->max_speed = pdata->max_speed; diff --git a/drivers/net/designware.h b/drivers/net/designware.h index ed6344c..d48df7b 100644 --- a/drivers/net/designware.h +++ b/drivers/net/designware.h @@ -110,8 +110,8 @@ struct eth_dma_regs { struct dmamacdescr { u32 txrx_status; u32 dmamac_cntl; - void *dmamac_addr; - struct dmamacdescr *dmamac_next; + u32 dmamac_addr; + u32 dmamac_next; } __aligned(ARCH_DMA_MINALIGN); /*
All members of the DMA descriptor must be 32-bit, even on 64-bit architectures: change the type to u32 to ensure this. Also, fix other warnings. Signed-off-by: Beniamino Galvani <b.galvani@gmail.com> --- drivers/net/designware.c | 59 ++++++++++++++++++++++++++---------------------- drivers/net/designware.h | 4 ++-- 2 files changed, 34 insertions(+), 29 deletions(-)