Messages in this thread | | | Date | Tue, 29 Oct 2013 17:47:20 +0100 | From | Veaceslav Falico <> | Subject | Re: [PATCH v3 1/3] msi: free msi_desc entry only after we've released the kobject |
| |
On Tue, Oct 29, 2013 at 09:34:28AM -0700, Linus Torvalds wrote: >On Tue, Oct 29, 2013 at 3:30 AM, Veaceslav Falico <vfalico@redhat.com> wrote: >> /* >> * Its possible that we get into this path >> * When populate_msi_sysfs fails, which means the entries >> * were not registered with sysfs. In that case don't >> - * unregister them. >> + * unregister them, and just free. Otherwise the >> + * kobject->release will take care of freeing the entry via >> + * msi_kobj_release(). >> */ >> if (entry->kobj.parent) { >> kobject_del(&entry->kobj); >> kobject_put(&entry->kobj); >> + } else { >> + kfree(entry); >> } >> - >> - list_del(&entry->list); >> - kfree(entry); > >So this code sequence still makes me very unhappy. > >Why does not just a simple unconditional > > kobject_del(&entry->kobj); > kobject_put(&entry->kobj); > >work for the "not registered with sysfs" case? And if the sysfs code >really gets confused, why not > > if (entry->kobj.parent) > kobject_del(&entry->kobj); > kobject_put(&entry->kobj);
It was fixed this way in 424eb39 ("PCI: msi: fix imbalanced refcount of msi irq sysfs objects"). kobject_put() still failed in case it wasn't registered with sysfs earlier. populate_msi_sysfs() creates an kobject for each entry in msi_list, and we have no idea (on fallback) up to which entry was it already registered, on which it failed and which entries are still not kobject_init_and_add()ed.
> >(btw, looking at the sysfs code, this looks *very* suspicious in >sysfs_remove_dir(): > > struct sysfs_dirent *sd = kobj->sd; > > spin_lock(&sysfs_assoc_lock); > kobj->sd = NULL; > spin_unlock(&sysfs_assoc_lock); > >and I would suggest that "sd = kobj->sd" should be done under the >lock, because otherwise the lock is kind of pointless..) > >Greg? > > Linus
| |