netdev - Re: [net-next PATCH 1/3] octeontx2-af: Add devlink suppoort to af driver

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>] [day] [month] [year] [list]

Message-ID: <BYAPR18MB26790653D85A78A92250B44DC5110@BYAPR18MB2679.namprd18.prod.outlook.com>
Date:   Tue, 3 Nov 2020 03:38:37 +0000
From:   George Cherian <gcherian@...vell.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC:     Network Development <netdev@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        "David Miller" <davem@...emloft.net>,
        Sunil Kovvuri Goutham <sgoutham@...vell.com>,
        Linu Cherian <lcherian@...vell.com>,
        Geethasowjanya Akula <gakula@...vell.com>,
        "masahiroy@...nel.org" <masahiroy@...nel.org>
Subject: Re: [net-next PATCH 1/3] octeontx2-af: Add devlink suppoort to af
 driver

Hi Willem,

Thanks for the review.

> -----Original Message-----
> From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> Sent: Monday, November 2, 2020 7:01 PM
> To: George Cherian <gcherian@...vell.com>
> Cc: Network Development <netdev@...r.kernel.org>; linux-kernel <linux-
> kernel@...r.kernel.org>; Jakub Kicinski <kuba@...nel.org>; David Miller
> <davem@...emloft.net>; Sunil Kovvuri Goutham
> <sgoutham@...vell.com>; Linu Cherian <lcherian@...vell.com>;
> Geethasowjanya Akula <gakula@...vell.com>; masahiroy@...nel.org
> Subject: Re: [net-next PATCH 1/3] octeontx2-af: Add devlink suppoort
> to af driver
> 
> On Mon, Nov 2, 2020 at 12:07 AM George Cherian
> <george.cherian@...vell.com> wrote:
> >
> > Add devlink support to AF driver. Basic devlink support is added.
> > Currently info_get is the only supported devlink ops.
> >
> > devlink ouptput looks like this
> >  # devlink dev
> >  pci/0002:01:00.0
> >  # devlink dev info
> >  pci/0002:01:00.0:
> >   driver octeontx2-af
> >   versions:
> >       fixed:
> >         mbox version: 9
> >
> > Signed-off-by: Sunil Kovvuri Goutham <sgoutham@...vell.com>
> > Signed-off-by: Jerin Jacob <jerinj@...vell.com>
> > Signed-off-by: George Cherian <george.cherian@...vell.com>
> 
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > index 5ac9bb12415f..c112b299635d 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> > @@ -12,7 +12,10 @@
> >  #define RVU_H
> >
> >  #include <linux/pci.h>
> > +#include <net/devlink.h>
> > +
> >  #include "rvu_struct.h"
> > +#include "rvu_devlink.h"
> >  #include "common.h"
> >  #include "mbox.h"
> >
> > @@ -372,10 +375,10 @@ struct rvu {
> >         struct npc_kpu_profile_adapter kpu;
> >
> >         struct ptp              *ptp;
> > -
> 
> accidentally removed this line?
Yes.
> 
> >  #ifdef CONFIG_DEBUG_FS
> >         struct rvu_debugfs      rvu_dbg;
> >  #endif
> > +       struct rvu_devlink      *rvu_dl;
> >  };
> 
> 
> > +int rvu_register_dl(struct rvu *rvu)
> > +{
> > +       struct rvu_devlink *rvu_dl;
> > +       struct devlink *dl;
> > +       int err;
> > +
> > +       rvu_dl = kzalloc(sizeof(*rvu_dl), GFP_KERNEL);
> > +       if (!rvu_dl)
> > +               return -ENOMEM;
> > +
> > +       dl = devlink_alloc(&rvu_devlink_ops, sizeof(struct rvu_devlink));
> > +       if (!dl) {
> > +               dev_warn(rvu->dev, "devlink_alloc failed\n");
> > +               return -ENOMEM;
> 
> rvu_dl not freed on error.
Thanks for pointing out, will address in v2.
> 
> This happens a couple of times in these patches
Will fix it.
> 
> Is the intermediate struct needed, or could you embed the fields directly into
> rvu and use container_of to get from devlink to struct rvu? Even if needed,
> perhaps easier to embed the struct into rvu rather than a pointer.
Currently only 2 hardware blocks are supported NIX and NPA.
Error reporting for more HW blocks will be added, that’s the reason for the intermediate struct.
> 
> > +       }
> > +
> > +       err = devlink_register(dl, rvu->dev);
> > +       if (err) {
> > +               dev_err(rvu->dev, "devlink register failed with error %d\n", err);
> > +               devlink_free(dl);
> > +               return err;
> > +       }
> > +
> > +       rvu_dl->dl = dl;
> > +       rvu_dl->rvu = rvu;
> > +       rvu->rvu_dl = rvu_dl;
> > +       return 0;
> > +}
> > +
> > +void rvu_unregister_dl(struct rvu *rvu) {
> > +       struct rvu_devlink *rvu_dl = rvu->rvu_dl;
> > +       struct devlink *dl = rvu_dl->dl;
> > +
> > +       if (!dl)
> > +               return;
> > +
> > +       devlink_unregister(dl);
> > +       devlink_free(dl);
> 
> here too
Yes, will fix in v2.

Regards,
-George