| Message ID | 20240113044721.481131-1-nikita.yoush@cogentembedded.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ravb: Fix wrong dma_unmap_single() calls in ring unmapping | expand |
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
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>
>> + 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
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,
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(-)