Re: [PATCH v3 1/3] msi: free msi_desc entry only after we've releasedthe kobject

[Linus Torvalds]

On Tue, Oct 29, 2013 at 3:30 AM, Veaceslav Falico <vfalico@xxxxxxxxxx> 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?

                Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/