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

[Neil Horman]

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.

> 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.

> 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.
> 
As far as this change goes, I think it looks ok

Acked-by: Neil Horman <nhorman@xxxxxxxxxxxxx>


> Bjorn
> 
--
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/