Re: [PATCH 1/3] msi: add forgotten pci_dev_put(pdev) to populate_msi_sysfs()

From: Bjorn Helgaas
Date: Wed Sep 25 2013 - 17:08:33 EST

[+cc Neil (he added this code in da8d1c8ba4), Greg]

On Mon, Sep 16, 2013 at 7:47 PM, Veaceslav Falico <vfalico@xxxxxxxxxx> 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@xxxxxxxxxx>
> CC: linux-pci@xxxxxxxxxxxxxxx
> CC: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Veaceslav Falico <vfalico@xxxxxxxxxx>
> ---
> 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

list_for_each_entry(entry, &pdev->msi_list, list) {
if (!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) {


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

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at