On 27/02/2024 01:40, Niklas Söderlund wrote:
> The RAVB device requires the SKB data to be aligned to 128 bytes. The
> alignment is done by allocating a skb 128 bytes larger than the maximum
> frame size supported by the device and adjusting the headroom to fit the
> requirement.
>
> This code has been refactored a few times and small issues have been
> added along the way. The issues are not harmful but prevents merging
> parts of the Rx code which have been split in two implementations with
> the addition of RZ/G2L support, a device that supports larger frame
> sizes.
>
> This change removes the need for duplicated and somewhat inaccurate
> hardware alignment constrains stored in the hardware information struct
> by creating a helper to handle the allocation of a skb and alignment of
> a skb data.
>
> For the R-Car class of devices the maximum frame size is 4K and each
> descriptor is limited to 2K of data. The current implementation does not
> support split descriptors, this limits the frame size to 2K. The
> current hardware information however records the descriptor size just
> under 2K due to bad understanding of the device when larger MTUs where
> added.
>
> For the RZ/G2L device the maximum frame size is 8K and each descriptor
> is limited to 4K of data. The current hardware information records this
> correctly, but it gets the alignment constrains wrong as just aligns it
> by 128, it does not extend it by 128 bytes to allow the full frame to be
> stored. This works because the RZ/G2L device supports split descriptors
> and allocates each skb to 8K and aligns each 4K descriptor in this
> space.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
After some discussion with Niklas on IRC, I'm dropping my NACK so that
this can hopefully get in to v6.9. I'll have to re-do some of my work,
but it was unlikely that would be ready to go in for v6.9 anyway. So,
here's some review...
> ---
> drivers/net/ethernet/renesas/ravb.h | 1 -
> drivers/net/ethernet/renesas/ravb_main.c | 41 +++++++++++++-----------
> 2 files changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 7f9e8b2c012a..751bb29cd488 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1057,7 +1057,6 @@ struct ravb_hw_info {
> netdev_features_t net_hw_features;
> netdev_features_t net_features;
> int stats_len;
> - size_t max_rx_len;
> u32 tccr_mask;
> u32 rx_max_frame_size;
> unsigned aligned_tx: 1;
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 3c59e2c317c7..6e39d498936f 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -113,12 +113,21 @@ static void ravb_set_rate_rcar(struct net_device *ndev)
> }
> }
>
> -static void ravb_set_buffer_align(struct sk_buff *skb)
> +static struct sk_buff *
> +ravb_alloc_skb(struct net_device *ndev, const struct ravb_hw_info *info)
This function should take an extra `gfp_t gfp_mask` argument since it is
called from two contexts: RX ring initialization where we want regular
allocation, and RX ring refill where we need atomic allocation.
> {
> - u32 reserve = (unsigned long)skb->data & (RAVB_ALIGN - 1);
> + struct sk_buff *skb;
> + u32 reserve;
>
> + skb = netdev_alloc_skb(ndev, info->rx_max_frame_size + RAVB_ALIGN - 1);
Call __netdev_alloc_skb() instead with the gfp_mask argument.
> + if (!skb)
> + return NULL;
> +
> + reserve = (unsigned long)skb->data & (RAVB_ALIGN - 1);
> if (reserve)
> skb_reserve(skb, RAVB_ALIGN - reserve);
> +
> + return skb;
> }
>
> /* Get MAC address from the MAC address registers
> @@ -251,7 +260,7 @@ static void ravb_rx_ring_free_gbeth(struct net_device *ndev, int q)
> le32_to_cpu(desc->dptr)))
> dma_unmap_single(ndev->dev.parent,
> le32_to_cpu(desc->dptr),
> - GBETH_RX_BUFF_MAX,
> + priv->info->rx_max_frame_size,
> DMA_FROM_DEVICE);
> }
> ring_size = sizeof(struct ravb_rx_desc) * (priv->num_rx_ring[q] + 1);
> @@ -276,7 +285,7 @@ static void ravb_rx_ring_free_rcar(struct net_device *ndev, int q)
> le32_to_cpu(desc->dptr)))
> dma_unmap_single(ndev->dev.parent,
> le32_to_cpu(desc->dptr),
> - RX_BUF_SZ,
> + priv->info->rx_max_frame_size,
> DMA_FROM_DEVICE);
> }
> ring_size = sizeof(struct ravb_ex_rx_desc) *
> @@ -342,7 +351,7 @@ static void ravb_rx_ring_format_gbeth(struct net_device *ndev, int q)
> rx_desc = &priv->rx_ring[q].desc[i];
> rx_desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
> dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
> - GBETH_RX_BUFF_MAX,
> + priv->info->rx_max_frame_size,
> DMA_FROM_DEVICE);
> /* We just set the data size to 0 for a failed mapping which
> * should prevent DMA from happening...
> @@ -372,7 +381,7 @@ static void ravb_rx_ring_format_rcar(struct net_device *ndev, int q)
> rx_desc = &priv->rx_ring[q].ex_desc[i];
> rx_desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
> dma_addr = dma_map_single(ndev->dev.parent, priv->rx_skb[q][i]->data,
> - RX_BUF_SZ,
> + priv->info->rx_max_frame_size,
> DMA_FROM_DEVICE);
> /* We just set the data size to 0 for a failed mapping which
> * should prevent DMA from happening...
> @@ -476,10 +485,9 @@ static int ravb_ring_init(struct net_device *ndev, int q)
> goto error;
>
> for (i = 0; i < priv->num_rx_ring[q]; i++) {
> - skb = __netdev_alloc_skb(ndev, info->max_rx_len, GFP_KERNEL);
> + skb = ravb_alloc_skb(ndev, info);
Add GFP_KERNEL as the gfp_mask argument.
> if (!skb)
> goto error;
> - ravb_set_buffer_align(skb);
> priv->rx_skb[q][i] = skb;
> }
>
> @@ -805,7 +813,8 @@ static struct sk_buff *ravb_get_skb_gbeth(struct net_device *ndev, int entry,
> skb = priv->rx_skb[RAVB_BE][entry];
> priv->rx_skb[RAVB_BE][entry] = NULL;
> dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
> - ALIGN(GBETH_RX_BUFF_MAX, 16), DMA_FROM_DEVICE);
> + ALIGN(priv->info->rx_max_frame_size, 16),
> + DMA_FROM_DEVICE);
>
> return skb;
> }
> @@ -912,13 +921,12 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
> desc->ds_cc = cpu_to_le16(GBETH_RX_DESC_DATA_SIZE);
>
> if (!priv->rx_skb[q][entry]) {
> - skb = netdev_alloc_skb(ndev, info->max_rx_len);
> + skb = ravb_alloc_skb(ndev, info);
Add GFP_ATOMIC as the gfp_mask argument.
> if (!skb)
> break;
> - ravb_set_buffer_align(skb);
> dma_addr = dma_map_single(ndev->dev.parent,
> skb->data,
> - GBETH_RX_BUFF_MAX,
> + priv->info->rx_max_frame_size,
> DMA_FROM_DEVICE);
> skb_checksum_none_assert(skb);
> /* We just set the data size to 0 for a failed mapping
> @@ -992,7 +1000,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> skb = priv->rx_skb[q][entry];
> priv->rx_skb[q][entry] = NULL;
> dma_unmap_single(ndev->dev.parent, le32_to_cpu(desc->dptr),
> - RX_BUF_SZ,
> + priv->info->rx_max_frame_size,
> DMA_FROM_DEVICE);
> get_ts &= (q == RAVB_NC) ?
> RAVB_RXTSTAMP_TYPE_V2_L2_EVENT :
> @@ -1028,10 +1036,9 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> desc->ds_cc = cpu_to_le16(RX_BUF_SZ);
>
> if (!priv->rx_skb[q][entry]) {
> - skb = netdev_alloc_skb(ndev, info->max_rx_len);
> + skb = ravb_alloc_skb(ndev, info);
Add GFP_ATOMIC as the gfp_mask argument.
> if (!skb)
> break; /* Better luck next round. */
> - ravb_set_buffer_align(skb);
> dma_addr = dma_map_single(ndev->dev.parent, skb->data,
> le16_to_cpu(desc->ds_cc),
> DMA_FROM_DEVICE);
> @@ -2682,7 +2689,6 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
> .net_hw_features = NETIF_F_RXCSUM,
> .net_features = NETIF_F_RXCSUM,
> .stats_len = ARRAY_SIZE(ravb_gstrings_stats),
> - .max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
> .tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
> .rx_max_frame_size = SZ_2K,
> .internal_delay = 1,
> @@ -2708,7 +2714,6 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
> .net_hw_features = NETIF_F_RXCSUM,
> .net_features = NETIF_F_RXCSUM,
> .stats_len = ARRAY_SIZE(ravb_gstrings_stats),
> - .max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
> .tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
> .rx_max_frame_size = SZ_2K,
> .aligned_tx = 1,
> @@ -2731,7 +2736,6 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
> .net_hw_features = NETIF_F_RXCSUM,
> .net_features = NETIF_F_RXCSUM,
> .stats_len = ARRAY_SIZE(ravb_gstrings_stats),
> - .max_rx_len = RX_BUF_SZ + RAVB_ALIGN - 1,
> .tccr_mask = TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
> .rx_max_frame_size = SZ_2K,
> .multi_irqs = 1,
> @@ -2756,7 +2760,6 @@ static const struct ravb_hw_info gbeth_hw_info = {
> .net_hw_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
> .net_features = NETIF_F_RXCSUM | NETIF_F_HW_CSUM,
> .stats_len = ARRAY_SIZE(ravb_gstrings_stats_gbeth),
> - .max_rx_len = ALIGN(GBETH_RX_BUFF_MAX, RAVB_ALIGN),
> .tccr_mask = TCCR_TSRQ0,
> .rx_max_frame_size = SZ_8K,
> .aligned_tx = 1,
Looks ok other than the above comments. I'll try to do some testing
tomorrow.
Thanks,
--
Paul BarkerAttachment:
OpenPGP_0x27F4B3459F002257.asc
Description: OpenPGP public key
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
