Messages in this thread | | | Date | Fri, 12 Jan 2024 14:08:08 -0800 | Subject | Re: [PATCH] net: ethernet: ravb: fix dma mapping failure handling | From | Florian Fainelli <> |
| |
On 1/12/24 01:04, Nikita Yushchenko wrote: > > > 12.01.2024 14:56, Denis Kirjanov wrote: >> >> >> On 1/12/24 08:06, Nikita Yushchenko wrote: >>> dma_mapping_error() depends on getting full 64-bit dma_addr_t and does >>> not work correctly if 32-bit value is passed instead. >>> >>> Fix handling of dma_map_single() failures on Rx ring entries: >>> - do not store return value of dma_map_signle() in 32-bit variable, >>> - do not use dma_mapping_error() against 32-bit descriptor field when >>> checking if unmap is needed, check for zero size instead. >> >> Hmm, something is wrong here since you're mixing DMA api and forced >> 32bit values. >> if dma uses 32bit addresses then dma_addr_t need only be 32 bits wide > > dma_addr_t is arch-wide type and it is 64bit on arm64
Correct, does not mean all of the bits will be used, nor that there is not an offset, see below.
> > Still, some devices use 32-bit dma addresses. > Proper setting of dma masks and/of configuring iommu ensures that in no > error case, dma address fits into 32 bits.
Yes, because dma_addr_t must be sized to the maximum supportable DMA address in any given system, hence it is 64-bit for a 64-bit architecture. If someone had a system with 32-bit DMA addressing limitation, they could technically introduce a Kconfig option to narrow dma_addr_t, not that this should ever be done. Anyway, I digress.
> Still, in error case dma_map_single() returns ~((dma_addr_t)0) which > uses fill dma_addr_t width and gets corrupted if assigned to 32-bit > value, then later call to dma_mapping_error() does not recognize it. The > patch fixes exactly this issue.
Your patch is actually fine, but you might have to write a lot more about it to tell the reviewers that it is fine.
At the very least you should explain that in case of DMA mapping failure by ravb_rx_ring_format_gbeth() and ravb_rx_ring_format_rcar(), the dsecriptor's ds_cc field is written to 0 to denote a mapping failure.
Note that we will still write a dma_addr_t cookie that corresponds to an error, but this may be OK, because the hardware looks for a ds_cc != 0 to determine whether to DMA the packet into memory or not.
Because of the convention established in ravb_rx_ring_format_gbeth() and ravb_rx_ring_format_rcar(), checking for ds_cc == 0 to denote a mapping error in ravb_rx_ring_free_gbeth() and ravb_rx_ring_free_rcar() is an acceptable way of checking for a valid mapping.
What is however not valid, is that again, we use desc->dptr and pass that to dma_unmap_single() which would expect the non-truncated dma_addr_t. Again, probably works by design, chance, whatever, but is not supposed to be done that way.
It looks like the hardware is limited to 32-bit of DMA addressing, and assumes that the dma_addr_t cookie is 0-indexed, which may very well be the case, even with 64-bit SoCs thanks to an IOMMU.
It would feel a lot more comfortable if there was an actual check on the upper 32-bits of dma_addr_t being zero, and issue a big fat warning in case they are not.
Thanks! -- Florian
| |