On 7/8/19 1:03 PM, Jiri Pirko wrote:
Mon, Jul 08, 2019 at 09:58:09PM CEST, snel...@pensando.io wrote:
On 7/8/19 12:34 PM, Jiri Pirko wrote:
Mon, Jul 08, 2019 at 09:25:32PM CEST, snel...@pensando.io wrote:

+
+static const struct devlink_ops ionic_dl_ops = {
+       .info_get       = ionic_dl_info_get,
+};
+
+int ionic_devlink_register(struct ionic *ionic)
+{
+       struct devlink *dl;
+       struct ionic **ip;
+       int err;
+
+       dl = devlink_alloc(&ionic_dl_ops, sizeof(struct ionic *));
Oups. Something is wrong with your flow. The devlink alloc is allocating
the structure that holds private data (per-device data) for you. This is
misuse :/

You are missing one parent device struct apparently.

Oh, I think I see something like it. The unused "struct ionic_devlink".
If I'm not mistaken, the alloc is only allocating enough for a pointer, not
the whole per device struct, and a few lines down from here the pointer to
the new devlink struct is assigned to ionic->dl.  This was based on what I
found in the qed driver's qed_devlink_register(), and it all seems to work.
I'm not saying your code won't work. What I say is that you should have
a struct for device that would be allocated by devlink_alloc()

Is there a particular reason why?  I appreciate that devlink_alloc() can give you this device specific space, just as alloc_etherdev_mq() can, but is there a specific reason why this should be used instead of setting up simply a pointer to a space that has already been allocated?  There are several drivers that are using it the way I've setup here, which happened to be the first examples I followed - are they doing something different that makes this valid for them?


The ionic struct should be associated with devlink_port. That you are
missing too.

We don't support any of devlink_port features at this point, just the simple device information.

sln



That unused struct ionic_devlink does need to go away, it was superfluous
after working out a better typecast off of devlink_priv().

I'll remove the unused struct ionic_devlink, but I think the rest is okay.

sln


+       if (!dl) {
+               dev_warn(ionic->dev, "devlink_alloc failed");
+               return -ENOMEM;
+       }
+
+       ip = (struct ionic **)devlink_priv(dl);
+       *ip = ionic;
+       ionic->dl = dl;
+
+       err = devlink_register(dl, ionic->dev);
+       if (err) {
+               dev_warn(ionic->dev, "devlink_register failed: %d\n", err);
+               goto err_dl_free;
+       }
+
+       return 0;
+
+err_dl_free:
+       ionic->dl = NULL;
+       devlink_free(dl);
+       return err;
+}
+
+void ionic_devlink_unregister(struct ionic *ionic)
+{
+       if (!ionic->dl)
+               return;
+
+       devlink_unregister(ionic->dl);
+       devlink_free(ionic->dl);
+}
diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h 
b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
new file mode 100644
index 000000000000..35528884e29f
--- /dev/null
+++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
+
+#ifndef _IONIC_DEVLINK_H_
+#define _IONIC_DEVLINK_H_
+
+#include <net/devlink.h>
+
+int ionic_devlink_register(struct ionic *ionic);
+void ionic_devlink_unregister(struct ionic *ionic);
+
+#endif /* _IONIC_DEVLINK_H_ */
--
2.17.1


Reply via email to