On 7/9/19 11:48 PM, Jiri Pirko wrote:

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

quoted

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

quoted

quoted

quoted

quoted

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.

Well, there's a bit of logic rework to and a little data twiddling - not 
too bad in our case.  Others may not be thrilled depending on how 
they've already implemented their drivers.

quoted

quoted

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.

Sure, we should all be able to poke through the code and figure out the 
basics - "use the Force, read the source" - but as software engineers we 
should be including some bits of documentation to help those new to the 
feature to steer away from pitfalls and use the feature correctly.  
We're all busy with our own projects and only have limited time to dig 
into and understand someone else's code; if there's not a guide, we'll 
do what we can to get it working and then move on, with no guarantee 
that we followed the original intent.

There's a Documentation page on the devlink-health feature, and a brief 
bit on devlink-params, but I haven't seen anything yet that spells out 
the "proper" way to use the devlink framework.  Of course, the 
open-source spirit is for me to scratch my own itch and take care of the 
need myself: I'd be happy to get a brief doc started, but if the 
original developers can take a few minutes to at least sketch some notes 
down about important bits like "the device struct should be associated 
with devlink_port" and why it should, then we have a chance at saving a 
lot of other people's time, and perhaps we can fill out the details 
correctly and not miss something important.

sln