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

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

[Nikita Yushchenko]

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.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 8649b3e90edb..4d4b5d44c4e7 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -256,8 +256,7 @@ static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
 		struct ravb_rx_desc *desc = &priv->gbeth_rx_ring[i];
 
-		if (!dma_mapping_error(ndev->dev.parent,
-				       le32_to_cpu(desc->dptr)))
+		if (le16_to_cpu(desc->ds_cc) != 0)
 			dma_unmap_single(ndev->dev.parent,
 					 le32_to_cpu(desc->dptr),
 					 GBETH_RX_BUFF_MAX,
@@ -281,8 +280,7 @@ static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
 	for (i = 0; i < priv->num_rx_ring[q]; i++) {
 		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
 
-		if (!dma_mapping_error(ndev->dev.parent,
-				       le32_to_cpu(desc->dptr)))
+		if (le16_to_cpu(desc->ds_cc) != 0)
 			dma_unmap_single(ndev->dev.parent,
 					 le32_to_cpu(desc->dptr),
 					 RX_BUF_SZ,
@@ -1949,7 +1947,7 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	struct ravb_tstamp_skb *ts_skb;
 	struct ravb_tx_desc *desc;
 	unsigned long flags;
-	u32 dma_addr;
+	dma_addr_t dma_addr;
 	void *buffer;
 	u32 entry;
 	u32 len;
-- 
2.39.2

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

[Denis Kirjanov]

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


> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 8649b3e90edb..4d4b5d44c4e7 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -256,8 +256,7 @@ static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
>  		struct ravb_rx_desc *desc = &priv->gbeth_rx_ring[i];
>  
> -		if (!dma_mapping_error(ndev->dev.parent,
> -				       le32_to_cpu(desc->dptr)))
> +		if (le16_to_cpu(desc->ds_cc) != 0)
>  			dma_unmap_single(ndev->dev.parent,
>  					 le32_to_cpu(desc->dptr),
>  					 GBETH_RX_BUFF_MAX,
> @@ -281,8 +280,7 @@ static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
>  		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
>  
> -		if (!dma_mapping_error(ndev->dev.parent,
> -				       le32_to_cpu(desc->dptr)))
> +		if (le16_to_cpu(desc->ds_cc) != 0)
>  			dma_unmap_single(ndev->dev.parent,
>  					 le32_to_cpu(desc->dptr),
>  					 RX_BUF_SZ,
> @@ -1949,7 +1947,7 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	struct ravb_tstamp_skb *ts_skb;
>  	struct ravb_tx_desc *desc;
>  	unsigned long flags;
> -	u32 dma_addr;
> +	dma_addr_t dma_addr;
>  	void *buffer;
>  	u32 entry;
>  	u32 len;

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

[Nikita Yushchenko]

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

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.
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.

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

[Florian Fainelli]

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

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

[Sergey Shtylyov]

On 1/12/24 8:06 AM, 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,

   This one is on the TX path, in ravb_start_xmit()... :-/

> - do not use dma_mapping_error() against 32-bit descriptor field when
>   checking if unmap is needed, check for zero size instead.

   This one is on the RX path indeed...
   I suggest that you split this patch to the RX/TX path patches, as it fixes different issues.

> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>

   You forgot to specify the Fixes tag (we'd need two of those,
one per patch)...

> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 8649b3e90edb..4d4b5d44c4e7 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -256,8 +256,7 @@ static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
>  		struct ravb_rx_desc *desc = &priv->gbeth_rx_ring[i];
>  
> -		if (!dma_mapping_error(ndev->dev.parent,
> -				       le32_to_cpu(desc->dptr)))
> +		if (le16_to_cpu(desc->ds_cc) != 0)
>  			dma_unmap_single(ndev->dev.parent,
>  					 le32_to_cpu(desc->dptr),
>  					 GBETH_RX_BUFF_MAX,
> @@ -281,8 +280,7 @@ static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
>  	for (i = 0; i < priv->num_rx_ring[q]; i++) {
>  		struct ravb_ex_rx_desc *desc = &priv->rx_ring[q][i];
>  
> -		if (!dma_mapping_error(ndev->dev.parent,
> -				       le32_to_cpu(desc->dptr)))
> +		if (le16_to_cpu(desc->ds_cc) != 0)
>  			dma_unmap_single(ndev->dev.parent,
>  					 le32_to_cpu(desc->dptr),
>  					 RX_BUF_SZ,

   For these hunks it will be:

Fixes: a47b70ea86bd ("ravb: unmap descriptors when freeing rings")

> @@ -1949,7 +1947,7 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	struct ravb_tstamp_skb *ts_skb;
>  	struct ravb_tx_desc *desc;
>  	unsigned long flags;
> -	u32 dma_addr;
> +	dma_addr_t dma_addr;
>  	void *buffer;
>  	u32 entry;
>  	u32 len;

   And for this hunk it will be:

Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")

MBR, Sergey