WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

Re: [Xen-devel] Re: [PATCH 20/23] xen-pcifront: Xen PCI frontend driver.

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