On Thu, Oct 14, 2010 at 08:15:06AM +0100, Jan Beulich wrote:
> >>> On 13.10.10 at 18:16, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> >>> wrote:
> > On Wed, Oct 13, 2010 at 09:53:44AM -0400, Konrad Rzeszutek Wilk wrote:
> >> Hey Jan,
> >>
> >> Thank you for taking your time to look at this patch. Will fix up, test
> >> it,
> >> and if there are no issues, have it ready tomorrow.
> >
> > Attached (and inline) is the updated version of this patch. If I missed
> > anything
> > please do point it out to me!
>
> There's one more missing "static", and one incorrect change to
> free_pdev() you did.
Fixed.
>
> Also, any word on the pdev_lock you dropped from the original
> implementation?
Yes. The reason for dropping it was that the xenwatch thread provides
the neccessary locking for the states. So no more need for this
spin_lock.
>
> > If this is to your satisfaction, can I put a Reviewed-by tag on the patch?
>
> Feel free to do so.
Thank you.
>
> > --- /dev/null
> > +++ b/drivers/pci/xen-pcifront.c
> >...
> > +int __devinit pcifront_scan_bus(struct pcifront_device *pdev,
>
> static?
Yup, done.
>
> >...
> > +static void free_pdev(struct pcifront_device *pdev)
> > +{
> > + dev_dbg(&pdev->xdev->dev, "freeing pdev @ 0x%p\n", pdev);
> > +
> > + pcifront_free_roots(pdev);
> > +
> > + /*For PCIE_AER error handling job*/
> > + flush_scheduled_work();
> > +
> > + if (pdev->irq)
>
> if (pdev->irq > 0)
>
> It gets initialized to -1 in alloc_pdev(). It may be debatable whether
> it should be >= 0 - I'm not sure if the pv-ops code allows IRQ 0 to
> be used. If it doesn't, initializing to 0 in alloc_pdev() would be an
> alternative.
I made it '>=' The Xen PCI (arch/x86/pci/xen.c) and Xen Events
(riers/xen/events.c)
are both OK with an IRQ of zero. So lets be uniform and be OK here too.
>
> > + unbind_from_irqhandler(pdev->irq, pdev);
> > +
> > + if (pdev->evtchn != INVALID_EVTCHN)
> > + xenbus_free_evtchn(pdev->xdev, pdev->evtchn);
> > +
> > + if (pdev->gnt_ref != INVALID_GRANT_REF)
> > + gnttab_end_foreign_access(pdev->gnt_ref, 0 /* r/w page */,
> > + (unsigned long)pdev->sh_info);
> > + else
> > + free_page((unsigned long)pdev->sh_info);
> > +
> > + dev_set_drvdata(&pdev->xdev->dev, NULL);
> > +
> > + kfree(pdev);
> > +}
> > +
> > +static int pcifront_publish_info(struct pcifront_device *pdev)
> > +{
> > + int err = 0;
> > + struct xenbus_transaction trans;
> > +
> > + err = xenbus_grant_ring(pdev->xdev, virt_to_mfn(pdev->sh_info));
> > + if (err < 0)
> > + goto out;
> > +
> > + pdev->gnt_ref = err;
> > +
> > + err = xenbus_alloc_evtchn(pdev->xdev, &pdev->evtchn);
> > + if (err)
> > + goto out;
> > +
> > + err = bind_evtchn_to_irqhandler(pdev->evtchn, pcifront_handler_aer,
> > + 0, "pcifront", pdev);
> > + if (err < 0) {
> > + /*
> > + xenbus_free_evtchn(pdev->xdev, pdev->evtchn);
> > + xenbus_dev_fatal(pdev->xdev, err, "Failed to bind evtchn to "
> > + "irqhandler.\n");
> > + */
>
> Why are you commenting it out rather than removing it?
Umm. I was in a hurry to test it out and just in case it would fail I
made it a comment. And then forgot about it. Removed that comented out
section of code.
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home