lkml.org 
[lkml]   [2024]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] net: ethernet: ravb: fix dma mapping failure handling
From
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


\
 
 \ /
  Last update: 2024-01-12 23:08    [W:0.046 / U:0.620 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site