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@gmail.com>
Sent: Monday, November 2, 2020 7:01 PM
To: George Cherian <gcherian@marvell.com>
Cc: Network Development <redacted>; linux-kernel <linux-
kernel@vger.kernel.org>; Jakub Kicinski [off-list ref]; David Miller
[off-list ref]; Sunil Kovvuri Goutham
[off-list ref]; Linu Cherian [off-list ref];
Geethasowjanya Akula [off-list ref]; masahiroy@kernel.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
[off-list ref] wrote:

quoted

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@marvell.com>
Signed-off-by: Jerin Jacob <redacted>
Signed-off-by: George Cherian <redacted>

quoted

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.

quoted

 #ifdef CONFIG_DEBUG_FS
        struct rvu_debugfs      rvu_dbg;
 #endif
+       struct rvu_devlink      *rvu_dl;
 };

quoted

+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.

quoted

+       }
+
+       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