Message-ID: <e1a9f5cf-d935-4754-9e8c-d34fcae000ed@gmail.com>
Date: Fri, 12 Jan 2024 14:08:08 -0800
From: Florian Fainelli <f.fainelli@...il.com>
To: Nikita Yushchenko <nikita.yoush@...entembedded.com>,
 Denis Kirjanov <dkirjanov@...e.de>, "David S. Miller" <davem@...emloft.net>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Cc: Sergey Shtylyov <s.shtylyov@....ru>,
 Claudiu Beznea <claudiu.beznea.uj@...renesas.com>,
 Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
 Wolfram Sang <wsa+renesas@...g-engineering.com>,
 Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
 netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: ethernet: ravb: fix dma mapping failure handling

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

Powered by blists - more mailing lists