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

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

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

Also, any word on the pdev_lock you dropped from the original
implementation?

> If this is to your satisfaction, can I put a Reviewed-by tag on the patch?

Feel free to do so.

> --- /dev/null
> +++ b/drivers/pci/xen-pcifront.c
>...
> +int __devinit pcifront_scan_bus(struct pcifront_device *pdev,

static?

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

> +             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?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel