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

[Shannon Nelson <snelson@pensando.io>]

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.

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

sln