Re: [PATCH v3 net-next 19/19] ionic: Add basic devlink interface

[Jiri Pirko <jiri@resnulli.us>]

Tue, Jul 09, 2019 at 09:13:53PM CEST, snelson@pensando.io wrote:
>On 7/8/19 11:56 PM, Jiri Pirko wrote:
>> Tue, Jul 09, 2019 at 12:58:00AM CEST, snelson@pensando.io wrote:
>> > On 7/8/19 1:03 PM, Jiri Pirko wrote:
>> > > Mon, Jul 08, 2019 at 09:58:09PM CEST, snelson@pensando.io wrote:
>> > > > On 7/8/19 12:34 PM, Jiri Pirko wrote:
>> > > > > Mon, Jul 08, 2019 at 09:25:32PM CEST, snelson@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
>> 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.


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