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

[Shannon Nelson <snelson@pensando.io>]

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

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

sln

>
>
>> That unused struct ionic_devlink does need to go away, it was superfluous
>> after working out a better typecast off of devlink_priv().
>>
>> I'll remove the unused struct ionic_devlink, but I think the rest is okay.
>>
>> sln
>>
>>>
>>>> +	if (!dl) {
>>>> +		dev_warn(ionic->dev, "devlink_alloc failed");
>>>> +		return -ENOMEM;
>>>> +	}
>>>> +
>>>> +	ip = (struct ionic **)devlink_priv(dl);
>>>> +	*ip = ionic;
>>>> +	ionic->dl = dl;
>>>> +
>>>> +	err = devlink_register(dl, ionic->dev);
>>>> +	if (err) {
>>>> +		dev_warn(ionic->dev, "devlink_register failed: %d\n", err);
>>>> +		goto err_dl_free;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +
>>>> +err_dl_free:
>>>> +	ionic->dl = NULL;
>>>> +	devlink_free(dl);
>>>> +	return err;
>>>> +}
>>>> +
>>>> +void ionic_devlink_unregister(struct ionic *ionic)
>>>> +{
>>>> +	if (!ionic->dl)
>>>> +		return;
>>>> +
>>>> +	devlink_unregister(ionic->dl);
>>>> +	devlink_free(ionic->dl);
>>>> +}
>>>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.h b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
>>>> new file mode 100644
>>>> index 000000000000..35528884e29f
>>>> --- /dev/null
>>>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.h
>>>> @@ -0,0 +1,12 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/* Copyright(c) 2017 - 2019 Pensando Systems, Inc */
>>>> +
>>>> +#ifndef _IONIC_DEVLINK_H_
>>>> +#define _IONIC_DEVLINK_H_
>>>> +
>>>> +#include <net/devlink.h>
>>>> +
>>>> +int ionic_devlink_register(struct ionic *ionic);
>>>> +void ionic_devlink_unregister(struct ionic *ionic);
>>>> +
>>>> +#endif /* _IONIC_DEVLINK_H_ */
>>>> -- 
>>>> 2.17.1
>>>>