| Message ID | 20230208073445.2317192-4-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: renesas: rswitch: Improve TX timestamp accuracy | expand |
On Wed, 2023-02-08 at 16:34 +0900, Yoshihiro Shimoda wrote: > The gptp flag is completely related to the !dir_tx in struct > rswitch_gwca_queue. In the future, a new queue handling for > timestamp will be implemented and this gptp flag is confusable. > So, remove the gptp flag. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Based on these changes I am assuming that gptp == !dir_tx? Am I understanding it correctly? It would be useful if you called that out in the patch description. > --- > drivers/net/ethernet/renesas/rswitch.c | 26 +++++++++++--------------- > drivers/net/ethernet/renesas/rswitch.h | 1 - > 2 files changed, 11 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c > index b256dadada1d..e408d10184e8 100644 > --- a/drivers/net/ethernet/renesas/rswitch.c > +++ b/drivers/net/ethernet/renesas/rswitch.c > @@ -280,11 +280,14 @@ static void rswitch_gwca_queue_free(struct net_device *ndev, > { > int i; > > - if (gq->gptp) { > + if (!gq->dir_tx) { > dma_free_coherent(ndev->dev.parent, > sizeof(struct rswitch_ext_ts_desc) * > (gq->ring_size + 1), gq->rx_ring, gq->ring_dma); > gq->rx_ring = NULL; > + > + for (i = 0; i < gq->ring_size; i++) > + dev_kfree_skb(gq->skbs[i]); > } else { > dma_free_coherent(ndev->dev.parent, > sizeof(struct rswitch_ext_desc) * > @@ -292,11 +295,6 @@ static void rswitch_gwca_queue_free(struct net_device *ndev, > gq->tx_ring = NULL; > } > > - if (!gq->dir_tx) { > - for (i = 0; i < gq->ring_size; i++) > - dev_kfree_skb(gq->skbs[i]); > - } > - > kfree(gq->skbs); > gq->skbs = NULL; > } One piece I don't understand is why freeing of the skbs stored in the array here was removed. Is this cleaned up somewhere else before we call this function?
Hi Alexander, > From: Alexander H Duyck, Sent: Thursday, February 9, 2023 1:07 AM > > On Wed, 2023-02-08 at 16:34 +0900, Yoshihiro Shimoda wrote: > > The gptp flag is completely related to the !dir_tx in struct > > rswitch_gwca_queue. In the future, a new queue handling for > > timestamp will be implemented and this gptp flag is confusable. > > So, remove the gptp flag. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Based on these changes I am assuming that gptp == !dir_tx? Am I > understanding it correctly? It would be useful if you called that out > in the patch description. You're correct. I'll modify the description to clear why gptp == !dir_tx like below on v2 patch. --- In the previous code, the gptp flag was completely related to the !dir_tx in struct rswitch_gwca_queue because rswitch_gwca_queue_alloc() was called below: < In rswitch_txdmac_alloc() > err = rswitch_gwca_queue_alloc(ndev, priv, rdev->tx_queue, true, false, TX_RING_SIZE); So, dir_tx = true, gptp = false < In rswitch_rxdmac_alloc() > err = rswitch_gwca_queue_alloc(ndev, priv, rdev->rx_queue, false, true, RX_RING_SIZE); So, dir_tx = false, gptp = true In the future, a new queue handling for timestamp will be implemented and this gptp flag is confusable. So, remove the gptp flag. --- > > --- > > drivers/net/ethernet/renesas/rswitch.c | 26 +++++++++++--------------- > > drivers/net/ethernet/renesas/rswitch.h | 1 - > > 2 files changed, 11 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c > > index b256dadada1d..e408d10184e8 100644 > > --- a/drivers/net/ethernet/renesas/rswitch.c > > +++ b/drivers/net/ethernet/renesas/rswitch.c > > @@ -280,11 +280,14 @@ static void rswitch_gwca_queue_free(struct net_device *ndev, > > { > > int i; > > > > - if (gq->gptp) { > > + if (!gq->dir_tx) { > > dma_free_coherent(ndev->dev.parent, > > sizeof(struct rswitch_ext_ts_desc) * > > (gq->ring_size + 1), gq->rx_ring, gq->ring_dma); > > gq->rx_ring = NULL; > > + > > + for (i = 0; i < gq->ring_size; i++) > > + dev_kfree_skb(gq->skbs[i]); > > } else { > > dma_free_coherent(ndev->dev.parent, > > sizeof(struct rswitch_ext_desc) * > > @@ -292,11 +295,6 @@ static void rswitch_gwca_queue_free(struct net_device *ndev, > > gq->tx_ring = NULL; > > } > > > > - if (!gq->dir_tx) { > > - for (i = 0; i < gq->ring_size; i++) > > - dev_kfree_skb(gq->skbs[i]); > > - } > > - > > kfree(gq->skbs); > > gq->skbs = NULL; > > } > > One piece I don't understand is why freeing of the skbs stored in the > array here was removed. Is this cleaned up somewhere else before we > call this function? "gq->skbs = NULL;" seems unnecessary because this driver doesn't check whether gq->skbs is NULL or not. Also, gq->[rt]x_ring seem to be the same. So, I'll make such a patch which is removing unnecessary code after this patch series was accepted. Best regards, Yoshihiro Shimoda
On Wed, Feb 8, 2023 at 3:33 PM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > Hi Alexander, > > > From: Alexander H Duyck, Sent: Thursday, February 9, 2023 1:07 AM > > > > On Wed, 2023-02-08 at 16:34 +0900, Yoshihiro Shimoda wrote: > > > The gptp flag is completely related to the !dir_tx in struct > > > rswitch_gwca_queue. In the future, a new queue handling for > > > timestamp will be implemented and this gptp flag is confusable. > > > So, remove the gptp flag. > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > Based on these changes I am assuming that gptp == !dir_tx? Am I > > understanding it correctly? It would be useful if you called that out > > in the patch description. > > You're correct. > I'll modify the description to clear why gptp == !dir_tx like below on v2 patch. > --- > In the previous code, the gptp flag was completely related to the !dir_tx > in struct rswitch_gwca_queue because rswitch_gwca_queue_alloc() was called > below: > > < In rswitch_txdmac_alloc() > > err = rswitch_gwca_queue_alloc(ndev, priv, rdev->tx_queue, true, false, > TX_RING_SIZE); > So, dir_tx = true, gptp = false > > < In rswitch_rxdmac_alloc() > > err = rswitch_gwca_queue_alloc(ndev, priv, rdev->rx_queue, false, true, > RX_RING_SIZE); > So, dir_tx = false, gptp = true > > In the future, a new queue handling for timestamp will be implemented > and this gptp flag is confusable. So, remove the gptp flag. > --- It is a bit more readable if the relation is explained so if you could call that out in the description I would appreciate it. > > > --- > > > drivers/net/ethernet/renesas/rswitch.c | 26 +++++++++++--------------- > > > drivers/net/ethernet/renesas/rswitch.h | 1 - > > > 2 files changed, 11 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c > > > index b256dadada1d..e408d10184e8 100644 > > > --- a/drivers/net/ethernet/renesas/rswitch.c > > > +++ b/drivers/net/ethernet/renesas/rswitch.c > > > @@ -280,11 +280,14 @@ static void rswitch_gwca_queue_free(struct net_device *ndev, > > > { > > > int i; > > > > > > - if (gq->gptp) { > > > + if (!gq->dir_tx) { > > > dma_free_coherent(ndev->dev.parent, > > > sizeof(struct rswitch_ext_ts_desc) * > > > (gq->ring_size + 1), gq->rx_ring, gq->ring_dma); > > > gq->rx_ring = NULL; > > > + > > > + for (i = 0; i < gq->ring_size; i++) > > > + dev_kfree_skb(gq->skbs[i]); > > > } else { > > > dma_free_coherent(ndev->dev.parent, > > > sizeof(struct rswitch_ext_desc) * > > > @@ -292,11 +295,6 @@ static void rswitch_gwca_queue_free(struct net_device *ndev, > > > gq->tx_ring = NULL; > > > } > > > > > > - if (!gq->dir_tx) { > > > - for (i = 0; i < gq->ring_size; i++) > > > - dev_kfree_skb(gq->skbs[i]); > > > - } > > > - > > > kfree(gq->skbs); > > > gq->skbs = NULL; > > > } > > > > One piece I don't understand is why freeing of the skbs stored in the > > array here was removed. Is this cleaned up somewhere else before we > > call this function? > > "gq->skbs = NULL;" seems unnecessary because this driver doesn't check > whether gq->skbs is NULL or not. Also, gq->[rt]x_ring seem to be the same. > So, I'll make such a patch which is removing unnecessary code after > this patch series was accepted. I was actually referring to the lines you removed above that. Specifically I am wondering why the calls to dev_kfree_skb(gq->skbs[i]); were removed? I am wondering if this might be introducing a memory leak.
Hi Alexander, > From: Alexander Duyck, Sent: Thursday, February 9, 2023 10:02 AM > > On Wed, Feb 8, 2023 at 3:33 PM Yoshihiro Shimoda > <yoshihiro.shimoda.uh@renesas.com> wrote: > > > > Hi Alexander, > > > > > From: Alexander H Duyck, Sent: Thursday, February 9, 2023 1:07 AM > > > > > > On Wed, 2023-02-08 at 16:34 +0900, Yoshihiro Shimoda wrote: > > > > The gptp flag is completely related to the !dir_tx in struct > > > > rswitch_gwca_queue. In the future, a new queue handling for > > > > timestamp will be implemented and this gptp flag is confusable. > > > > So, remove the gptp flag. > > > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > > > Based on these changes I am assuming that gptp == !dir_tx? Am I > > > understanding it correctly? It would be useful if you called that out > > > in the patch description. > > > > You're correct. > > I'll modify the description to clear why gptp == !dir_tx like below on v2 patch. > > --- > > In the previous code, the gptp flag was completely related to the !dir_tx > > in struct rswitch_gwca_queue because rswitch_gwca_queue_alloc() was called > > below: > > > > < In rswitch_txdmac_alloc() > > > err = rswitch_gwca_queue_alloc(ndev, priv, rdev->tx_queue, true, false, > > TX_RING_SIZE); > > So, dir_tx = true, gptp = false > > > > < In rswitch_rxdmac_alloc() > > > err = rswitch_gwca_queue_alloc(ndev, priv, rdev->rx_queue, false, true, > > RX_RING_SIZE); > > So, dir_tx = false, gptp = true > > > > In the future, a new queue handling for timestamp will be implemented > > and this gptp flag is confusable. So, remove the gptp flag. > > --- > > It is a bit more readable if the relation is explained so if you could > call that out in the description I would appreciate it. I added the description on v2 patch. > > > > --- > > > > drivers/net/ethernet/renesas/rswitch.c | 26 +++++++++++--------------- > > > > drivers/net/ethernet/renesas/rswitch.h | 1 - > > > > 2 files changed, 11 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c > > > > index b256dadada1d..e408d10184e8 100644 > > > > --- a/drivers/net/ethernet/renesas/rswitch.c > > > > +++ b/drivers/net/ethernet/renesas/rswitch.c > > > > @@ -280,11 +280,14 @@ static void rswitch_gwca_queue_free(struct net_device *ndev, > > > > { > > > > int i; > > > > > > > > - if (gq->gptp) { > > > > + if (!gq->dir_tx) { > > > > dma_free_coherent(ndev->dev.parent, > > > > sizeof(struct rswitch_ext_ts_desc) * > > > > (gq->ring_size + 1), gq->rx_ring, gq->ring_dma); > > > > gq->rx_ring = NULL; > > > > + > > > > + for (i = 0; i < gq->ring_size; i++) > > > > + dev_kfree_skb(gq->skbs[i]); > > > > } else { > > > > dma_free_coherent(ndev->dev.parent, > > > > sizeof(struct rswitch_ext_desc) * > > > > @@ -292,11 +295,6 @@ static void rswitch_gwca_queue_free(struct net_device *ndev, > > > > gq->tx_ring = NULL; > > > > } > > > > > > > > - if (!gq->dir_tx) { > > > > - for (i = 0; i < gq->ring_size; i++) > > > > - dev_kfree_skb(gq->skbs[i]); > > > > - } > > > > - > > > > kfree(gq->skbs); > > > > gq->skbs = NULL; > > > > } > > > > > > One piece I don't understand is why freeing of the skbs stored in the > > > array here was removed. Is this cleaned up somewhere else before we > > > call this function? > > > > "gq->skbs = NULL;" seems unnecessary because this driver doesn't check > > whether gq->skbs is NULL or not. Also, gq->[rt]x_ring seem to be the same. > > So, I'll make such a patch which is removing unnecessary code after > > this patch series was accepted. > > I was actually referring to the lines you removed above that. > Specifically I am wondering why the calls to > dev_kfree_skb(gq->skbs[i]); were removed? I am wondering if this might > be introducing a memory leak. dev_kfree_skb(gq->skbs[i]); were not removed. This patch Just moves it into the first "if (!gq->dir_tx) {" because having double "if (!gq->dir_tx) {" is not good. Best regards, Yoshihiro Shimoda
diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c index b256dadada1d..e408d10184e8 100644 --- a/drivers/net/ethernet/renesas/rswitch.c +++ b/drivers/net/ethernet/renesas/rswitch.c @@ -280,11 +280,14 @@ static void rswitch_gwca_queue_free(struct net_device *ndev, { int i; - if (gq->gptp) { + if (!gq->dir_tx) { dma_free_coherent(ndev->dev.parent, sizeof(struct rswitch_ext_ts_desc) * (gq->ring_size + 1), gq->rx_ring, gq->ring_dma); gq->rx_ring = NULL; + + for (i = 0; i < gq->ring_size; i++) + dev_kfree_skb(gq->skbs[i]); } else { dma_free_coherent(ndev->dev.parent, sizeof(struct rswitch_ext_desc) * @@ -292,11 +295,6 @@ static void rswitch_gwca_queue_free(struct net_device *ndev, gq->tx_ring = NULL; } - if (!gq->dir_tx) { - for (i = 0; i < gq->ring_size; i++) - dev_kfree_skb(gq->skbs[i]); - } - kfree(gq->skbs); gq->skbs = NULL; } @@ -304,12 +302,11 @@ static void rswitch_gwca_queue_free(struct net_device *ndev, static int rswitch_gwca_queue_alloc(struct net_device *ndev, struct rswitch_private *priv, struct rswitch_gwca_queue *gq, - bool dir_tx, bool gptp, int ring_size) + bool dir_tx, int ring_size) { int i, bit; gq->dir_tx = dir_tx; - gq->gptp = gptp; gq->ring_size = ring_size; gq->ndev = ndev; @@ -317,17 +314,18 @@ static int rswitch_gwca_queue_alloc(struct net_device *ndev, if (!gq->skbs) return -ENOMEM; - if (!dir_tx) + if (!dir_tx) { rswitch_gwca_queue_alloc_skb(gq, 0, gq->ring_size); - if (gptp) gq->rx_ring = dma_alloc_coherent(ndev->dev.parent, sizeof(struct rswitch_ext_ts_desc) * (gq->ring_size + 1), &gq->ring_dma, GFP_KERNEL); - else + } else { gq->tx_ring = dma_alloc_coherent(ndev->dev.parent, sizeof(struct rswitch_ext_desc) * (gq->ring_size + 1), &gq->ring_dma, GFP_KERNEL); + } + if (!gq->rx_ring && !gq->tx_ring) goto out; @@ -539,8 +537,7 @@ static int rswitch_txdmac_alloc(struct net_device *ndev) if (!rdev->tx_queue) return -EBUSY; - err = rswitch_gwca_queue_alloc(ndev, priv, rdev->tx_queue, true, false, - TX_RING_SIZE); + err = rswitch_gwca_queue_alloc(ndev, priv, rdev->tx_queue, true, TX_RING_SIZE); if (err < 0) { rswitch_gwca_put(priv, rdev->tx_queue); return err; @@ -574,8 +571,7 @@ static int rswitch_rxdmac_alloc(struct net_device *ndev) if (!rdev->rx_queue) return -EBUSY; - err = rswitch_gwca_queue_alloc(ndev, priv, rdev->rx_queue, false, true, - RX_RING_SIZE); + err = rswitch_gwca_queue_alloc(ndev, priv, rdev->rx_queue, false, RX_RING_SIZE); if (err < 0) { rswitch_gwca_put(priv, rdev->rx_queue); return err; diff --git a/drivers/net/ethernet/renesas/rswitch.h b/drivers/net/ethernet/renesas/rswitch.h index 79c8ff01021c..ee36e8e896d2 100644 --- a/drivers/net/ethernet/renesas/rswitch.h +++ b/drivers/net/ethernet/renesas/rswitch.h @@ -913,7 +913,6 @@ struct rswitch_etha { struct rswitch_gwca_queue { int index; bool dir_tx; - bool gptp; union { struct rswitch_ext_desc *tx_ring; struct rswitch_ext_ts_desc *rx_ring;
The gptp flag is completely related to the !dir_tx in struct rswitch_gwca_queue. In the future, a new queue handling for timestamp will be implemented and this gptp flag is confusable. So, remove the gptp flag. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/net/ethernet/renesas/rswitch.c | 26 +++++++++++--------------- drivers/net/ethernet/renesas/rswitch.h | 1 - 2 files changed, 11 insertions(+), 16 deletions(-)