| Message ID | 20240112050639.405784-1-nikita.yoush@cogentembedded.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ethernet: ravb: fix dma mapping failure handling | expand |
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;
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.
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
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!
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;
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(-)