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

[Jan Beulich]

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

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