Re: [PATCH] net: ethernet: ravb: fix dma mapping failure handling

From: Florian Fainelli
Date: Fri Jan 12 2024 - 17:08:24 EST


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