Re: [PATCH 06/10] nvme: track subsystems

[Keith Busch <keith.busch@intel.com>]

Looks great. A few minor comments below.

On Wed, Aug 23, 2017 at 07:58:11PM +0200, Christoph Hellwig wrote:
> +static struct nvme_subsystem *__nvme_find_get_subsystem(const char *subsysnqn)
> +{
> +	struct nvme_subsystem *subsys;
> +
> +	lockdep_assert_held(&nvme_subsystems_lock);
> +
> +	list_for_each_entry(subsys, &nvme_subsystems, entry) {
> +		if (strcmp(subsys->subnqn, subsysnqn))
> +			continue;
> +		if (!kref_get_unless_zero(&subsys->ref))
> +			continue;

You should be able to just return immediately here since there can't be
a duplicated subsysnqn in the list.

> +		return subsys;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
> +{
> +	struct nvme_subsystem *subsys, *found;
> +
> +	subsys = kzalloc(sizeof(*subsys), GFP_KERNEL);
> +	if (!subsys)
> +		return -ENOMEM;
> +	INIT_LIST_HEAD(&subsys->ctrls);
> +	kref_init(&subsys->ref);
> +	nvme_init_subnqn(subsys, ctrl, id);
> +	mutex_init(&subsys->lock);
> +
> +	mutex_lock(&nvme_subsystems_lock);

This could be a spinlock instead of a mutex.

> +	found = __nvme_find_get_subsystem(subsys->subnqn);
> +	if (found) {
> +		/*
> +		 * Verify that the subsystem actually supports multiple
> +		 * controllers, else bail out.
> +		 */
> +		kfree(subsys);
> +		if (!(id->cmic & (1 << 1))) {
> +			dev_err(ctrl->device,
> +				"ignoring ctrl due to duplicate subnqn (%s).\n",
> +				found->subnqn);
> +			mutex_unlock(&nvme_subsystems_lock);
> +			return -EINVAL;

Returning -EINVAL here will cause nvme_init_identify to fail. Do we want
that to happen here? I think we want to be able to manage controllers
in such a state, but just checking if there's a good reason to not allow
them.

> +		}
> +
> +		subsys = found;
> +	} else {
> +		list_add_tail(&subsys->entry, &nvme_subsystems);
> +	}
> +
> +	ctrl->subsys = subsys;
> +	mutex_unlock(&nvme_subsystems_lock);
> +
> +	mutex_lock(&subsys->lock);
> +	list_add_tail(&ctrl->subsys_entry, &subsys->ctrls);
> +	mutex_unlock(&subsys->lock);
> +
> +	return 0;
>  }
>  
>  /*
> @@ -1801,7 +1882,11 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
>  		return -EIO;
>  	}
>  
> -	nvme_init_subnqn(ctrl, id);
> +	ret = nvme_init_subsystem(ctrl, id);
> +	if (ret) {
> +		kfree(id);
> +		return ret;
> +	}