netdev - RE: [PATCH net-next 3/4] net: renesas: rswitch: Remove gptp flag from rswitch_gwca

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 9 Feb 2023 01:13:30 +0000
From:   Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "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 Duyck, Sent: Thursday, February 9, 2023 10:02 AM
> 
> On Wed, Feb 8, 2023 at 3:33 PM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@...esas.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@...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.
> > ---
> 
> 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