[PATCH 1/3] net: ravb: Make DBAT entry count configurable per-SoC

[PATCH 1/3] net: ravb: Make DBAT entry count configurable per-SoC

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The number of CDARq (Current Descriptor Address Register) registers is not
fixed to 22 across all SoC variants. For example, the GBETH implementation
uses only two entries. Hardcoding the value leads to incorrect resource
allocation on such platforms.

Pass the DBAT entry count through the per-SoC hardware info struct and use
it during probe instead of relying on a fixed constant. This ensures
correct descriptor table sizing and initialization across different SoCs.

Fixes: feab85c7ccea ("ravb: Add support for RZ/G2L SoC")
Cc: stable@vger.kernel.org
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      | 2 +-
 drivers/net/ethernet/renesas/ravb_main.c | 9 +++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index 7b48060c250b..d65cd83ddd16 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1017,7 +1017,6 @@ enum CSR2_BIT {
 #define CSR2_CSUM_ENABLE (CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4 | \
 			  CSR2_RTCP6 | CSR2_RUDP6 | CSR2_RICMP6)
 
-#define DBAT_ENTRY_NUM	22
 #define RX_QUEUE_OFFSET	4
 #define NUM_RX_QUEUE	2
 #define NUM_TX_QUEUE	2
@@ -1062,6 +1061,7 @@ struct ravb_hw_info {
 	u32 rx_max_frame_size;
 	u32 rx_buffer_size;
 	u32 rx_desc_size;
+	u32 dbat_entry_num;
 	unsigned aligned_tx: 1;
 	unsigned coalesce_irqs:1;	/* Needs software IRQ coalescing */
 
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 9d3bd65b85ff..69d382e8757d 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2694,6 +2694,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
 	.rx_buffer_size = SZ_2K +
 			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
 	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
+	.dbat_entry_num = 22,
 	.aligned_tx = 1,
 	.gptp = 1,
 	.nc_queues = 1,
@@ -2717,6 +2718,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
 	.rx_buffer_size = SZ_2K +
 			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
 	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
+	.dbat_entry_num = 22,
 	.internal_delay = 1,
 	.tx_counters = 1,
 	.multi_irqs = 1,
@@ -2743,6 +2745,7 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
 	.rx_buffer_size = SZ_2K +
 			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
 	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
+	.dbat_entry_num = 22,
 	.internal_delay = 1,
 	.tx_counters = 1,
 	.multi_irqs = 1,
@@ -2769,6 +2772,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
 	.rx_buffer_size = SZ_2K +
 			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
 	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
+	.dbat_entry_num = 22,
 	.multi_irqs = 1,
 	.err_mgmt_irqs = 1,
 	.gptp = 1,
@@ -2794,6 +2798,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
 	.rx_max_frame_size = SZ_8K,
 	.rx_buffer_size = SZ_2K,
 	.rx_desc_size = sizeof(struct ravb_rx_desc),
+	.dbat_entry_num = 2,
 	.aligned_tx = 1,
 	.coalesce_irqs = 1,
 	.tx_counters = 1,
@@ -3025,7 +3030,7 @@ static int ravb_probe(struct platform_device *pdev)
 	ravb_parse_delay_mode(np, ndev);
 
 	/* Allocate descriptor base address table */
-	priv->desc_bat_size = sizeof(struct ravb_desc) * DBAT_ENTRY_NUM;
+	priv->desc_bat_size = sizeof(struct ravb_desc) * info->dbat_entry_num;
 	priv->desc_bat = dma_alloc_coherent(ndev->dev.parent, priv->desc_bat_size,
 					    &priv->desc_bat_dma, GFP_KERNEL);
 	if (!priv->desc_bat) {
@@ -3035,7 +3040,7 @@ static int ravb_probe(struct platform_device *pdev)
 		error = -ENOMEM;
 		goto out_rpm_put;
 	}
-	for (q = RAVB_BE; q < DBAT_ENTRY_NUM; q++)
+	for (q = RAVB_BE; q < info->dbat_entry_num; q++)
 		priv->desc_bat[q].die_dt = DT_EOS;
 
 	/* Initialise HW timestamp list */
-- 
2.43.0

Re: [PATCH 1/3] net: ravb: Make DBAT entry count configurable per-SoC

[Niklas Söderlund]

Hi Prabhakar,

Thanks for your work.

On 2025-10-15 16:00:24 +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> The number of CDARq (Current Descriptor Address Register) registers is not
> fixed to 22 across all SoC variants. For example, the GBETH implementation
> uses only two entries. Hardcoding the value leads to incorrect resource
> allocation on such platforms.
> 
> Pass the DBAT entry count through the per-SoC hardware info struct and use
> it during probe instead of relying on a fixed constant. This ensures
> correct descriptor table sizing and initialization across different SoCs.
> 
> Fixes: feab85c7ccea ("ravb: Add support for RZ/G2L SoC")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

I have not verified with documentation the setting of 2 for 
gbeth_hw_info. But the change itself is good.

> ---
>  drivers/net/ethernet/renesas/ravb.h      | 2 +-
>  drivers/net/ethernet/renesas/ravb_main.c | 9 +++++++--
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 7b48060c250b..d65cd83ddd16 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1017,7 +1017,6 @@ enum CSR2_BIT {
>  #define CSR2_CSUM_ENABLE (CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4 | \
>  			  CSR2_RTCP6 | CSR2_RUDP6 | CSR2_RICMP6)
>  
> -#define DBAT_ENTRY_NUM	22
>  #define RX_QUEUE_OFFSET	4
>  #define NUM_RX_QUEUE	2
>  #define NUM_TX_QUEUE	2
> @@ -1062,6 +1061,7 @@ struct ravb_hw_info {
>  	u32 rx_max_frame_size;
>  	u32 rx_buffer_size;
>  	u32 rx_desc_size;
> +	u32 dbat_entry_num;

I have been wondering for some time if we shall not start to document 
these fields as they always take so much time to get back to what each 
of them represent. How do you feel about starting a header?

/**
 * dbat_entry_num: Describe me here.
 */

Without, but preferably with, this added.

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

>  	unsigned aligned_tx: 1;
>  	unsigned coalesce_irqs:1;	/* Needs software IRQ coalescing */
>  
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 9d3bd65b85ff..69d382e8757d 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2694,6 +2694,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
>  	.rx_buffer_size = SZ_2K +
>  			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
>  	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> +	.dbat_entry_num = 22,
>  	.aligned_tx = 1,
>  	.gptp = 1,
>  	.nc_queues = 1,
> @@ -2717,6 +2718,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
>  	.rx_buffer_size = SZ_2K +
>  			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
>  	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> +	.dbat_entry_num = 22,
>  	.internal_delay = 1,
>  	.tx_counters = 1,
>  	.multi_irqs = 1,
> @@ -2743,6 +2745,7 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
>  	.rx_buffer_size = SZ_2K +
>  			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
>  	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> +	.dbat_entry_num = 22,
>  	.internal_delay = 1,
>  	.tx_counters = 1,
>  	.multi_irqs = 1,
> @@ -2769,6 +2772,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
>  	.rx_buffer_size = SZ_2K +
>  			  SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
>  	.rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> +	.dbat_entry_num = 22,
>  	.multi_irqs = 1,
>  	.err_mgmt_irqs = 1,
>  	.gptp = 1,
> @@ -2794,6 +2798,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
>  	.rx_max_frame_size = SZ_8K,
>  	.rx_buffer_size = SZ_2K,
>  	.rx_desc_size = sizeof(struct ravb_rx_desc),
> +	.dbat_entry_num = 2,
>  	.aligned_tx = 1,
>  	.coalesce_irqs = 1,
>  	.tx_counters = 1,
> @@ -3025,7 +3030,7 @@ static int ravb_probe(struct platform_device *pdev)
>  	ravb_parse_delay_mode(np, ndev);
>  
>  	/* Allocate descriptor base address table */
> -	priv->desc_bat_size = sizeof(struct ravb_desc) * DBAT_ENTRY_NUM;
> +	priv->desc_bat_size = sizeof(struct ravb_desc) * info->dbat_entry_num;
>  	priv->desc_bat = dma_alloc_coherent(ndev->dev.parent, priv->desc_bat_size,
>  					    &priv->desc_bat_dma, GFP_KERNEL);
>  	if (!priv->desc_bat) {
> @@ -3035,7 +3040,7 @@ static int ravb_probe(struct platform_device *pdev)
>  		error = -ENOMEM;
>  		goto out_rpm_put;
>  	}
> -	for (q = RAVB_BE; q < DBAT_ENTRY_NUM; q++)
> +	for (q = RAVB_BE; q < info->dbat_entry_num; q++)
>  		priv->desc_bat[q].die_dt = DT_EOS;
>  
>  	/* Initialise HW timestamp list */
> -- 
> 2.43.0
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH 1/3] net: ravb: Make DBAT entry count configurable per-SoC

[Lad, Prabhakar]

Hi Niklas,

Thank you for the review.

On Wed, Oct 15, 2025 at 4:36 PM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
>
> Hi Prabhakar,
>
> Thanks for your work.
>
> On 2025-10-15 16:00:24 +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > The number of CDARq (Current Descriptor Address Register) registers is not
> > fixed to 22 across all SoC variants. For example, the GBETH implementation
> > uses only two entries. Hardcoding the value leads to incorrect resource
> > allocation on such platforms.
> >
> > Pass the DBAT entry count through the per-SoC hardware info struct and use
> > it during probe instead of relying on a fixed constant. This ensures
> > correct descriptor table sizing and initialization across different SoCs.
> >
> > Fixes: feab85c7ccea ("ravb: Add support for RZ/G2L SoC")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> I have not verified with documentation the setting of 2 for
> gbeth_hw_info. But the change itself is good.
>
> > ---
> >  drivers/net/ethernet/renesas/ravb.h      | 2 +-
> >  drivers/net/ethernet/renesas/ravb_main.c | 9 +++++++--
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> > index 7b48060c250b..d65cd83ddd16 100644
> > --- a/drivers/net/ethernet/renesas/ravb.h
> > +++ b/drivers/net/ethernet/renesas/ravb.h
> > @@ -1017,7 +1017,6 @@ enum CSR2_BIT {
> >  #define CSR2_CSUM_ENABLE (CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4 | \
> >                         CSR2_RTCP6 | CSR2_RUDP6 | CSR2_RICMP6)
> >
> > -#define DBAT_ENTRY_NUM       22
> >  #define RX_QUEUE_OFFSET      4
> >  #define NUM_RX_QUEUE 2
> >  #define NUM_TX_QUEUE 2
> > @@ -1062,6 +1061,7 @@ struct ravb_hw_info {
> >       u32 rx_max_frame_size;
> >       u32 rx_buffer_size;
> >       u32 rx_desc_size;
> > +     u32 dbat_entry_num;
>
> I have been wondering for some time if we shall not start to document
> these fields as they always take so much time to get back to what each
> of them represent. How do you feel about starting a header?
>
> /**
>  * dbat_entry_num: Describe me here.
>  */
>
I agree, let's take this separately into a different patch as it will
make things easier to backport. What do you think?

Cheers,
Prabhakar

> Without, but preferably with, this added.
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> >       unsigned aligned_tx: 1;
> >       unsigned coalesce_irqs:1;       /* Needs software IRQ coalescing */
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index 9d3bd65b85ff..69d382e8757d 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -2694,6 +2694,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
> >       .rx_buffer_size = SZ_2K +
> >                         SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> >       .rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> > +     .dbat_entry_num = 22,
> >       .aligned_tx = 1,
> >       .gptp = 1,
> >       .nc_queues = 1,
> > @@ -2717,6 +2718,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
> >       .rx_buffer_size = SZ_2K +
> >                         SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> >       .rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> > +     .dbat_entry_num = 22,
> >       .internal_delay = 1,
> >       .tx_counters = 1,
> >       .multi_irqs = 1,
> > @@ -2743,6 +2745,7 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
> >       .rx_buffer_size = SZ_2K +
> >                         SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> >       .rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> > +     .dbat_entry_num = 22,
> >       .internal_delay = 1,
> >       .tx_counters = 1,
> >       .multi_irqs = 1,
> > @@ -2769,6 +2772,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
> >       .rx_buffer_size = SZ_2K +
> >                         SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> >       .rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> > +     .dbat_entry_num = 22,
> >       .multi_irqs = 1,
> >       .err_mgmt_irqs = 1,
> >       .gptp = 1,
> > @@ -2794,6 +2798,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
> >       .rx_max_frame_size = SZ_8K,
> >       .rx_buffer_size = SZ_2K,
> >       .rx_desc_size = sizeof(struct ravb_rx_desc),
> > +     .dbat_entry_num = 2,
> >       .aligned_tx = 1,
> >       .coalesce_irqs = 1,
> >       .tx_counters = 1,
> > @@ -3025,7 +3030,7 @@ static int ravb_probe(struct platform_device *pdev)
> >       ravb_parse_delay_mode(np, ndev);
> >
> >       /* Allocate descriptor base address table */
> > -     priv->desc_bat_size = sizeof(struct ravb_desc) * DBAT_ENTRY_NUM;
> > +     priv->desc_bat_size = sizeof(struct ravb_desc) * info->dbat_entry_num;
> >       priv->desc_bat = dma_alloc_coherent(ndev->dev.parent, priv->desc_bat_size,
> >                                           &priv->desc_bat_dma, GFP_KERNEL);
> >       if (!priv->desc_bat) {
> > @@ -3035,7 +3040,7 @@ static int ravb_probe(struct platform_device *pdev)
> >               error = -ENOMEM;
> >               goto out_rpm_put;
> >       }
> > -     for (q = RAVB_BE; q < DBAT_ENTRY_NUM; q++)
> > +     for (q = RAVB_BE; q < info->dbat_entry_num; q++)
> >               priv->desc_bat[q].die_dt = DT_EOS;
> >
> >       /* Initialise HW timestamp list */
> > --
> > 2.43.0
> >
>
> --
> Kind Regards,
> Niklas Söderlund

Re: [PATCH 1/3] net: ravb: Make DBAT entry count configurable per-SoC

[Niklas Söderlund]

On 2025-10-15 18:05:34 +0100, Lad, Prabhakar wrote:
> Hi Niklas,
> 
> Thank you for the review.
> 
> On Wed, Oct 15, 2025 at 4:36 PM Niklas Söderlund
> <niklas.soderlund@ragnatech.se> wrote:
> >
> > Hi Prabhakar,
> >
> > Thanks for your work.
> >
> > On 2025-10-15 16:00:24 +0100, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > The number of CDARq (Current Descriptor Address Register) registers is not
> > > fixed to 22 across all SoC variants. For example, the GBETH implementation
> > > uses only two entries. Hardcoding the value leads to incorrect resource
> > > allocation on such platforms.
> > >
> > > Pass the DBAT entry count through the per-SoC hardware info struct and use
> > > it during probe instead of relying on a fixed constant. This ensures
> > > correct descriptor table sizing and initialization across different SoCs.
> > >
> > > Fixes: feab85c7ccea ("ravb: Add support for RZ/G2L SoC")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > I have not verified with documentation the setting of 2 for
> > gbeth_hw_info. But the change itself is good.
> >
> > > ---
> > >  drivers/net/ethernet/renesas/ravb.h      | 2 +-
> > >  drivers/net/ethernet/renesas/ravb_main.c | 9 +++++++--
> > >  2 files changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> > > index 7b48060c250b..d65cd83ddd16 100644
> > > --- a/drivers/net/ethernet/renesas/ravb.h
> > > +++ b/drivers/net/ethernet/renesas/ravb.h
> > > @@ -1017,7 +1017,6 @@ enum CSR2_BIT {
> > >  #define CSR2_CSUM_ENABLE (CSR2_RTCP4 | CSR2_RUDP4 | CSR2_RICMP4 | \
> > >                         CSR2_RTCP6 | CSR2_RUDP6 | CSR2_RICMP6)
> > >
> > > -#define DBAT_ENTRY_NUM       22
> > >  #define RX_QUEUE_OFFSET      4
> > >  #define NUM_RX_QUEUE 2
> > >  #define NUM_TX_QUEUE 2
> > > @@ -1062,6 +1061,7 @@ struct ravb_hw_info {
> > >       u32 rx_max_frame_size;
> > >       u32 rx_buffer_size;
> > >       u32 rx_desc_size;
> > > +     u32 dbat_entry_num;
> >
> > I have been wondering for some time if we shall not start to document
> > these fields as they always take so much time to get back to what each
> > of them represent. How do you feel about starting a header?
> >
> > /**
> >  * dbat_entry_num: Describe me here.
> >  */
> >
> I agree, let's take this separately into a different patch as it will
> make things easier to backport. What do you think?

Works for me.

> 
> Cheers,
> Prabhakar
> 
> > Without, but preferably with, this added.
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >
> > >       unsigned aligned_tx: 1;
> > >       unsigned coalesce_irqs:1;       /* Needs software IRQ coalescing */
> > >
> > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > > index 9d3bd65b85ff..69d382e8757d 100644
> > > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > > @@ -2694,6 +2694,7 @@ static const struct ravb_hw_info ravb_gen2_hw_info = {
> > >       .rx_buffer_size = SZ_2K +
> > >                         SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> > >       .rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> > > +     .dbat_entry_num = 22,
> > >       .aligned_tx = 1,
> > >       .gptp = 1,
> > >       .nc_queues = 1,
> > > @@ -2717,6 +2718,7 @@ static const struct ravb_hw_info ravb_gen3_hw_info = {
> > >       .rx_buffer_size = SZ_2K +
> > >                         SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> > >       .rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> > > +     .dbat_entry_num = 22,
> > >       .internal_delay = 1,
> > >       .tx_counters = 1,
> > >       .multi_irqs = 1,
> > > @@ -2743,6 +2745,7 @@ static const struct ravb_hw_info ravb_gen4_hw_info = {
> > >       .rx_buffer_size = SZ_2K +
> > >                         SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> > >       .rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> > > +     .dbat_entry_num = 22,
> > >       .internal_delay = 1,
> > >       .tx_counters = 1,
> > >       .multi_irqs = 1,
> > > @@ -2769,6 +2772,7 @@ static const struct ravb_hw_info ravb_rzv2m_hw_info = {
> > >       .rx_buffer_size = SZ_2K +
> > >                         SKB_DATA_ALIGN(sizeof(struct skb_shared_info)),
> > >       .rx_desc_size = sizeof(struct ravb_ex_rx_desc),
> > > +     .dbat_entry_num = 22,
> > >       .multi_irqs = 1,
> > >       .err_mgmt_irqs = 1,
> > >       .gptp = 1,
> > > @@ -2794,6 +2798,7 @@ static const struct ravb_hw_info gbeth_hw_info = {
> > >       .rx_max_frame_size = SZ_8K,
> > >       .rx_buffer_size = SZ_2K,
> > >       .rx_desc_size = sizeof(struct ravb_rx_desc),
> > > +     .dbat_entry_num = 2,
> > >       .aligned_tx = 1,
> > >       .coalesce_irqs = 1,
> > >       .tx_counters = 1,
> > > @@ -3025,7 +3030,7 @@ static int ravb_probe(struct platform_device *pdev)
> > >       ravb_parse_delay_mode(np, ndev);
> > >
> > >       /* Allocate descriptor base address table */
> > > -     priv->desc_bat_size = sizeof(struct ravb_desc) * DBAT_ENTRY_NUM;
> > > +     priv->desc_bat_size = sizeof(struct ravb_desc) * info->dbat_entry_num;
> > >       priv->desc_bat = dma_alloc_coherent(ndev->dev.parent, priv->desc_bat_size,
> > >                                           &priv->desc_bat_dma, GFP_KERNEL);
> > >       if (!priv->desc_bat) {
> > > @@ -3035,7 +3040,7 @@ static int ravb_probe(struct platform_device *pdev)
> > >               error = -ENOMEM;
> > >               goto out_rpm_put;
> > >       }
> > > -     for (q = RAVB_BE; q < DBAT_ENTRY_NUM; q++)
> > > +     for (q = RAVB_BE; q < info->dbat_entry_num; q++)
> > >               priv->desc_bat[q].die_dt = DT_EOS;
> > >
> > >       /* Initialise HW timestamp list */
> > > --
> > > 2.43.0
> > >
> >
> > --
> > Kind Regards,
> > Niklas Söderlund

-- 
Kind Regards,
Niklas Söderlund

[PATCH 2/3] net: ravb: Allocate correct number of queues based on SoC support

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

On SoCs that only support the best-effort queue and not the network
control queue, calling alloc_etherdev_mqs() with fixed values for
TX/RX queues is not appropriate. Use the nc_queues flag from the
per-SoC match data to determine whether the network control queue
is available, and fall back to a single TX/RX queue when it is not.
This ensures correct queue allocation across all supported SoCs.

Fixes: a92f4f0662bf ("ravb: Add nc_queue to struct ravb_hw_info")
Cc: stable@vger.kernel.org
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 69d382e8757d..a200e205825a 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2926,13 +2926,14 @@ static int ravb_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, PTR_ERR(rstc),
 				     "failed to get cpg reset\n");
 
+	info = of_device_get_match_data(&pdev->dev);
+
 	ndev = alloc_etherdev_mqs(sizeof(struct ravb_private),
-				  NUM_TX_QUEUE, NUM_RX_QUEUE);
+				  info->nc_queues ? NUM_TX_QUEUE : 1,
+				  info->nc_queues ? NUM_RX_QUEUE : 1);
 	if (!ndev)
 		return -ENOMEM;
 
-	info = of_device_get_match_data(&pdev->dev);
-
 	ndev->features = info->net_features;
 	ndev->hw_features = info->net_hw_features;
 	ndev->vlan_features = info->vlan_features;
-- 
2.43.0

Re: [PATCH 2/3] net: ravb: Allocate correct number of queues based on SoC support

[Niklas Söderlund]

Hi Prabhakar,

Thanks for your work.

On 2025-10-15 16:00:25 +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> On SoCs that only support the best-effort queue and not the network
> control queue, calling alloc_etherdev_mqs() with fixed values for
> TX/RX queues is not appropriate. Use the nc_queues flag from the
> per-SoC match data to determine whether the network control queue
> is available, and fall back to a single TX/RX queue when it is not.
> This ensures correct queue allocation across all supported SoCs.
> 
> Fixes: a92f4f0662bf ("ravb: Add nc_queue to struct ravb_hw_info")
> Cc: stable@vger.kernel.org
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 69d382e8757d..a200e205825a 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2926,13 +2926,14 @@ static int ravb_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, PTR_ERR(rstc),
>  				     "failed to get cpg reset\n");
>  
> +	info = of_device_get_match_data(&pdev->dev);
> +
>  	ndev = alloc_etherdev_mqs(sizeof(struct ravb_private),
> -				  NUM_TX_QUEUE, NUM_RX_QUEUE);
> +				  info->nc_queues ? NUM_TX_QUEUE : 1,
> +				  info->nc_queues ? NUM_RX_QUEUE : 1);
>  	if (!ndev)
>  		return -ENOMEM;
>  
> -	info = of_device_get_match_data(&pdev->dev);
> -
>  	ndev->features = info->net_features;
>  	ndev->hw_features = info->net_hw_features;
>  	ndev->vlan_features = info->vlan_features;
> -- 
> 2.43.0
> 

-- 
Kind Regards,
Niklas Söderlund

[PATCH 3/3] net: ravb: Enforce descriptor type ordering to prevent early DMA start

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Ensure TX descriptor type fields are written in a safe order so the DMA
engine does not begin processing a chain before all descriptors are
fully initialised.

For multi-descriptor transmissions the driver writes DT_FEND into the
last descriptor and DT_FSTART into the first. The DMA engine starts
processing when it sees DT_FSTART. If the compiler or CPU reorders the
writes and publishes DT_FSTART before DT_FEND, the DMA can start early
and process an incomplete chain, leading to corrupted transmissions or
DMA errors.

Fix this by writing DT_FEND before the dma_wmb() barrier, executing
dma_wmb() immediately before DT_FSTART (or DT_FSINGLE in the single
descriptor case), and then adding a wmb() after the type updates to
ensure CPU-side ordering before ringing the hardware doorbell.

On an RZ/G2L platform running an RT kernel, this reordering hazard was
observed as TX stalls and timeouts:

  [  372.968431] NETDEV WATCHDOG: end0 (ravb): transmit queue 0 timed out
  [  372.968494] WARNING: CPU: 0 PID: 10 at net/sched/sch_generic.c:467 dev_watchdog+0x4a4/0x4ac
  [  373.969291] ravb 11c20000.ethernet end0: transmit timed out, status 00000000, resetting...

This change enforces the required ordering and prevents the DMA engine
from observing DT_FSTART before the rest of the descriptor chain is
valid.

Fixes: 2f45d1902acf ("ravb: minimize TX data copying")
Cc: stable@vger.kernel.org
Co-developed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index a200e205825a..2a995fa9bfff 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -2211,15 +2211,19 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 		skb_tx_timestamp(skb);
 	}
-	/* Descriptor type must be set after all the above writes */
-	dma_wmb();
+
+	/* For multi-descriptors set DT_FEND before calling dma_wmb() */
 	if (num_tx_desc > 1) {
 		desc->die_dt = DT_FEND;
 		desc--;
-		desc->die_dt = DT_FSTART;
-	} else {
-		desc->die_dt = DT_FSINGLE;
 	}
+
+	/* Descriptor type must be set after all the above writes */
+	dma_wmb();
+	desc->die_dt = (num_tx_desc > 1) ? DT_FSTART : DT_FSINGLE;
+
+	/* Ensure data is written to RAM before initiating DMA transfer */
+	wmb();
 	ravb_modify(ndev, TCCR, TCCR_TSRQ0 << q, TCCR_TSRQ0 << q);
 
 	priv->cur_tx[q] += num_tx_desc;
-- 
2.43.0

Re: [PATCH 3/3] net: ravb: Enforce descriptor type ordering to prevent early DMA start

[Niklas Söderlund]

Hi Prabhakar,

Thanks for your work.

On 2025-10-15 16:00:26 +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Ensure TX descriptor type fields are written in a safe order so the DMA
> engine does not begin processing a chain before all descriptors are
> fully initialised.
> 
> For multi-descriptor transmissions the driver writes DT_FEND into the
> last descriptor and DT_FSTART into the first. The DMA engine starts
> processing when it sees DT_FSTART. If the compiler or CPU reorders the
> writes and publishes DT_FSTART before DT_FEND, the DMA can start early
> and process an incomplete chain, leading to corrupted transmissions or
> DMA errors.
> 
> Fix this by writing DT_FEND before the dma_wmb() barrier, executing
> dma_wmb() immediately before DT_FSTART (or DT_FSINGLE in the single
> descriptor case), and then adding a wmb() after the type updates to
> ensure CPU-side ordering before ringing the hardware doorbell.
> 
> On an RZ/G2L platform running an RT kernel, this reordering hazard was
> observed as TX stalls and timeouts:
> 
>   [  372.968431] NETDEV WATCHDOG: end0 (ravb): transmit queue 0 timed out
>   [  372.968494] WARNING: CPU: 0 PID: 10 at net/sched/sch_generic.c:467 dev_watchdog+0x4a4/0x4ac
>   [  373.969291] ravb 11c20000.ethernet end0: transmit timed out, status 00000000, resetting...
> 
> This change enforces the required ordering and prevents the DMA engine
> from observing DT_FSTART before the rest of the descriptor chain is
> valid.
> 
> Fixes: 2f45d1902acf ("ravb: minimize TX data copying")
> Cc: stable@vger.kernel.org
> Co-developed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index a200e205825a..2a995fa9bfff 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -2211,15 +2211,19 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  
>  		skb_tx_timestamp(skb);
>  	}
> -	/* Descriptor type must be set after all the above writes */
> -	dma_wmb();
> +
> +	/* For multi-descriptors set DT_FEND before calling dma_wmb() */
>  	if (num_tx_desc > 1) {
>  		desc->die_dt = DT_FEND;
>  		desc--;
> -		desc->die_dt = DT_FSTART;
> -	} else {
> -		desc->die_dt = DT_FSINGLE;
>  	}
> +
> +	/* Descriptor type must be set after all the above writes */
> +	dma_wmb();
> +	desc->die_dt = (num_tx_desc > 1) ? DT_FSTART : DT_FSINGLE;

IMHO it's ugly to evaluate num_tx_desc twice. I would rather just open 
code the full steps in each branch of the if above. It would make it 
easier to read and understand.

> +
> +	/* Ensure data is written to RAM before initiating DMA transfer */
> +	wmb();

All of this looks a bit odd, why not just do a single dma_wmb() or wmb() 
before ringing the doorbell? Maybe I'm missing something obvious?

>  	ravb_modify(ndev, TCCR, TCCR_TSRQ0 << q, TCCR_TSRQ0 << q);
>  
>  	priv->cur_tx[q] += num_tx_desc;
> -- 
> 2.43.0
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH 3/3] net: ravb: Enforce descriptor type ordering to prevent early DMA start

[Lad, Prabhakar]

Hi Niklas,

Thank you for the review.

On Wed, Oct 15, 2025 at 4:56 PM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
>
> Hi Prabhakar,
>
> Thanks for your work.
>
> On 2025-10-15 16:00:26 +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Ensure TX descriptor type fields are written in a safe order so the DMA
> > engine does not begin processing a chain before all descriptors are
> > fully initialised.
> >
> > For multi-descriptor transmissions the driver writes DT_FEND into the
> > last descriptor and DT_FSTART into the first. The DMA engine starts
> > processing when it sees DT_FSTART. If the compiler or CPU reorders the
> > writes and publishes DT_FSTART before DT_FEND, the DMA can start early
> > and process an incomplete chain, leading to corrupted transmissions or
> > DMA errors.
> >
> > Fix this by writing DT_FEND before the dma_wmb() barrier, executing
> > dma_wmb() immediately before DT_FSTART (or DT_FSINGLE in the single
> > descriptor case), and then adding a wmb() after the type updates to
> > ensure CPU-side ordering before ringing the hardware doorbell.
> >
> > On an RZ/G2L platform running an RT kernel, this reordering hazard was
> > observed as TX stalls and timeouts:
> >
> >   [  372.968431] NETDEV WATCHDOG: end0 (ravb): transmit queue 0 timed out
> >   [  372.968494] WARNING: CPU: 0 PID: 10 at net/sched/sch_generic.c:467 dev_watchdog+0x4a4/0x4ac
> >   [  373.969291] ravb 11c20000.ethernet end0: transmit timed out, status 00000000, resetting...
> >
> > This change enforces the required ordering and prevents the DMA engine
> > from observing DT_FSTART before the rest of the descriptor chain is
> > valid.
> >
> > Fixes: 2f45d1902acf ("ravb: minimize TX data copying")
> > Cc: stable@vger.kernel.org
> > Co-developed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/net/ethernet/renesas/ravb_main.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > index a200e205825a..2a995fa9bfff 100644
> > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > @@ -2211,15 +2211,19 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> >
> >               skb_tx_timestamp(skb);
> >       }
> > -     /* Descriptor type must be set after all the above writes */
> > -     dma_wmb();
> > +
> > +     /* For multi-descriptors set DT_FEND before calling dma_wmb() */
> >       if (num_tx_desc > 1) {
> >               desc->die_dt = DT_FEND;
> >               desc--;
> > -             desc->die_dt = DT_FSTART;
> > -     } else {
> > -             desc->die_dt = DT_FSINGLE;
> >       }
> > +
> > +     /* Descriptor type must be set after all the above writes */
> > +     dma_wmb();
> > +     desc->die_dt = (num_tx_desc > 1) ? DT_FSTART : DT_FSINGLE;
>
> IMHO it's ugly to evaluate num_tx_desc twice. I would rather just open
> code the full steps in each branch of the if above. It would make it
> easier to read and understand.
>
I did this just to avoid compiler optimizations. With the previous
similar code on 5.10 CIP RT it was observed that the compiler
optimized code in such a way that the DT_FSTART was written first
before DT_FEND while the DMA was active because of which we ran into
DMA issues causing QEF errors.

> > +
> > +     /* Ensure data is written to RAM before initiating DMA transfer */
> > +     wmb();
>
> All of this looks a bit odd, why not just do a single dma_wmb() or wmb()
> before ringing the doorbell? Maybe I'm missing something obvious?
>
This wmb() was mainly added to ensure all the descriptor data is in
RAM. The HW manual for RZ/G1/2, R-Car Gen1/2 and RZ/G2L family
mentions that we need to read back the last written descriptor before
triggering the DMA. Please let me know if you think this can be
handled differently.

Cheers,
Prabhakar

> >       ravb_modify(ndev, TCCR, TCCR_TSRQ0 << q, TCCR_TSRQ0 << q);
> >
> >       priv->cur_tx[q] += num_tx_desc;
> > --
> > 2.43.0
> >
>
> --
> Kind Regards,
> Niklas Söderlund

Re: [PATCH 3/3] net: ravb: Enforce descriptor type ordering to prevent early DMA start

[Niklas Söderlund]

Hello,

On 2025-10-15 18:01:13 +0100, Lad, Prabhakar wrote:
> Hi Niklas,
> 
> Thank you for the review.
> 
> On Wed, Oct 15, 2025 at 4:56 PM Niklas Söderlund
> <niklas.soderlund@ragnatech.se> wrote:
> >
> > Hi Prabhakar,
> >
> > Thanks for your work.
> >
> > On 2025-10-15 16:00:26 +0100, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Ensure TX descriptor type fields are written in a safe order so the DMA
> > > engine does not begin processing a chain before all descriptors are
> > > fully initialised.
> > >
> > > For multi-descriptor transmissions the driver writes DT_FEND into the
> > > last descriptor and DT_FSTART into the first. The DMA engine starts
> > > processing when it sees DT_FSTART. If the compiler or CPU reorders the
> > > writes and publishes DT_FSTART before DT_FEND, the DMA can start early
> > > and process an incomplete chain, leading to corrupted transmissions or
> > > DMA errors.
> > >
> > > Fix this by writing DT_FEND before the dma_wmb() barrier, executing
> > > dma_wmb() immediately before DT_FSTART (or DT_FSINGLE in the single
> > > descriptor case), and then adding a wmb() after the type updates to
> > > ensure CPU-side ordering before ringing the hardware doorbell.
> > >
> > > On an RZ/G2L platform running an RT kernel, this reordering hazard was
> > > observed as TX stalls and timeouts:
> > >
> > >   [  372.968431] NETDEV WATCHDOG: end0 (ravb): transmit queue 0 timed out
> > >   [  372.968494] WARNING: CPU: 0 PID: 10 at net/sched/sch_generic.c:467 dev_watchdog+0x4a4/0x4ac
> > >   [  373.969291] ravb 11c20000.ethernet end0: transmit timed out, status 00000000, resetting...
> > >
> > > This change enforces the required ordering and prevents the DMA engine
> > > from observing DT_FSTART before the rest of the descriptor chain is
> > > valid.
> > >
> > > Fixes: 2f45d1902acf ("ravb: minimize TX data copying")
> > > Cc: stable@vger.kernel.org
> > > Co-developed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  drivers/net/ethernet/renesas/ravb_main.c | 14 +++++++++-----
> > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > > index a200e205825a..2a995fa9bfff 100644
> > > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > > @@ -2211,15 +2211,19 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> > >
> > >               skb_tx_timestamp(skb);
> > >       }
> > > -     /* Descriptor type must be set after all the above writes */
> > > -     dma_wmb();
> > > +
> > > +     /* For multi-descriptors set DT_FEND before calling dma_wmb() */
> > >       if (num_tx_desc > 1) {
> > >               desc->die_dt = DT_FEND;
> > >               desc--;
> > > -             desc->die_dt = DT_FSTART;
> > > -     } else {
> > > -             desc->die_dt = DT_FSINGLE;
> > >       }
> > > +
> > > +     /* Descriptor type must be set after all the above writes */
> > > +     dma_wmb();
> > > +     desc->die_dt = (num_tx_desc > 1) ? DT_FSTART : DT_FSINGLE;
> >
> > IMHO it's ugly to evaluate num_tx_desc twice. I would rather just open
> > code the full steps in each branch of the if above. It would make it
> > easier to read and understand.
> >
> I did this just to avoid compiler optimizations. With the previous
> similar code on 5.10 CIP RT it was observed that the compiler
> optimized code in such a way that the DT_FSTART was written first
> before DT_FEND while the DMA was active because of which we ran into
> DMA issues causing QEF errors.

I was thinking of something like

  /* Descriptor type must be set after all the above writes */
  dma_wmb();

  if (num_tx_desc > 1) {
          desc->die_dt = DT_FEND;
          desc--;
	  /* For multi-descriptors ensure DT_FEND before start
           * TODO: Add explanation about compiler optimised code etc.
           */
          dma_wmb();
          desc->die_dt = DT_FSTART;
  } else {
          desc->die_dt = DT_FSINGLE;
  }


Would make new new special case clearer to understand. And if we figure 
out different way of doing it it's very clear why the second dma_wmb() 
is needed. But after writing it out I'm not so sure anymore, maybe 
adding a temporary variable instead would make it a clearer read.

    u8 die_dt = DT_FSINGLE;

    /* For multi-descriptors ensure DT_FEND before DT_FEND
     * TODO: Add explanation about compiler optimised code etc.
     */
    if (num_tx_desc > 1) {
        desc->die_dt = DT_FEND;
        desc--;
        die_dt = DT_FSTART;
    }

    /* Descriptor type must be set after all the above writes */
    dma_wmb();
    desc->die_dt = die_dt;

I think my main complaint is that evaluating num_tx_desc > 1 multiple 
times makes the code read as stuff was just thrown into the driver until 
a test-case passed without understanding the root cause.

> 
> > > +
> > > +     /* Ensure data is written to RAM before initiating DMA transfer */
> > > +     wmb();
> >
> > All of this looks a bit odd, why not just do a single dma_wmb() or wmb()
> > before ringing the doorbell? Maybe I'm missing something obvious?
> >
> This wmb() was mainly added to ensure all the descriptor data is in
> RAM. The HW manual for RZ/G1/2, R-Car Gen1/2 and RZ/G2L family
> mentions that we need to read back the last written descriptor before
> triggering the DMA. Please let me know if you think this can be
> handled differently.

Have you observed any issues without the wmb() here?

What I'm trying to understand is why a new barrier is needed here when 
it have worked without one before. I'm likely just slow but what I'm 
trying to grasp is the need for the intermittent dma_wmb() above in 
relation to this one.

Should it not be, setup the descriptors, barrier to ensure it's in RAM, 
ring doorbell. The staggered method of setup descriptors, barrier, setup 
more descriptors, barrier, ring doorbell is what confuses me, I think 
:-)

> 
> Cheers,
> Prabhakar
> 
> > >       ravb_modify(ndev, TCCR, TCCR_TSRQ0 << q, TCCR_TSRQ0 << q);
> > >
> > >       priv->cur_tx[q] += num_tx_desc;
> > > --
> > > 2.43.0
> > >
> >
> > --
> > Kind Regards,
> > Niklas Söderlund

-- 
Kind Regards,
Niklas Söderlund

RE: [PATCH 3/3] net: ravb: Enforce descriptor type ordering to prevent early DMA start

[Fabrizio Castro]

Hi Niklas,

Thanks for your feedback!

> From: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Sent: 15 October 2025 18:28
> To: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Cc: Paul Barker <paul@pbarker.dev>; Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>;
> netdev@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-kernel@vger.kernel.org; Biju Das
> <biju.das.jz@bp.renesas.com>; Fabrizio Castro <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com>; stable@vger.kernel.org
> Subject: Re: [PATCH 3/3] net: ravb: Enforce descriptor type ordering to prevent early DMA start
> 
> Hello,
> 
> On 2025-10-15 18:01:13 +0100, Lad, Prabhakar wrote:
> > Hi Niklas,
> >
> > Thank you for the review.
> >
> > On Wed, Oct 15, 2025 at 4:56 PM Niklas Söderlund
> > <niklas.soderlund@ragnatech.se> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > Thanks for your work.
> > >
> > > On 2025-10-15 16:00:26 +0100, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Ensure TX descriptor type fields are written in a safe order so the DMA
> > > > engine does not begin processing a chain before all descriptors are
> > > > fully initialised.
> > > >
> > > > For multi-descriptor transmissions the driver writes DT_FEND into the
> > > > last descriptor and DT_FSTART into the first. The DMA engine starts
> > > > processing when it sees DT_FSTART. If the compiler or CPU reorders the
> > > > writes and publishes DT_FSTART before DT_FEND, the DMA can start early
> > > > and process an incomplete chain, leading to corrupted transmissions or
> > > > DMA errors.
> > > >
> > > > Fix this by writing DT_FEND before the dma_wmb() barrier, executing
> > > > dma_wmb() immediately before DT_FSTART (or DT_FSINGLE in the single
> > > > descriptor case), and then adding a wmb() after the type updates to
> > > > ensure CPU-side ordering before ringing the hardware doorbell.
> > > >
> > > > On an RZ/G2L platform running an RT kernel, this reordering hazard was
> > > > observed as TX stalls and timeouts:
> > > >
> > > >   [  372.968431] NETDEV WATCHDOG: end0 (ravb): transmit queue 0 timed out
> > > >   [  372.968494] WARNING: CPU: 0 PID: 10 at net/sched/sch_generic.c:467 dev_watchdog+0x4a4/0x4ac
> > > >   [  373.969291] ravb 11c20000.ethernet end0: transmit timed out, status 00000000, resetting...
> > > >
> > > > This change enforces the required ordering and prevents the DMA engine
> > > > from observing DT_FSTART before the rest of the descriptor chain is
> > > > valid.
> > > >
> > > > Fixes: 2f45d1902acf ("ravb: minimize TX data copying")
> > > > Cc: stable@vger.kernel.org
> > > > Co-developed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  drivers/net/ethernet/renesas/ravb_main.c | 14 +++++++++-----
> > > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > > > index a200e205825a..2a995fa9bfff 100644
> > > > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > > > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > > > @@ -2211,15 +2211,19 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device
> *ndev)
> > > >
> > > >               skb_tx_timestamp(skb);
> > > >       }
> > > > -     /* Descriptor type must be set after all the above writes */
> > > > -     dma_wmb();
> > > > +
> > > > +     /* For multi-descriptors set DT_FEND before calling dma_wmb() */
> > > >       if (num_tx_desc > 1) {
> > > >               desc->die_dt = DT_FEND;
> > > >               desc--;
> > > > -             desc->die_dt = DT_FSTART;
> > > > -     } else {
> > > > -             desc->die_dt = DT_FSINGLE;
> > > >       }
> > > > +
> > > > +     /* Descriptor type must be set after all the above writes */
> > > > +     dma_wmb();
> > > > +     desc->die_dt = (num_tx_desc > 1) ? DT_FSTART : DT_FSINGLE;
> > >
> > > IMHO it's ugly to evaluate num_tx_desc twice. I would rather just open
> > > code the full steps in each branch of the if above. It would make it
> > > easier to read and understand.
> > >
> > I did this just to avoid compiler optimizations. With the previous
> > similar code on 5.10 CIP RT it was observed that the compiler
> > optimized code in such a way that the DT_FSTART was written first
> > before DT_FEND while the DMA was active because of which we ran into
> > DMA issues causing QEF errors.
> 
> I was thinking of something like
> 
>   /* Descriptor type must be set after all the above writes */
>   dma_wmb();
> 
>   if (num_tx_desc > 1) {
>           desc->die_dt = DT_FEND;
>           desc--;
> 	  /* For multi-descriptors ensure DT_FEND before start
>            * TODO: Add explanation about compiler optimised code etc.
>            */
>           dma_wmb();
>           desc->die_dt = DT_FSTART;
>   } else {
>           desc->die_dt = DT_FSINGLE;
>   }
> 
> 
> Would make new new special case clearer to understand. And if we figure
> out different way of doing it it's very clear why the second dma_wmb()
> is needed. But after writing it out I'm not so sure anymore, maybe
> adding a temporary variable instead would make it a clearer read.
> 
>     u8 die_dt = DT_FSINGLE;
> 
>     /* For multi-descriptors ensure DT_FEND before DT_FEND
>      * TODO: Add explanation about compiler optimised code etc.
>      */
>     if (num_tx_desc > 1) {
>         desc->die_dt = DT_FEND;
>         desc--;
>         die_dt = DT_FSTART;
>     }
> 
>     /* Descriptor type must be set after all the above writes */
>     dma_wmb();
>     desc->die_dt = die_dt;
> 
> I think my main complaint is that evaluating num_tx_desc > 1 multiple
> times makes the code read as stuff was just thrown into the driver until
> a test-case passed without understanding the root cause.

What about the below instead?

  if (num_tx_desc > 1) {
          desc->die_dt = DT_FEND;
          desc--;
	    /* When using multi-descriptors, DT_FEND needs to get written
           * before DT_FSTART, but the compiler may reorder the memory
           * writes in an attempt to optimize the code.
           * Use a dma_wmb() barrier to make sure DT_FEND and DT_FSTART
           * are written exactly in the order shown in the code.
           */
          dma_wmb();
          desc->die_dt = DT_FSTART;
  } else {
	    /* The descriptor type must be set after all the previous writes */
	    dma_wmb();
          desc->die_dt = DT_FSINGLE;
  }

> 
> >
> > > > +
> > > > +     /* Ensure data is written to RAM before initiating DMA transfer */
> > > > +     wmb();
> > >
> > > All of this looks a bit odd, why not just do a single dma_wmb() or wmb()
> > > before ringing the doorbell? Maybe I'm missing something obvious?
> > >
> > This wmb() was mainly added to ensure all the descriptor data is in
> > RAM. The HW manual for RZ/G1/2, R-Car Gen1/2 and RZ/G2L family
> > mentions that we need to read back the last written descriptor before
> > triggering the DMA. Please let me know if you think this can be
> > handled differently.
> 
> Have you observed any issues without the wmb() here?

No, we haven't. We have added it because after further discussions
with the design team, it became clear that the best thing to do
is to wait for all the previous memory writes to have completed fully,
alongside cache, branch prediction, and other operations, so that when
we ring the DMA bell everything is in the right place.

As Prabhakar pointed out, the manual refers to a `read` operation to be
used as a delay, but a wmb() barrier is more accurate and its purposes
is clearer than a random memory read.

Having said that, we haven't noticed any issues without it in practice,
but having it might prevent issues going forward.

The dma_wmb() barrier is still needed for cases where the DMA engine
is already active, therefore writing the descriptor(s) at the wrong
time can lead to error conditions (which we have managed to see, and
which this patch addresses for multi-descriptor cases).

Perhaps this patch should be split into two patches:
1. to address the error we have seen, for use cases where the DMA engine
   is already running (this is regarding dma_wmb())
2. to address the use cases where the DMA engine is not running yet,
   and we want to avoid the possibility of ringing the bell when things
   are not 100% ready.

What do you think?

Cheers,
Fab

> 
> What I'm trying to understand is why a new barrier is needed here when
> it have worked without one before. I'm likely just slow but what I'm
> trying to grasp is the need for the intermittent dma_wmb() above in
> relation to this one.
> 
> Should it not be, setup the descriptors, barrier to ensure it's in RAM,
> ring doorbell. The staggered method of setup descriptors, barrier, setup
> more descriptors, barrier, ring doorbell is what confuses me, I think
> :-)
> 
> >
> > Cheers,
> > Prabhakar
> >
> > > >       ravb_modify(ndev, TCCR, TCCR_TSRQ0 << q, TCCR_TSRQ0 << q);
> > > >
> > > >       priv->cur_tx[q] += num_tx_desc;
> > > > --
> > > > 2.43.0
> > > >
> > >
> > > --
> > > Kind Regards,
> > > Niklas Söderlund
> 
> --
> Kind Regards,
> Niklas Söderlund

Re: [PATCH 3/3] net: ravb: Enforce descriptor type ordering to prevent early DMA start

[niklas.soderlund]

Hi Fabrizio,

On 2025-10-16 12:00:29 +0000, Fabrizio Castro wrote:
> Hi Niklas,
> 
> Thanks for your feedback!
> 
> > From: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Sent: 15 October 2025 18:28
> > To: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> > Cc: Paul Barker <paul@pbarker.dev>; Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller
> > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> > Abeni <pabeni@redhat.com>; Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>;
> > netdev@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-kernel@vger.kernel.org; Biju Das
> > <biju.das.jz@bp.renesas.com>; Fabrizio Castro <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev Lad
> > <prabhakar.mahadev-lad.rj@bp.renesas.com>; stable@vger.kernel.org
> > Subject: Re: [PATCH 3/3] net: ravb: Enforce descriptor type ordering to prevent early DMA start
> > 
> > Hello,
> > 
> > On 2025-10-15 18:01:13 +0100, Lad, Prabhakar wrote:
> > > Hi Niklas,
> > >
> > > Thank you for the review.
> > >
> > > On Wed, Oct 15, 2025 at 4:56 PM Niklas Söderlund
> > > <niklas.soderlund@ragnatech.se> wrote:
> > > >
> > > > Hi Prabhakar,
> > > >
> > > > Thanks for your work.
> > > >
> > > > On 2025-10-15 16:00:26 +0100, Prabhakar wrote:
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > >
> > > > > Ensure TX descriptor type fields are written in a safe order so the DMA
> > > > > engine does not begin processing a chain before all descriptors are
> > > > > fully initialised.
> > > > >
> > > > > For multi-descriptor transmissions the driver writes DT_FEND into the
> > > > > last descriptor and DT_FSTART into the first. The DMA engine starts
> > > > > processing when it sees DT_FSTART. If the compiler or CPU reorders the
> > > > > writes and publishes DT_FSTART before DT_FEND, the DMA can start early
> > > > > and process an incomplete chain, leading to corrupted transmissions or
> > > > > DMA errors.
> > > > >
> > > > > Fix this by writing DT_FEND before the dma_wmb() barrier, executing
> > > > > dma_wmb() immediately before DT_FSTART (or DT_FSINGLE in the single
> > > > > descriptor case), and then adding a wmb() after the type updates to
> > > > > ensure CPU-side ordering before ringing the hardware doorbell.
> > > > >
> > > > > On an RZ/G2L platform running an RT kernel, this reordering hazard was
> > > > > observed as TX stalls and timeouts:
> > > > >
> > > > >   [  372.968431] NETDEV WATCHDOG: end0 (ravb): transmit queue 0 timed out
> > > > >   [  372.968494] WARNING: CPU: 0 PID: 10 at net/sched/sch_generic.c:467 dev_watchdog+0x4a4/0x4ac
> > > > >   [  373.969291] ravb 11c20000.ethernet end0: transmit timed out, status 00000000, resetting...
> > > > >
> > > > > This change enforces the required ordering and prevents the DMA engine
> > > > > from observing DT_FSTART before the rest of the descriptor chain is
> > > > > valid.
> > > > >
> > > > > Fixes: 2f45d1902acf ("ravb: minimize TX data copying")
> > > > > Cc: stable@vger.kernel.org
> > > > > Co-developed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > ---
> > > > >  drivers/net/ethernet/renesas/ravb_main.c | 14 +++++++++-----
> > > > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> > > > > index a200e205825a..2a995fa9bfff 100644
> > > > > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > > > > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > > > > @@ -2211,15 +2211,19 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct net_device
> > *ndev)
> > > > >
> > > > >               skb_tx_timestamp(skb);
> > > > >       }
> > > > > -     /* Descriptor type must be set after all the above writes */
> > > > > -     dma_wmb();
> > > > > +
> > > > > +     /* For multi-descriptors set DT_FEND before calling dma_wmb() */
> > > > >       if (num_tx_desc > 1) {
> > > > >               desc->die_dt = DT_FEND;
> > > > >               desc--;
> > > > > -             desc->die_dt = DT_FSTART;
> > > > > -     } else {
> > > > > -             desc->die_dt = DT_FSINGLE;
> > > > >       }
> > > > > +
> > > > > +     /* Descriptor type must be set after all the above writes */
> > > > > +     dma_wmb();
> > > > > +     desc->die_dt = (num_tx_desc > 1) ? DT_FSTART : DT_FSINGLE;
> > > >
> > > > IMHO it's ugly to evaluate num_tx_desc twice. I would rather just open
> > > > code the full steps in each branch of the if above. It would make it
> > > > easier to read and understand.
> > > >
> > > I did this just to avoid compiler optimizations. With the previous
> > > similar code on 5.10 CIP RT it was observed that the compiler
> > > optimized code in such a way that the DT_FSTART was written first
> > > before DT_FEND while the DMA was active because of which we ran into
> > > DMA issues causing QEF errors.
> > 
> > I was thinking of something like
> > 
> >   /* Descriptor type must be set after all the above writes */
> >   dma_wmb();
> > 
> >   if (num_tx_desc > 1) {
> >           desc->die_dt = DT_FEND;
> >           desc--;
> > 	  /* For multi-descriptors ensure DT_FEND before start
> >            * TODO: Add explanation about compiler optimised code etc.
> >            */
> >           dma_wmb();
> >           desc->die_dt = DT_FSTART;
> >   } else {
> >           desc->die_dt = DT_FSINGLE;
> >   }
> > 
> > 
> > Would make new new special case clearer to understand. And if we figure
> > out different way of doing it it's very clear why the second dma_wmb()
> > is needed. But after writing it out I'm not so sure anymore, maybe
> > adding a temporary variable instead would make it a clearer read.
> > 
> >     u8 die_dt = DT_FSINGLE;
> > 
> >     /* For multi-descriptors ensure DT_FEND before DT_FEND
> >      * TODO: Add explanation about compiler optimised code etc.
> >      */
> >     if (num_tx_desc > 1) {
> >         desc->die_dt = DT_FEND;
> >         desc--;
> >         die_dt = DT_FSTART;
> >     }
> > 
> >     /* Descriptor type must be set after all the above writes */
> >     dma_wmb();
> >     desc->die_dt = die_dt;
> > 
> > I think my main complaint is that evaluating num_tx_desc > 1 multiple
> > times makes the code read as stuff was just thrown into the driver until
> > a test-case passed without understanding the root cause.
> 
> What about the below instead?
> 
>   if (num_tx_desc > 1) {
>           desc->die_dt = DT_FEND;
>           desc--;
> 	    /* When using multi-descriptors, DT_FEND needs to get written
>            * before DT_FSTART, but the compiler may reorder the memory
>            * writes in an attempt to optimize the code.
>            * Use a dma_wmb() barrier to make sure DT_FEND and DT_FSTART
>            * are written exactly in the order shown in the code.
>            */
>           dma_wmb();
>           desc->die_dt = DT_FSTART;
>   } else {
> 	    /* The descriptor type must be set after all the previous writes */
> 	    dma_wmb();
>           desc->die_dt = DT_FSINGLE;
>   }

I like this. It makes the two conditions very clear. Maybe extend the 
first comment a bit with the information you add below. That the 
important thing is that this protects for cases where the DMA engine is 
already running, and it's very important that it do not see a DT_FSTART 
before a DT_FEND is already committed, else it can run amuck. That would 
make the intent super clear.

> 
> > 
> > >
> > > > > +
> > > > > +     /* Ensure data is written to RAM before initiating DMA transfer */
> > > > > +     wmb();
> > > >
> > > > All of this looks a bit odd, why not just do a single dma_wmb() or wmb()
> > > > before ringing the doorbell? Maybe I'm missing something obvious?
> > > >
> > > This wmb() was mainly added to ensure all the descriptor data is in
> > > RAM. The HW manual for RZ/G1/2, R-Car Gen1/2 and RZ/G2L family
> > > mentions that we need to read back the last written descriptor before
> > > triggering the DMA. Please let me know if you think this can be
> > > handled differently.
> > 
> > Have you observed any issues without the wmb() here?
> 
> No, we haven't. We have added it because after further discussions
> with the design team, it became clear that the best thing to do
> is to wait for all the previous memory writes to have completed fully,
> alongside cache, branch prediction, and other operations, so that when
> we ring the DMA bell everything is in the right place.
> 
> As Prabhakar pointed out, the manual refers to a `read` operation to be
> used as a delay, but a wmb() barrier is more accurate and its purposes
> is clearer than a random memory read.
> 
> Having said that, we haven't noticed any issues without it in practice,
> but having it might prevent issues going forward.
> 
> The dma_wmb() barrier is still needed for cases where the DMA engine
> is already active, therefore writing the descriptor(s) at the wrong
> time can lead to error conditions (which we have managed to see, and
> which this patch addresses for multi-descriptor cases).
> 
> Perhaps this patch should be split into two patches:
> 1. to address the error we have seen, for use cases where the DMA engine
>    is already running (this is regarding dma_wmb())
> 2. to address the use cases where the DMA engine is not running yet,
>    and we want to avoid the possibility of ringing the bell when things
>    are not 100% ready.
> 
> What do you think?

More and smaller patches are always a good idea IMHO, makes bisecting 
stuff so much easier.

I still have my doubts about the usefulness of adding a wmb() here.  
Maybe beacause I'm also confused about why it's a wmb() and not a 
dma_wmb(), are not only trying to make sure the DMA is not started 
before we know the last desc->die_dt = {DT_FSTART,DT_FSINGLE} have would 
be visible?

The usefulness of that I can imagine. As if the DMA engine is not 
running and we start it and that write is not visible we could delay 
sending until the doorbell is rang again.

> 
> Cheers,
> Fab
> 
> > 
> > What I'm trying to understand is why a new barrier is needed here when
> > it have worked without one before. I'm likely just slow but what I'm
> > trying to grasp is the need for the intermittent dma_wmb() above in
> > relation to this one.
> > 
> > Should it not be, setup the descriptors, barrier to ensure it's in RAM,
> > ring doorbell. The staggered method of setup descriptors, barrier, setup
> > more descriptors, barrier, ring doorbell is what confuses me, I think
> > :-)
> > 
> > >
> > > Cheers,
> > > Prabhakar
> > >
> > > > >       ravb_modify(ndev, TCCR, TCCR_TSRQ0 << q, TCCR_TSRQ0 << q);
> > > > >
> > > > >       priv->cur_tx[q] += num_tx_desc;
> > > > > --
> > > > > 2.43.0
> > > > >
> > > >
> > > > --
> > > > Kind Regards,
> > > > Niklas Söderlund
> > 
> > --
> > Kind Regards,
> > Niklas Söderlund

-- 
Kind Regards,
Niklas Söderlund

RE: [PATCH 3/3] net: ravb: Enforce descriptor type ordering to prevent early DMA start

[Fabrizio Castro]

Hi Niklas,

Thanks for your feedback!

> From: niklas.soderlund <niklas.soderlund@ragnatech.se>
> Sent: 16 October 2025 13:39
> To: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> Cc: Lad, Prabhakar <prabhakar.csengg@gmail.com>; Paul Barker <paul@pbarker.dev>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>;
> Jakub Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Sergei Shtylyov
> <sergei.shtylyov@cogentembedded.com>; netdev@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-
> kernel@vger.kernel.org; Biju Das <biju.das.jz@bp.renesas.com>; Prabhakar Mahadev Lad
> <prabhakar.mahadev-lad.rj@bp.renesas.com>; stable@vger.kernel.org
> Subject: Re: [PATCH 3/3] net: ravb: Enforce descriptor type ordering to prevent early DMA start
> 
> Hi Fabrizio,
> 
> On 2025-10-16 12:00:29 +0000, Fabrizio Castro wrote:
> > Hi Niklas,
> >
> > Thanks for your feedback!
> >
> > > From: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Sent: 15 October 2025 18:28
> > > To: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> > > Cc: Paul Barker <paul@pbarker.dev>; Andrew Lunn <andrew+netdev@lunn.ch>; David S. Miller
> > > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> > > Abeni <pabeni@redhat.com>; Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>;
> > > netdev@vger.kernel.org; linux-renesas-soc@vger.kernel.org; linux-kernel@vger.kernel.org; Biju Das
> > > <biju.das.jz@bp.renesas.com>; Fabrizio Castro <fabrizio.castro.jz@renesas.com>; Prabhakar Mahadev
> Lad
> > > <prabhakar.mahadev-lad.rj@bp.renesas.com>; stable@vger.kernel.org
> > > Subject: Re: [PATCH 3/3] net: ravb: Enforce descriptor type ordering to prevent early DMA start
> > >
> > > Hello,
> > >
> > > On 2025-10-15 18:01:13 +0100, Lad, Prabhakar wrote:
> > > > Hi Niklas,
> > > >
> > > > Thank you for the review.
> > > >
> > > > On Wed, Oct 15, 2025 at 4:56 PM Niklas Söderlund
> > > > <niklas.soderlund@ragnatech.se> wrote:
> > > > >
> > > > > Hi Prabhakar,
> > > > >
> > > > > Thanks for your work.
> > > > >
> > > > > On 2025-10-15 16:00:26 +0100, Prabhakar wrote:
> > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > >
> > > > > > Ensure TX descriptor type fields are written in a safe order so the DMA
> > > > > > engine does not begin processing a chain before all descriptors are
> > > > > > fully initialised.
> > > > > >
> > > > > > For multi-descriptor transmissions the driver writes DT_FEND into the
> > > > > > last descriptor and DT_FSTART into the first. The DMA engine starts
> > > > > > processing when it sees DT_FSTART. If the compiler or CPU reorders the
> > > > > > writes and publishes DT_FSTART before DT_FEND, the DMA can start early
> > > > > > and process an incomplete chain, leading to corrupted transmissions or
> > > > > > DMA errors.
> > > > > >
> > > > > > Fix this by writing DT_FEND before the dma_wmb() barrier, executing
> > > > > > dma_wmb() immediately before DT_FSTART (or DT_FSINGLE in the single
> > > > > > descriptor case), and then adding a wmb() after the type updates to
> > > > > > ensure CPU-side ordering before ringing the hardware doorbell.
> > > > > >
> > > > > > On an RZ/G2L platform running an RT kernel, this reordering hazard was
> > > > > > observed as TX stalls and timeouts:
> > > > > >
> > > > > >   [  372.968431] NETDEV WATCHDOG: end0 (ravb): transmit queue 0 timed out
> > > > > >   [  372.968494] WARNING: CPU: 0 PID: 10 at net/sched/sch_generic.c:467
> dev_watchdog+0x4a4/0x4ac
> > > > > >   [  373.969291] ravb 11c20000.ethernet end0: transmit timed out, status 00000000,
> resetting...
> > > > > >
> > > > > > This change enforces the required ordering and prevents the DMA engine
> > > > > > from observing DT_FSTART before the rest of the descriptor chain is
> > > > > > valid.
> > > > > >
> > > > > > Fixes: 2f45d1902acf ("ravb: minimize TX data copying")
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Co-developed-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com>
> > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > > ---
> > > > > >  drivers/net/ethernet/renesas/ravb_main.c | 14 +++++++++-----
> > > > > >  1 file changed, 9 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> b/drivers/net/ethernet/renesas/ravb_main.c
> > > > > > index a200e205825a..2a995fa9bfff 100644
> > > > > > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > > > > > +++ b/drivers/net/ethernet/renesas/ravb_main.c
> > > > > > @@ -2211,15 +2211,19 @@ static netdev_tx_t ravb_start_xmit(struct sk_buff *skb, struct
> net_device
> > > *ndev)
> > > > > >
> > > > > >               skb_tx_timestamp(skb);
> > > > > >       }
> > > > > > -     /* Descriptor type must be set after all the above writes */
> > > > > > -     dma_wmb();
> > > > > > +
> > > > > > +     /* For multi-descriptors set DT_FEND before calling dma_wmb() */
> > > > > >       if (num_tx_desc > 1) {
> > > > > >               desc->die_dt = DT_FEND;
> > > > > >               desc--;
> > > > > > -             desc->die_dt = DT_FSTART;
> > > > > > -     } else {
> > > > > > -             desc->die_dt = DT_FSINGLE;
> > > > > >       }
> > > > > > +
> > > > > > +     /* Descriptor type must be set after all the above writes */
> > > > > > +     dma_wmb();
> > > > > > +     desc->die_dt = (num_tx_desc > 1) ? DT_FSTART : DT_FSINGLE;
> > > > >
> > > > > IMHO it's ugly to evaluate num_tx_desc twice. I would rather just open
> > > > > code the full steps in each branch of the if above. It would make it
> > > > > easier to read and understand.
> > > > >
> > > > I did this just to avoid compiler optimizations. With the previous
> > > > similar code on 5.10 CIP RT it was observed that the compiler
> > > > optimized code in such a way that the DT_FSTART was written first
> > > > before DT_FEND while the DMA was active because of which we ran into
> > > > DMA issues causing QEF errors.
> > >
> > > I was thinking of something like
> > >
> > >   /* Descriptor type must be set after all the above writes */
> > >   dma_wmb();
> > >
> > >   if (num_tx_desc > 1) {
> > >           desc->die_dt = DT_FEND;
> > >           desc--;
> > > 	  /* For multi-descriptors ensure DT_FEND before start
> > >            * TODO: Add explanation about compiler optimised code etc.
> > >            */
> > >           dma_wmb();
> > >           desc->die_dt = DT_FSTART;
> > >   } else {
> > >           desc->die_dt = DT_FSINGLE;
> > >   }
> > >
> > >
> > > Would make new new special case clearer to understand. And if we figure
> > > out different way of doing it it's very clear why the second dma_wmb()
> > > is needed. But after writing it out I'm not so sure anymore, maybe
> > > adding a temporary variable instead would make it a clearer read.
> > >
> > >     u8 die_dt = DT_FSINGLE;
> > >
> > >     /* For multi-descriptors ensure DT_FEND before DT_FEND
> > >      * TODO: Add explanation about compiler optimised code etc.
> > >      */
> > >     if (num_tx_desc > 1) {
> > >         desc->die_dt = DT_FEND;
> > >         desc--;
> > >         die_dt = DT_FSTART;
> > >     }
> > >
> > >     /* Descriptor type must be set after all the above writes */
> > >     dma_wmb();
> > >     desc->die_dt = die_dt;
> > >
> > > I think my main complaint is that evaluating num_tx_desc > 1 multiple
> > > times makes the code read as stuff was just thrown into the driver until
> > > a test-case passed without understanding the root cause.
> >
> > What about the below instead?
> >
> >   if (num_tx_desc > 1) {
> >           desc->die_dt = DT_FEND;
> >           desc--;
> > 	    /* When using multi-descriptors, DT_FEND needs to get written
> >            * before DT_FSTART, but the compiler may reorder the memory
> >            * writes in an attempt to optimize the code.
> >            * Use a dma_wmb() barrier to make sure DT_FEND and DT_FSTART
> >            * are written exactly in the order shown in the code.
> >            */
> >           dma_wmb();
> >           desc->die_dt = DT_FSTART;
> >   } else {
> > 	    /* The descriptor type must be set after all the previous writes */
> > 	    dma_wmb();
> >           desc->die_dt = DT_FSINGLE;
> >   }
> 
> I like this. It makes the two conditions very clear. Maybe extend the
> first comment a bit with the information you add below. That the
> important thing is that this protects for cases where the DMA engine is
> already running, and it's very important that it do not see a DT_FSTART
> before a DT_FEND is already committed, else it can run amuck. That would
> make the intent super clear.

Okay then. v2 will contain the above code and we'll expand the comment.
Thanks for the suggestions.

> 
> >
> > >
> > > >
> > > > > > +
> > > > > > +     /* Ensure data is written to RAM before initiating DMA transfer */
> > > > > > +     wmb();
> > > > >
> > > > > All of this looks a bit odd, why not just do a single dma_wmb() or wmb()
> > > > > before ringing the doorbell? Maybe I'm missing something obvious?
> > > > >
> > > > This wmb() was mainly added to ensure all the descriptor data is in
> > > > RAM. The HW manual for RZ/G1/2, R-Car Gen1/2 and RZ/G2L family
> > > > mentions that we need to read back the last written descriptor before
> > > > triggering the DMA. Please let me know if you think this can be
> > > > handled differently.
> > >
> > > Have you observed any issues without the wmb() here?
> >
> > No, we haven't. We have added it because after further discussions
> > with the design team, it became clear that the best thing to do
> > is to wait for all the previous memory writes to have completed fully,
> > alongside cache, branch prediction, and other operations, so that when
> > we ring the DMA bell everything is in the right place.
> >
> > As Prabhakar pointed out, the manual refers to a `read` operation to be
> > used as a delay, but a wmb() barrier is more accurate and its purposes
> > is clearer than a random memory read.
> >
> > Having said that, we haven't noticed any issues without it in practice,
> > but having it might prevent issues going forward.
> >
> > The dma_wmb() barrier is still needed for cases where the DMA engine
> > is already active, therefore writing the descriptor(s) at the wrong
> > time can lead to error conditions (which we have managed to see, and
> > which this patch addresses for multi-descriptor cases).
> >
> > Perhaps this patch should be split into two patches:
> > 1. to address the error we have seen, for use cases where the DMA engine
> >    is already running (this is regarding dma_wmb())
> > 2. to address the use cases where the DMA engine is not running yet,
> >    and we want to avoid the possibility of ringing the bell when things
> >    are not 100% ready.
> >
> > What do you think?
> 
> More and smaller patches are always a good idea IMHO, makes bisecting
> stuff so much easier.
> 
> I still have my doubts about the usefulness of adding a wmb() here.
> Maybe beacause I'm also confused about why it's a wmb() and not a
> dma_wmb(), are not only trying to make sure the DMA is not started
> before we know the last desc->die_dt = {DT_FSTART,DT_FSINGLE} have would
> be visible?

A dma_wmb() should work as well. Perhaps we could add a comment in v2
explaining why we need it, so that if the code between the first and
the second barrier gets amended in the future there is going to be
enough info in the code for people to consider.

We'll send this as a separated patch.

> 
> The usefulness of that I can imagine. As if the DMA engine is not
> running and we start it and that write is not visible we could delay
> sending until the doorbell is rang again.

Perhaps the comment should resemble your point here.

Thanks!

Kind regards,
Fab

> 
> >
> > Cheers,
> > Fab
> >
> > >
> > > What I'm trying to understand is why a new barrier is needed here when
> > > it have worked without one before. I'm likely just slow but what I'm
> > > trying to grasp is the need for the intermittent dma_wmb() above in
> > > relation to this one.
> > >
> > > Should it not be, setup the descriptors, barrier to ensure it's in RAM,
> > > ring doorbell. The staggered method of setup descriptors, barrier, setup
> > > more descriptors, barrier, ring doorbell is what confuses me, I think
> > > :-)
> > >
> > > >
> > > > Cheers,
> > > > Prabhakar
> > > >
> > > > > >       ravb_modify(ndev, TCCR, TCCR_TSRQ0 << q, TCCR_TSRQ0 << q);
> > > > > >
> > > > > >       priv->cur_tx[q] += num_tx_desc;
> > > > > > --
> > > > > > 2.43.0
> > > > > >
> > > > >
> > > > > --
> > > > > Kind Regards,
> > > > > Niklas Söderlund
> > >
> > > --
> > > Kind Regards,
> > > Niklas Söderlund
> 
> --
> Kind Regards,
> Niklas Söderlund