[PATCH] net: ravb: Fix wrong dma_unmap_single() calls in ring unmapping

[PATCH] net: ravb: Fix wrong dma_unmap_single() calls in ring unmapping

[Nikita Yushchenko]

When unmapping ring entries on Rx ring release, ravb driver needs to
unmap only those entries that have been mapped successfully.

To check if an entry needs to be unmapped, currently the address stored
inside descriptor is passed to dma_mapping_error() call. But, address
field inside descriptor is 32-bit, while dma_mapping_error() is
implemented by comparing it's argument with DMA_MAPPING_ERROR constant
that is 64-bit when dma_addr_t is 64-bit. So the comparison gets wrong,
resulting into ravb driver calling dma_unnmap_single() for 0xffffffff
address.

When the ring entries are mapped, in case of mapping failure the driver
sets descriptor's size field to zero (which is a signal to hardware to
not use this descriptor). Fix ring unmapping to detect if an entry needs
to be unmapped by checking for zero size field.

Fixes: a47b70ea86bd ("ravb: unmap descriptors when freeing rings")
Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 0e3731f50fc2..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,
-- 
2.39.2

Re: [PATCH] net: ravb: Fix wrong dma_unmap_single() calls in ring unmapping

[Sergey Shtylyov]

On 1/13/24 7:47 AM, Nikita Yushchenko wrote:

> When unmapping ring entries on Rx ring release, ravb driver needs to
> unmap only those entries that have been mapped successfully.
> 
> To check if an entry needs to be unmapped, currently the address stored
> inside descriptor is passed to dma_mapping_error() call. But, address
> field inside descriptor is 32-bit, while dma_mapping_error() is
> implemented by comparing it's argument with DMA_MAPPING_ERROR constant
> that is 64-bit when dma_addr_t is 64-bit. So the comparison gets wrong,
> resulting into ravb driver calling dma_unnmap_single() for 0xffffffff

   It's dma_unmap_single(). :-)

> address.
> 
> When the ring entries are mapped, in case of mapping failure the driver
> sets descriptor's size field to zero (which is a signal to hardware to
> not use this descriptor). Fix ring unmapping to detect if an entry needs
> to be unmapped by checking for zero size field.
> 
> Fixes: a47b70ea86bd ("ravb: unmap descriptors when freeing rings")
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>

Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru>

[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 0e3731f50fc2..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)

   It's not that != 0 or le16_to_cpu() are necessary here but we're on
the little-endian platforms anyways...

[...]
> @@ -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,

MBR, Sergey

Re: [PATCH] net: ravb: Fix wrong dma_unmap_single() calls in ring unmapping

[Florian Fainelli]

On 1/12/2024 8:47 PM, Nikita Yushchenko wrote:
> When unmapping ring entries on Rx ring release, ravb driver needs to
> unmap only those entries that have been mapped successfully.
> 
> To check if an entry needs to be unmapped, currently the address stored
> inside descriptor is passed to dma_mapping_error() call. But, address
> field inside descriptor is 32-bit, while dma_mapping_error() is
> implemented by comparing it's argument with DMA_MAPPING_ERROR constant
> that is 64-bit when dma_addr_t is 64-bit. So the comparison gets wrong,
> resulting into ravb driver calling dma_unnmap_single() for 0xffffffff
> address.

I would still spell out explicitly that a failed mapping from 
ravb_rx_ring_format_gbeth() and ravb_rx_ring_format_rcar() results in 
writing the ds_cc descriptor field to 0.

With that fixed and the typo spotted by Sergey, you may add:

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH] net: ravb: Fix wrong dma_unmap_single() calls in ring unmapping

[Nikita Yushchenko]

>> +		if (le16_to_cpu(desc->ds_cc) != 0)
> 
>     It's not that != 0 or le16_to_cpu() are necessary here but we're on
> the little-endian platforms anyways...

Let's leave optimizing this out to compiler.

In the code, I'd vote for always having explicit conversion operation when reading __le16 value from memory.

Nikita