diff mbox series

[net,v2] net: renesas: rswitch: Fix timestamp feature after all descriptors are used

Message ID 20230607070141.1795982-1-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series [net,v2] net: renesas: rswitch: Fix timestamp feature after all descriptors are used | expand

Commit Message

Yoshihiro Shimoda June 7, 2023, 7:01 a.m. UTC 
The timestamp descriptors were intended to act cyclically. Descriptors
from index 0 through gq->ring_size - 1 contain actual information, and
the last index (gq->ring_size) should have LINKFIX to indicate
the first index 0 descriptor. However, thie LINKFIX value is missing,
causing the timestamp feature to stop after all descriptors are used.
To resolve this issue, set the LINKFIX to the timestamp descritors.

Reported-by: Phong Hoang <phong.hoang.wz@renesas.com>
Fixes: 33f5d733b589 ("net: renesas: rswitch: Improve TX timestamp accuracy")
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 Since I got this report locally, I didn't add Closes: tag.

 Changes from v1:
https://lore.kernel.org/all/20230607064402.1795548-1-yoshihiro.shimoda.uh@renesas.com/
 - Fix typo in the subject.

 drivers/net/ethernet/renesas/rswitch.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Hao Lan June 7, 2023, 11:27 a.m. UTC | #1 
On 2023/6/7 15:01, Yoshihiro Shimoda wrote:
> diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> index aace87139cea..049adbf5a642 100644
> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -420,7 +420,7 @@ static int rswitch_gwca_queue_format(struct net_device *ndev,
>  }
>  
>  static void rswitch_gwca_ts_queue_fill(struct rswitch_private *priv,
> -				       int start_index, int num)
> +				       int start_index, int num, bool last)
>  {
>  	struct rswitch_gwca_queue *gq = &priv->gwca.ts_queue;
>  	struct rswitch_ts_desc *desc;
> @@ -431,6 +431,12 @@ static void rswitch_gwca_ts_queue_fill(struct rswitch_private *priv,
>  		desc = &gq->ts_ring[index];
>  		desc->desc.die_dt = DT_FEMPTY_ND | DIE;
>  	}
> +
> +	if (last) {
> +		desc = &gq->ts_ring[gq->ring_size];
> +		rswitch_desc_set_dptr(&desc->desc, gq->ring_dma);
> +		desc->desc.die_dt = DT_LINKFIX;
> +	}
>  }
>  
Hello Yoshihiro Shimoda，

Does your function set the last descriptor to hardware on initialization, but not at other times?
I think rswitch_gwca_ts_queue_fill should be implemented in a separate function,
not use the 'last' distinguish the last descriptor.
But if it should be setting every cycle, I think rswitch_gwca_queue_ext_ts_fill should
check if the descriptor is the last in the queue and set the LINKFIX flag.

>  static int rswitch_gwca_queue_ext_ts_fill(struct net_device *ndev,
> @@ -941,7 +947,7 @@ static void rswitch_ts(struct rswitch_private *priv)
>  	}
>  
>  	num = rswitch_get_num_cur_queues(gq);
> -	rswitch_gwca_ts_queue_fill(priv, gq->dirty, num);
> +	rswitch_gwca_ts_queue_fill(priv, gq->dirty, num, false);
>  	gq->dirty = rswitch_next_queue_index(gq, false, num);
>  }
>  
> @@ -1780,7 +1786,7 @@ static int rswitch_init(struct rswitch_private *priv)
>  	if (err < 0)
>  		goto err_ts_queue_alloc;
>  
> -	rswitch_gwca_ts_queue_fill(priv, 0, TS_RING_SIZE);
> +	rswitch_gwca_ts_queue_fill(priv, 0, TS_RING_SIZE, true);
>  	INIT_LIST_HEAD(&priv->gwca.ts_info_list);
>  
>  	for (i = 0; i < RSWITCH_NUM_PORTS; i++) {
>
Simon Horman June 7, 2023, 3:25 p.m. UTC | #2 
On Wed, Jun 07, 2023 at 04:01:41PM +0900, Yoshihiro Shimoda wrote:
> The timestamp descriptors were intended to act cyclically. Descriptors
> from index 0 through gq->ring_size - 1 contain actual information, and
> the last index (gq->ring_size) should have LINKFIX to indicate
> the first index 0 descriptor. However, thie LINKFIX value is missing,

Hi Shimoda-san,

a very minor nit, in case you end up posting a v3 for some other reason:

	thie -> the

> causing the timestamp feature to stop after all descriptors are used.
> To resolve this issue, set the LINKFIX to the timestamp descritors.
> 
> Reported-by: Phong Hoang <phong.hoang.wz@renesas.com>
> Fixes: 33f5d733b589 ("net: renesas: rswitch: Improve TX timestamp accuracy")

...
Yoshihiro Shimoda June 8, 2023, 12:20 a.m. UTC | #3 
Hi Hao,

> From: Hao Lan, Sent: Wednesday, June 7, 2023 8:28 PM
> 
> On 2023/6/7 15:01, Yoshihiro Shimoda wrote:
> > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> > index aace87139cea..049adbf5a642 100644
> > --- a/drivers/net/ethernet/renesas/rswitch.c
> > +++ b/drivers/net/ethernet/renesas/rswitch.c
> > @@ -420,7 +420,7 @@ static int rswitch_gwca_queue_format(struct net_device *ndev,
> >  }
> >
> >  static void rswitch_gwca_ts_queue_fill(struct rswitch_private *priv,
> > -				       int start_index, int num)
> > +				       int start_index, int num, bool last)
> >  {
> >  	struct rswitch_gwca_queue *gq = &priv->gwca.ts_queue;
> >  	struct rswitch_ts_desc *desc;
> > @@ -431,6 +431,12 @@ static void rswitch_gwca_ts_queue_fill(struct rswitch_private *priv,
> >  		desc = &gq->ts_ring[index];
> >  		desc->desc.die_dt = DT_FEMPTY_ND | DIE;
> >  	}
> > +
> > +	if (last) {
> > +		desc = &gq->ts_ring[gq->ring_size];
> > +		rswitch_desc_set_dptr(&desc->desc, gq->ring_dma);
> > +		desc->desc.die_dt = DT_LINKFIX;
> > +	}
> >  }
> >
> Hello Yoshihiro Shimoda，
> 
> Does your function set the last descriptor to hardware on initialization, but not at other times?

The last descriptor initialization is needed at the first time. So, after the initialization,
the last setting will not run anymore.

> I think rswitch_gwca_ts_queue_fill should be implemented in a separate function,
> not use the 'last' distinguish the last descriptor.

I got it. I'll modify this patch on v3.

> But if it should be setting every cycle, I think rswitch_gwca_queue_ext_ts_fill should
> check if the descriptor is the last in the queue and set the LINKFIX flag.

Thank you for the comment. The last descriptor should not be setting every cycle.
To implement the code for consistency, I think that I should add rswitch_tsdesc_init()
rswitch_gwca_queue_format() like rswitch_txdmac_init() and rswitch_gwca_queue_format()

Best regards,
Yoshihiro Shimoda

> >  static int rswitch_gwca_queue_ext_ts_fill(struct net_device *ndev,
> > @@ -941,7 +947,7 @@ static void rswitch_ts(struct rswitch_private *priv)
> >  	}
> >
> >  	num = rswitch_get_num_cur_queues(gq);
> > -	rswitch_gwca_ts_queue_fill(priv, gq->dirty, num);
> > +	rswitch_gwca_ts_queue_fill(priv, gq->dirty, num, false);
> >  	gq->dirty = rswitch_next_queue_index(gq, false, num);
> >  }
> >
> > @@ -1780,7 +1786,7 @@ static int rswitch_init(struct rswitch_private *priv)
> >  	if (err < 0)
> >  		goto err_ts_queue_alloc;
> >
> > -	rswitch_gwca_ts_queue_fill(priv, 0, TS_RING_SIZE);
> > +	rswitch_gwca_ts_queue_fill(priv, 0, TS_RING_SIZE, true);
> >  	INIT_LIST_HEAD(&priv->gwca.ts_info_list);
> >
> >  	for (i = 0; i < RSWITCH_NUM_PORTS; i++) {
> >
Yoshihiro Shimoda June 8, 2023, 12:21 a.m. UTC | #4 
Hi Simon-san,

> From: Simon Horman, Sent: Thursday, June 8, 2023 12:26 AM
> 
> On Wed, Jun 07, 2023 at 04:01:41PM +0900, Yoshihiro Shimoda wrote:
> > The timestamp descriptors were intended to act cyclically. Descriptors
> > from index 0 through gq->ring_size - 1 contain actual information, and
> > the last index (gq->ring_size) should have LINKFIX to indicate
> > the first index 0 descriptor. However, thie LINKFIX value is missing,
> 
> Hi Shimoda-san,
> 
> a very minor nit, in case you end up posting a v3 for some other reason:
> 
> 	thie -> the

Thank you for your review! I'll fix this on v3.

Best regards,
Yoshihiro Shimoda

> > causing the timestamp feature to stop after all descriptors are used.
> > To resolve this issue, set the LINKFIX to the timestamp descritors.
> >
> > Reported-by: Phong Hoang <phong.hoang.wz@renesas.com>
> > Fixes: 33f5d733b589 ("net: renesas: rswitch: Improve TX timestamp accuracy")
> 
> ...
Yoshihiro Shimoda June 8, 2023, 12:57 a.m. UTC | #5 
Hi again,

> From: Yoshihiro Shimoda, Sent: Thursday, June 8, 2023 9:21 AM
> 
> Hi Hao,
> 
> > From: Hao Lan, Sent: Wednesday, June 7, 2023 8:28 PM
> >
> > On 2023/6/7 15:01, Yoshihiro Shimoda wrote:
> > > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> > > index aace87139cea..049adbf5a642 100644
> > > --- a/drivers/net/ethernet/renesas/rswitch.c
> > > +++ b/drivers/net/ethernet/renesas/rswitch.c
> > > @@ -420,7 +420,7 @@ static int rswitch_gwca_queue_format(struct net_device *ndev,
> > >  }
> > >
> > >  static void rswitch_gwca_ts_queue_fill(struct rswitch_private *priv,
> > > -				       int start_index, int num)
> > > +				       int start_index, int num, bool last)
> > >  {
> > >  	struct rswitch_gwca_queue *gq = &priv->gwca.ts_queue;
> > >  	struct rswitch_ts_desc *desc;
> > > @@ -431,6 +431,12 @@ static void rswitch_gwca_ts_queue_fill(struct rswitch_private *priv,
> > >  		desc = &gq->ts_ring[index];
> > >  		desc->desc.die_dt = DT_FEMPTY_ND | DIE;
> > >  	}
> > > +
> > > +	if (last) {
> > > +		desc = &gq->ts_ring[gq->ring_size];
> > > +		rswitch_desc_set_dptr(&desc->desc, gq->ring_dma);
> > > +		desc->desc.die_dt = DT_LINKFIX;
> > > +	}
> > >  }
> > >
> > Hello Yoshihiro Shimoda，
> >
> > Does your function set the last descriptor to hardware on initialization, but not at other times?
> 
> The last descriptor initialization is needed at the first time. So, after the initialization,
> the last setting will not run anymore.
> 
> > I think rswitch_gwca_ts_queue_fill should be implemented in a separate function,
> > not use the 'last' distinguish the last descriptor.
> 
> I got it. I'll modify this patch on v3.
> 
> > But if it should be setting every cycle, I think rswitch_gwca_queue_ext_ts_fill should
> > check if the descriptor is the last in the queue and set the LINKFIX flag.
> 
> Thank you for the comment. The last descriptor should not be setting every cycle.
> To implement the code for consistency, I think that I should add rswitch_tsdesc_init()
> rswitch_gwca_queue_format() like rswitch_txdmac_init() and rswitch_gwca_queue_format()

About ts_queue, it is similar with linkfix, not txdmac. So, now I'm thinking that modifying
the rswitch_gwca_ts_queue_alloc() is suitable.

Best regards,
Yoshihiro Shimoda

> Best regards,
> Yoshihiro Shimoda
> 
> > >  static int rswitch_gwca_queue_ext_ts_fill(struct net_device *ndev,
> > > @@ -941,7 +947,7 @@ static void rswitch_ts(struct rswitch_private *priv)
> > >  	}
> > >
> > >  	num = rswitch_get_num_cur_queues(gq);
> > > -	rswitch_gwca_ts_queue_fill(priv, gq->dirty, num);
> > > +	rswitch_gwca_ts_queue_fill(priv, gq->dirty, num, false);
> > >  	gq->dirty = rswitch_next_queue_index(gq, false, num);
> > >  }
> > >
> > > @@ -1780,7 +1786,7 @@ static int rswitch_init(struct rswitch_private *priv)
> > >  	if (err < 0)
> > >  		goto err_ts_queue_alloc;
> > >
> > > -	rswitch_gwca_ts_queue_fill(priv, 0, TS_RING_SIZE);
> > > +	rswitch_gwca_ts_queue_fill(priv, 0, TS_RING_SIZE, true);
> > >  	INIT_LIST_HEAD(&priv->gwca.ts_info_list);
> > >
> > >  	for (i = 0; i < RSWITCH_NUM_PORTS; i++) {
> > >
Hao Lan June 8, 2023, 1:30 p.m. UTC | #6 
On 2023/6/8 8:57, Yoshihiro Shimoda wrote:
> Hi again,
> 
>> From: Yoshihiro Shimoda, Sent: Thursday, June 8, 2023 9:21 AM
>>
>> Hi Hao,
>>
>>> From: Hao Lan, Sent: Wednesday, June 7, 2023 8:28 PM
>>> Hello Yoshihiro Shimoda，
>>>
>>> Does your function set the last descriptor to hardware on initialization, but not at other times?
>>
>> The last descriptor initialization is needed at the first time. So, after the initialization,
>> the last setting will not run anymore.
>>
>>> I think rswitch_gwca_ts_queue_fill should be implemented in a separate function,
>>> not use the 'last' distinguish the last descriptor.
>>
>> I got it. I'll modify this patch on v3.
>>
>>> But if it should be setting every cycle, I think rswitch_gwca_queue_ext_ts_fill should
>>> check if the descriptor is the last in the queue and set the LINKFIX flag.
>>
>> Thank you for the comment. The last descriptor should not be setting every cycle.
>> To implement the code for consistency, I think that I should add rswitch_tsdesc_init()
>> rswitch_gwca_queue_format() like rswitch_txdmac_init() and rswitch_gwca_queue_format()
> 
> About ts_queue, it is similar with linkfix, not txdmac. So, now I'm thinking that modifying
> the rswitch_gwca_ts_queue_alloc() is suitable.
> 
> Best regards,
> Yoshihiro Shimoda
> 

Reviewed-by: Hao Lan <lanhao@huawei.com>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index aace87139cea..049adbf5a642 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -420,7 +420,7 @@  static int rswitch_gwca_queue_format(struct net_device *ndev,
 }
 
 static void rswitch_gwca_ts_queue_fill(struct rswitch_private *priv,
-				       int start_index, int num)
+				       int start_index, int num, bool last)
 {
 	struct rswitch_gwca_queue *gq = &priv->gwca.ts_queue;
 	struct rswitch_ts_desc *desc;
@@ -431,6 +431,12 @@  static void rswitch_gwca_ts_queue_fill(struct rswitch_private *priv,
 		desc = &gq->ts_ring[index];
 		desc->desc.die_dt = DT_FEMPTY_ND | DIE;
 	}
+
+	if (last) {
+		desc = &gq->ts_ring[gq->ring_size];
+		rswitch_desc_set_dptr(&desc->desc, gq->ring_dma);
+		desc->desc.die_dt = DT_LINKFIX;
+	}
 }
 
 static int rswitch_gwca_queue_ext_ts_fill(struct net_device *ndev,
@@ -941,7 +947,7 @@  static void rswitch_ts(struct rswitch_private *priv)
 	}
 
 	num = rswitch_get_num_cur_queues(gq);
-	rswitch_gwca_ts_queue_fill(priv, gq->dirty, num);
+	rswitch_gwca_ts_queue_fill(priv, gq->dirty, num, false);
 	gq->dirty = rswitch_next_queue_index(gq, false, num);
 }
 
@@ -1780,7 +1786,7 @@  static int rswitch_init(struct rswitch_private *priv)
 	if (err < 0)
 		goto err_ts_queue_alloc;
 
-	rswitch_gwca_ts_queue_fill(priv, 0, TS_RING_SIZE);
+	rswitch_gwca_ts_queue_fill(priv, 0, TS_RING_SIZE, true);
 	INIT_LIST_HEAD(&priv->gwca.ts_info_list);
 
 	for (i = 0; i < RSWITCH_NUM_PORTS; i++) {