Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 11 Dec 2019 09:58:54 -0800
From:   Jakub Kicinski <jakub.kicinski@netronome.com>
To:     Yuval Avnery <yuvalav@mellanox.com>
Cc:     jiri@mellanox.com, davem@davemloft.net, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] netdevsim: Add max_vfs to bus_dev
Message-ID: <20191211095854.6cd860f1@cakuba.netronome.com>
In-Reply-To: <1576033133-18845-1-git-send-email-yuvalav@mellanox.com>
References: <1576033133-18845-1-git-send-email-yuvalav@mellanox.com>
Organization: Netronome Systems, Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 11 Dec 2019 04:58:53 +0200, Yuval Avnery wrote:
> Currently there is no limit to the number of VFs netdevsim can enable.
> In a real systems this value exist and used by driver.
> Fore example, Some features might need to consider this value when
> allocating memory.

Thanks for the patch!

Can you shed a little bit more light on where it pops up? Just for my
curiosity?

> Signed-off-by: Yuval Avnery <yuvalav@mellanox.com>
> Acked-by: Jiri Pirko <jiri@mellanox.com>

> diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
> index 6aeed0c600f8..f1a0171080cb 100644
> --- a/drivers/net/netdevsim/bus.c
> +++ b/drivers/net/netdevsim/bus.c
> @@ -26,9 +26,9 @@ static struct nsim_bus_dev *to_nsim_bus_dev(struct device *dev)
>  static int nsim_bus_dev_vfs_enable(struct nsim_bus_dev *nsim_bus_dev,
>  				   unsigned int num_vfs)
>  {
> -	nsim_bus_dev->vfconfigs = kcalloc(num_vfs,
> -					  sizeof(struct nsim_vf_config),
> -					  GFP_KERNEL);

You're changing the semantics of the enable/disable as well now.
The old values used to be wiped when SR-IOV is disabled, now they
will be retained across disable/enable pair.

I think it'd be better if that wasn't the case. Users may expect a
system to be in the same state after they enable SR-IOV, regardless if
someone else used SR-IOV since last reboot.

Could you add a memset(,0,) here?

> +	if (nsim_bus_dev->max_vfs < num_vfs)
> +		return -ENOMEM;
> +
>  	if (!nsim_bus_dev->vfconfigs)
>  		return -ENOMEM;

This check seems useless now, no? We will always have vfconfigs

>  	nsim_bus_dev->num_vfs = num_vfs;
> @@ -38,8 +38,6 @@ static int nsim_bus_dev_vfs_enable(struct nsim_bus_dev *nsim_bus_dev,
>  
>  static void nsim_bus_dev_vfs_disable(struct nsim_bus_dev *nsim_bus_dev)
>  {
> -	kfree(nsim_bus_dev->vfconfigs);
> -	nsim_bus_dev->vfconfigs = NULL;
>  	nsim_bus_dev->num_vfs = 0;
>  }
>  
> @@ -154,22 +152,29 @@ static struct device_type nsim_bus_dev_type = {
>  };
>  
>  static struct nsim_bus_dev *
> -nsim_bus_dev_new(unsigned int id, unsigned int port_count);
> +nsim_bus_dev_new(unsigned int id, unsigned int port_count,
> +		 unsigned int max_vfs);
> +
> +#define NSIM_BUS_DEV_MAX_VFS 4
>  
>  static ssize_t
>  new_device_store(struct bus_type *bus, const char *buf, size_t count)
>  {
>  	struct nsim_bus_dev *nsim_bus_dev;
>  	unsigned int port_count;
> +	unsigned int max_vfs;
>  	unsigned int id;
>  	int err;
>  
> -	err = sscanf(buf, "%u %u", &id, &port_count);
> +	err = sscanf(buf, "%u %u %u", &id, &port_count, &max_vfs);
>  	switch (err) {
>  	case 1:
>  		port_count = 1;
>  		/* fall through */
>  	case 2:
> +		max_vfs = NSIM_BUS_DEV_MAX_VFS;
> +		/* fall through */
> +	case 3:
>  		if (id > INT_MAX) {
>  			pr_err("Value of \"id\" is too big.\n");
>  			return -EINVAL;

Is 0 VFs okay? will kcalloc(0, size, flags) behave correctly?

> @@ -179,7 +184,7 @@ new_device_store(struct bus_type *bus, const char *buf, size_t count)
>  		pr_err("Format for adding new device is \"id port_count\" (uint uint).\n");
>  		return -EINVAL;
>  	}
> -	nsim_bus_dev = nsim_bus_dev_new(id, port_count);
> +	nsim_bus_dev = nsim_bus_dev_new(id, port_count, max_vfs);
>  	if (IS_ERR(nsim_bus_dev))
>  		return PTR_ERR(nsim_bus_dev);
>  
> @@ -267,7 +272,8 @@ static struct bus_type nsim_bus = {
>  };
>  
>  static struct nsim_bus_dev *
> -nsim_bus_dev_new(unsigned int id, unsigned int port_count)
> +nsim_bus_dev_new(unsigned int id, unsigned int port_count,
> +		 unsigned int max_vfs)
>  {
>  	struct nsim_bus_dev *nsim_bus_dev;
>  	int err;
> @@ -284,12 +290,24 @@ nsim_bus_dev_new(unsigned int id, unsigned int port_count)
>  	nsim_bus_dev->dev.type = &nsim_bus_dev_type;
>  	nsim_bus_dev->port_count = port_count;
>  	nsim_bus_dev->initial_net = current->nsproxy->net_ns;
> +	nsim_bus_dev->max_vfs = max_vfs;
> +
> +	nsim_bus_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs,
> +					  sizeof(struct nsim_vf_config),
> +					  GFP_KERNEL);
> +	if (!nsim_bus_dev->vfconfigs) {
> +		err = -ENOMEM;
> +		goto err_nsim_bus_dev_id_free;
> +	}
>  
>  	err = device_register(&nsim_bus_dev->dev);
>  	if (err)
> -		goto err_nsim_bus_dev_id_free;
> +		goto err_nsim_vfconfigs_free;
> +
>  	return nsim_bus_dev;
>  
> +err_nsim_vfconfigs_free:
> +	kfree(nsim_bus_dev->vfconfigs);
>  err_nsim_bus_dev_id_free:
>  	ida_free(&nsim_bus_dev_ids, nsim_bus_dev->dev.id);
>  err_nsim_bus_dev_free:
> @@ -301,6 +319,7 @@ static void nsim_bus_dev_del(struct nsim_bus_dev *nsim_bus_dev)
>  {
>  	device_unregister(&nsim_bus_dev->dev);
>  	ida_free(&nsim_bus_dev_ids, nsim_bus_dev->dev.id);
> +	kfree(nsim_bus_dev->vfconfigs);
>  	kfree(nsim_bus_dev);
>  }
>  
> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
> index 94df795ef4d3..e2049856add8 100644
> --- a/drivers/net/netdevsim/netdevsim.h
> +++ b/drivers/net/netdevsim/netdevsim.h
> @@ -238,6 +238,7 @@ struct nsim_bus_dev {
>  	struct net *initial_net; /* Purpose of this is to carry net pointer
>  				  * during the probe time only.
>  				  */
> +	unsigned int max_vfs;
>  	unsigned int num_vfs;
>  	struct nsim_vf_config *vfconfigs;
>  };