On Wed, Sep 25, 2013 at 05:35:54PM -0600, Bjorn Helgaas wrote:
On Wed, Sep 25, 2013 at 5:23 PM, Neil Horman <nhorman@xxxxxxxxxxxxx> wrote:
On Wed, Sep 25, 2013 at 03:08:05PM -0600, Bjorn Helgaas wrote:
[+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.
Its been a few years now, but IIRC I did the pci_dev_get/put here to ensure that
people didn't try to remove the device prior to freeing all their interrupts
(i.e I didn't want a broken driver to go through its remove routine without
freeing all its irqs). That might have been the wrong thing to do, but thats
what bubbles to the front of my head when looking at this.
That sounds plausible, but I think I'd rather deal with that by having
the PCI core remove logic free all the interrupts. I *think* that's
already in place, i.e., pci_free_resources() calls
msi_remove_pci_irq_vectors(). So I propose that we remove the
pci_dev_get()/put() unless we come up with a more compelling reason
for it.
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.
I think thats exactly why I did it, because of the documentation. I agree
however, it does look redundant. Harmless, but redundant.
OK, thanks. I think we should remove it on the grounds that it's not
needed and removing it will make this code look more similar to other
callers of kobject_init_and_add(), which means bugs will have fewer
places to hide.
Hi,
Sounds great, I'll add this as new patches and send this in a separate
patchset with the unregister msi_kset in free_msi_irqs patch, so that it
won't depend on the fix (both removing kobject_del() and
pci_dev_get/put()).
Thank you!
Thanks, Neil!
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
