lkml.org 
[lkml]   [2013]   [Sep]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 1/3] msi: add forgotten pci_dev_put(pdev) to populate_msi_sysfs()
    [+cc Neil (he added this code in da8d1c8ba4), Greg]

    On Mon, Sep 16, 2013 at 7:47 PM, Veaceslav Falico <vfalico@redhat.com> wrote:
    > Before trying to kobject_init_and_add(), we add a reference to pdev via
    > pci_dev_get(pdev). However, if it fails to init and/or add the kobject, we
    > don't return it back - even on out_unroll.
    >
    > Fix this by adding pci_dev_put(pdev) before going to unrolling section.
    >
    > CC: Bjorn Helgaas <bhelgaas@google.com>
    > CC: linux-pci@vger.kernel.org
    > CC: linux-kernel@vger.kernel.org
    > Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
    > ---
    > drivers/pci/msi.c | 4 +++-
    > 1 file changed, 3 insertions(+), 1 deletion(-)
    >
    > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
    > index d5f90d6..14bf578 100644
    > --- a/drivers/pci/msi.c
    > +++ b/drivers/pci/msi.c
    > @@ -534,8 +534,10 @@ static int populate_msi_sysfs(struct pci_dev *pdev)
    > pci_dev_get(pdev);
    > ret = kobject_init_and_add(kobj, &msi_irq_ktype, NULL,
    > "%u", entry->irq);
    > - if (ret)
    > + if (ret) {
    > + pci_dev_put(pdev);
    > goto out_unroll;
    > + }
    >
    > count++;
    > }

    I don't understand why this code does the pci_dev_get() in the first
    place. The pdev->msi_list of msi_desc structs is private to the
    pci_dev, and even without bumping the refcount, there should be no way
    for the pci_dev to be freed before the msi_desc.

    I also don't understand this nearby code (the same pattern appears in
    free_msi_irqs()):

    out_unroll:
    list_for_each_entry(entry, &pdev->msi_list, list) {
    if (!count)
    break;
    kobject_del(&entry->kobj);
    kobject_put(&entry->kobj);
    count--;
    }

    Why do we call kobject_del() here? The kobject_put() will call
    kobject_del() anyway, so it looks redundant.
    Documentation/kobject.txt says kobject_del() must be called explicitly
    to break a circular reference, but I don't think we have that here.

    Also, I think it is incorrect that free_msi_irqs() does this:

    if (entry->kobj.parent) {
    kobject_del(&entry->kobj);
    kobject_put(&entry->kobj);
    }

    list_del(&entry->list);
    kfree(entry);

    I think the "kfree(entry)" should be in msi_kobj_release() instead.

    Bjorn


    \
     
     \ /
      Last update: 2013-09-25 23:21    [W:3.924 / U:0.116 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site