On Wed, Feb 8, 2023 at 3:33 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >
> > 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.
