On Wed, Aug 23, 2017 at 06:04:34PM -0400, Keith Busch wrote:
> > + 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.
We could have a race where we tear one subsystem instance down and
are setting up another one.
> > + 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.
We could. But given that the lock is not in the hot path there seems
to be no point in making it a spinlock.
> > + 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.
Without this we will get duplicate nvme_subsystem structures, messing
up the whole lookup. We could mark them as buggy with a flag and
make sure controllers without CMIC bit 1 set will never be linked.
