Message-ID: <dddb1a17-991a-30b6-a1ad-b7c5bc05348a@pensando.io>
Date:   Wed, 10 Jul 2019 10:06:05 -0700
From:   Shannon Nelson <snelson@...sando.io>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH v3 net-next 19/19] ionic: Add basic devlink interface

On 7/9/19 11:48 PM, Jiri Pirko wrote:
> Tue, Jul 09, 2019 at 09:13:53PM CEST, snelson@...sando.io wrote:
>> On 7/8/19 11:56 PM, Jiri Pirko wrote:
>>> Tue, Jul 09, 2019 at 12:58:00AM CEST, snelson@...sando.io wrote:
>>>> On 7/8/19 1:03 PM, Jiri Pirko wrote:
>>>>> Mon, Jul 08, 2019 at 09:58:09PM CEST, snelson@...sando.io wrote:

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

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

Powered by blists - more mailing lists