[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <TYBPR01MB5341C01EC932D1F53AEF188AD8D89@TYBPR01MB5341.jpnprd01.prod.outlook.com>
Date: Wed, 8 Feb 2023 23:33:06 +0000
From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To: Alexander H Duyck <alexander.duyck@...il.com>,
"davem@...emloft.net" <davem@...emloft.net>,
"edumazet@...gle.com" <edumazet@...gle.com>,
"kuba@...nel.org" <kuba@...nel.org>,
"pabeni@...hat.com" <pabeni@...hat.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-renesas-soc@...r.kernel.org"
<linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH net-next 3/4] net: renesas: rswitch: Remove gptp flag from
rswitch_gwca_queue
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@...esas.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
Powered by blists - more mailing lists