From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Subject: Re: [PATCH net-next v3 02/11] devlink: Add callback to query for
 snapshot id before snapshot create
Date: Thu, 12 Jul 2018 17:51:28 -0700
Message-ID: <20180712175128.457848ec@cakuba.lan>
References: <1531397598-11207-1-git-send-email-valex@mellanox.com>
        <1531397598-11207-3-git-send-email-valex@mellanox.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: netdev@vger.kernel.org, jiri@mellanox.com, dsahern@gmail.com,
        andrew@lunn.ch, rahul.lakkireddy@chelsio.com
To: Alex Vesker <valex@mellanox.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-qk0-f180.google.com ([209.85.220.180]:41680 "EHLO
        mail-qk0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S2387827AbeGMBDi (ORCPT
        <rfc822;netdev@vger.kernel.org>); Thu, 12 Jul 2018 21:03:38 -0400
Received: by mail-qk0-f180.google.com with SMTP id d22-v6so16423201qkc.8
        for <netdev@vger.kernel.org>; Thu, 12 Jul 2018 17:51:32 -0700 (PDT)
In-Reply-To: <1531397598-11207-3-git-send-email-valex@mellanox.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Thu, 12 Jul 2018 15:13:09 +0300, Alex Vesker wrote:
> To restrict the driver with the snapshot ID selection a new callback
> is introduced for the driver to get the snapshot ID before creating
> a new snapshot. This will also allow giving the same ID for multiple
> snapshots taken of different regions on the same time.

I'm not in position to criticize other people's commit messages :), but
I find this one hard to parse.  I think what you meant to say is that
you add a helper for numbering the snapshot per-devlink instance.
There is no callback to be seen here.  You *prevent* from giving the
same ID to multiple snapshot even if they are from different regions.

> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index cac8561..6c92ddd 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -4193,6 +4193,27 @@ void devlink_region_destroy(struct devlink_region *region)
>  }
>  EXPORT_SYMBOL_GPL(devlink_region_destroy);
>  
> +/**
> + *	devlink_region_shapshot_id_get - get snapshot ID
> + *
> + *	This callback should be called when adding a new snapshot,
> + *	Driver should use the same id for multiple snapshots taken
> + *	on multiple regions at the same time/by the same trigger.
> + *
> + *	@devlink: devlink
> + */
> +u32 devlink_region_shapshot_id_get(struct devlink *devlink)
> +{
> +	u32 id;
> +
> +	mutex_lock(&devlink->lock);
> +	id = ++devlink->snapshot_id;

Any reason not to use an IDA?  The reuse may seem unlikely, OTOH IDA
isn't going to cost much, so why risk it...

> +	mutex_unlock(&devlink->lock);
> +
> +	return id;
> +}
> +EXPORT_SYMBOL_GPL(devlink_region_shapshot_id_get);

Sorry for only spotting this now.