On Thu, Nov 30, 2017 at 05:35:39PM -0800, Jakub Kicinski wrote:
[...]
> +static int nsim_vfs_enable(struct netdevsim *ns, unsigned int num_vfs)
> +{
> + ns->vfconfigs = kcalloc(num_vfs, sizeof(struct nsim_vf_config),
> + GFP_KERNEL);
> + if (!ns->vfconfigs)
> + return -ENOMEM;
> + ns->num_vfs = num_vfs;
> +
> + return 0;
> +}
> +
> +static void nsim_vfs_disable(struct netdevsim *ns)
> +{
> + kfree(ns->vfconfigs);
> + ns->vfconfigs = NULL;
> + ns->num_vfs = 0;
> +}
Why not something like:
| static int nsim_vfs_set(struct netdevsim *ns, unsigned int num_vfs)
| {
| void *ptr = krealloc(ns->vfconfigs,
| num_vfs * sizeof(struct nsim_vf_config),
| GFP_KERNEL);
|
| if (!ptr)
| return -ENOMEM;
|
| ns->vfconfigs = ptr;
| ns->num_vfs = num_vfs;
| return 0;
| }
> +static ssize_t
> +nsim_numvfs_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct netdevsim *ns = to_nsim(dev);
> + unsigned int num_vfs;
> + int ret;
> +
> + ret = kstrtouint(buf, 0, &num_vfs);
> + if (ret)
> + return ret;
> +
> + rtnl_lock();
> + if (ns->num_vfs == num_vfs)
> + goto exit_good;
Then replace this:
> + if (ns->num_vfs && num_vfs) {
> + ret = -EBUSY;
> + goto exit_unlock;
> + }
> +
> + if (num_vfs) {
> + ret = nsim_vfs_enable(ns, num_vfs);
> + if (ret)
> + goto exit_unlock;
> + } else {
> + nsim_vfs_disable(ns);
> + }
with just:
| nsim_vfs_set(ns, num_vfs);
> +exit_good:
> + ret = count;
> +exit_unlock:
> + rtnl_unlock();
> +
> + return ret;
> +}
[...]
> +static void nsim_free(struct net_device *dev)
> +{
> + struct netdevsim *ns = netdev_priv(dev);
> +
> + device_unregister(&ns->dev);
> }
Shouldn't this also kfree(ns->vfconfigs)?
Cheers, Phil
