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

[Jiri Pirko <jiri@resnulli.us>]

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.


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


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