Received-SPF: pass (google.com: domain of linux-kernel+bounces-25055-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <e1a9f5cf-d935-4754-9e8c-d34fcae000ed@gmail.com>
Date: Fri, 12 Jan 2024 14:08:08 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] net: ethernet: ravb: fix dma mapping failure handling
Content-Language: en-US
To: Nikita Yushchenko <nikita.yoush@cogentembedded.com>,
 Denis Kirjanov <dkirjanov@suse.de>, "David S. Miller" <davem@davemloft.net>,
 Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
Cc: Sergey Shtylyov <s.shtylyov@omp.ru>,
 Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>,
 Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
 Wolfram Sang <wsa+renesas@sang-engineering.com>,
 =?UTF-8?Q?Uwe_Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>,
 netdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240112050639.405784-1-nikita.yoush@cogentembedded.com>
 <64deebbd-93d0-47dc-835e-f719655e076c@suse.de>
 <804a4586-1909-44ee-a40c-d9cb615f75ad@cogentembedded.com>
From: Florian Fainelli <f.fainelli@gmail.com>
In-Reply-To: <804a4586-1909-44ee-a40c-d9cb615f75ad@cogentembedded.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

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