Tue, Jul 09, 2019 at 09:13:53PM CEST, snelson@pensando.io wrote:

On 7/8/19 11:56 PM, Jiri Pirko wrote:

quoted

Tue, Jul 09, 2019 at 12:58:00AM CEST, snelson@pensando.io wrote:

quoted

On 7/8/19 1:03 PM, Jiri Pirko wrote:

quoted

Mon, Jul 08, 2019 at 09:58:09PM CEST, snelson@pensando.io wrote:

quoted

On 7/8/19 12:34 PM, Jiri Pirko wrote:

quoted

Mon, Jul 08, 2019 at 09:25:32PM CEST, snelson@pensando.io wrote:

quoted

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

Yes. Devlink manipulates with the whole device. However,
alloc_etherdev_mq() allocates only net_device. These are 2 different
things. devlink port relates 1:1 to net_device. However, devlink
instance can have multiple ports. What I say is do it correctly.

So what you are saying is that anyone who wants to add even the smallest
devlink feature to their driver needs to rework their basic device memory
setup to do it the devlink way.  I can see where some folks may have a
problem with this.

It's just about having a structure to hold device data. You don't have
to rework anything, just add this small one.

quoted

quoted

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?

Nope. I'll look at that and fix.

quoted

quoted

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.

No problem, you can still register devlink_port. You don't have to do
much in order to do so.

Is there any write-up to help guide developers new to devlink in using the
interface correctly?  I haven't found much yet, but perhaps I've missed
something.  The manpages are somewhat useful in showing what the user might
do, but they really don't help much in guiding the developer through these
details.

That is not job of a manpage. See the rest of the code to get inspired.

sln