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
