Messages in this thread | | | Date | Tue, 29 Oct 2013 14:49:53 -0700 | From | Greg Kroah-Hartman <> | 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); > > (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?
That is really odd, but I guess it works as-is because no one ever calls that function on the same kobject at the same time. I don't know what that is trying to do. There has been some work by Tejun in this area for linux-next, but that lock and logic is still there, I'll look into fixing that up...
thanks,
greg k-h
| |