public inbox for netdev@vger.kernel.org 
 help / color / mirror / Atom feed

* [PATCH net v2] net: renesas: rswitch: Fix timestamp feature after all descriptors are used
@ 2023-06-07  7:01 Yoshihiro Shimoda
  2023-06-07 11:27 ` Hao Lan
  2023-06-07 15:25 ` Simon Horman
  0 siblings, 2 replies; 7+ messages in thread
From: Yoshihiro Shimoda @ 2023-06-07  7:01 UTC (permalink / raw)
  To: s.shtylyov, davem, edumazet, kuba, pabeni
  Cc: netdev, linux-renesas-soc, Yoshihiro Shimoda, Phong Hoang

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(-)

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++) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

• * Re: [PATCH net v2] net: renesas: rswitch: Fix timestamp feature after all descriptors are used
    2023-06-07  7:01 [PATCH net v2] net: renesas: rswitch: Fix timestamp feature after all descriptors are used Yoshihiro Shimoda
  @ 2023-06-07 11:27 ` Hao Lan
    2023-06-08  0:20   ` Yoshihiro Shimoda
    2023-06-07 15:25 ` Simon Horman
    1 sibling, 1 reply; 7+ messages in thread
  From: Hao Lan @ 2023-06-07 11:27 UTC (permalink / raw)
    To: Yoshihiro Shimoda, s.shtylyov, davem, edumazet, kuba, pabeni
    Cc: netdev, linux-renesas-soc, Phong Hoang
  
  
  
  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++) {
  > 
  
  ^ permalink raw reply	[flat|nested] 7+ messages in thread

• • * RE: [PATCH net v2] net: renesas: rswitch: Fix timestamp feature after all descriptors are used
      2023-06-07 11:27 ` Hao Lan
    @ 2023-06-08  0:20   ` Yoshihiro Shimoda
      2023-06-08  0:57     ` Yoshihiro Shimoda
      0 siblings, 1 reply; 7+ messages in thread
    From: Yoshihiro Shimoda @ 2023-06-08  0:20 UTC (permalink / raw)
      To: Hao Lan, s.shtylyov@omp•ru, davem@davemloft•net,
    	edumazet@google•com, kuba@kernel•org, pabeni@redhat•com
      Cc: netdev@vger•kernel.org, linux-renesas-soc@vger•kernel.org,
    	Phong Hoang
    
    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++) {
    > >
    
    ^ permalink raw reply	[flat|nested] 7+ messages in thread

  • • * RE: [PATCH net v2] net: renesas: rswitch: Fix timestamp feature after all descriptors are used
        2023-06-08  0:20   ` Yoshihiro Shimoda
      @ 2023-06-08  0:57     ` Yoshihiro Shimoda
        2023-06-08 13:30       ` Hao Lan
        0 siblings, 1 reply; 7+ messages in thread
      From: Yoshihiro Shimoda @ 2023-06-08  0:57 UTC (permalink / raw)
        To: Hao Lan, s.shtylyov@omp•ru, davem@davemloft•net,
      	edumazet@google•com, kuba@kernel•org, pabeni@redhat•com
        Cc: netdev@vger•kernel.org, linux-renesas-soc@vger•kernel.org,
      	Phong Hoang
      
      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++) {
      > > >
      
      ^ permalink raw reply	[flat|nested] 7+ messages in thread

    • • * Re: [PATCH net v2] net: renesas: rswitch: Fix timestamp feature after all descriptors are used
          2023-06-08  0:57     ` Yoshihiro Shimoda
        @ 2023-06-08 13:30       ` Hao Lan
          0 siblings, 0 replies; 7+ messages in thread
        From: Hao Lan @ 2023-06-08 13:30 UTC (permalink / raw)
          To: Yoshihiro Shimoda, s.shtylyov@omp•ru, davem@davemloft•net,
        	edumazet@google•com, kuba@kernel•org, pabeni@redhat•com
          Cc: netdev@vger•kernel.org, linux-renesas-soc@vger•kernel.org,
        	Phong Hoang
        
        
        
        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>
        
        ^ permalink raw reply	[flat|nested] 7+ messages in thread

• * Re: [PATCH net v2] net: renesas: rswitch: Fix timestamp feature after all descriptors are used
    2023-06-07  7:01 [PATCH net v2] net: renesas: rswitch: Fix timestamp feature after all descriptors are used Yoshihiro Shimoda
    2023-06-07 11:27 ` Hao Lan
  @ 2023-06-07 15:25 ` Simon Horman
    2023-06-08  0:21   ` Yoshihiro Shimoda
    1 sibling, 1 reply; 7+ messages in thread
  From: Simon Horman @ 2023-06-07 15:25 UTC (permalink / raw)
    To: Yoshihiro Shimoda
    Cc: s.shtylyov, davem, edumazet, kuba, pabeni, netdev,
  	linux-renesas-soc, Phong Hoang
  
  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")
  
  ...
  
  ^ permalink raw reply	[flat|nested] 7+ messages in thread

• • * RE: [PATCH net v2] net: renesas: rswitch: Fix timestamp feature after all descriptors are used
      2023-06-07 15:25 ` Simon Horman
    @ 2023-06-08  0:21   ` Yoshihiro Shimoda
      0 siblings, 0 replies; 7+ messages in thread
    From: Yoshihiro Shimoda @ 2023-06-08  0:21 UTC (permalink / raw)
      To: Simon Horman
      Cc: s.shtylyov@omp•ru, davem@davemloft•net, edumazet@google•com,
    	kuba@kernel•org, pabeni@redhat•com, netdev@vger•kernel.org,
    	linux-renesas-soc@vger•kernel.org, Phong Hoang
    
    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")
    > 
    > ...
    
    ^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-06-08 13:30 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-07  7:01 [PATCH net v2] net: renesas: rswitch: Fix timestamp feature after all descriptors are used Yoshihiro Shimoda
2023-06-07 11:27 ` Hao Lan
2023-06-08  0:20   ` Yoshihiro Shimoda
2023-06-08  0:57     ` Yoshihiro Shimoda
2023-06-08 13:30       ` Hao Lan
2023-06-07 15:25 ` Simon Horman
2023-06-08  0:21   ` Yoshihiro Shimoda

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox