Re: [Xen-devel] [PATCH] xen: use PCI interfaces to request IO and MEM resources on platform device.

[Ian Campbell <Ian.Campbell@eu.citrix.com>]

On Tue, 2011-01-11 at 12:48 +0000, Jan Beulich wrote:
> >>> On 11.01.11 at 12:50, Ian Campbell <ian.campbell@citrix.com> wrote:
> > This is the correct interface to use and something has broken the use
> > of the previous incorrect interface (which fails because the request
> > conflicts with the resources assigned for the PCI device itself
> > instead of nesting like the PCI interfaces do).
> 
> While I agree that using the PCI interface is preferable, I can't see
> how this can make a functional difference: __pci_request_region()
> calls request_region() for the port resource and
> __request_mem_region() for the mmio one exactly like what was
> open coded before (request_mem_region() being equivalent to
> __request_mem_region(..., 0), so I don't follow how this fixes
> anything

request_mem_region was failing, the requested region appeared to
conflict with the top level "PCI mem" resource. I don't know why and I
didn't investigate deeper because using the pci_* interfaces is correct
in its own right.

The old way was correct circa 2.6.37-rc5 but broken in 2.6.37 final,
there were a bunch of (reverts of) PCI resource changes in that window,
If I had to make a guess I'd say it was something in there which did it.

>  but half of the potentially incorrect use of "long" for
> mmio_addr.

Hmm yes, that should probably have a better type, although we probably
know it's going to be <4G. Can you spin a patch please?

> I'm mainly asking because if it really addresses some problem,
> then unmodified_drivers/linux-2.6/platform-pci/platform-pci.c
> in -unstable should probably get updated too.

Assuming the pci_* interfaces exist back as far as we care about for
those drivers then I don't see why not to apply it.

Ian.

> 
> Jan
> 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: stable@kernel.org # 2.6.37 only
> > ---
> >  drivers/xen/platform-pci.c |   21 +++++++--------------
> >  1 files changed, 7 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/xen/platform-pci.c b/drivers/xen/platform-pci.c
> > index c01b5dd..afbe041 100644
> > --- a/drivers/xen/platform-pci.c
> > +++ b/drivers/xen/platform-pci.c
> > @@ -105,7 +105,7 @@ static int __devinit platform_pci_init(struct pci_dev 
> > *pdev,
> >  				       const struct pci_device_id *ent)
> >  {
> >  	int i, ret;
> > -	long ioaddr, iolen;
> > +	long ioaddr;
> >  	long mmio_addr, mmio_len;
> >  	unsigned int max_nr_gframes;
> >  
> > @@ -114,7 +114,6 @@ static int __devinit platform_pci_init(struct pci_dev 
> > *pdev,
> >  		return i;
> >  
> >  	ioaddr = pci_resource_start(pdev, 0);
> > -	iolen = pci_resource_len(pdev, 0);
> >  
> >  	mmio_addr = pci_resource_start(pdev, 1);
> >  	mmio_len = pci_resource_len(pdev, 1);
> > @@ -125,19 +124,13 @@ static int __devinit platform_pci_init(struct pci_dev 
> > *pdev,
> >  		goto pci_out;
> >  	}
> >  
> > -	if (request_mem_region(mmio_addr, mmio_len, DRV_NAME) == NULL) {
> > -		dev_err(&pdev->dev, "MEM I/O resource 0x%lx @ 0x%lx busy\n",
> > -		       mmio_addr, mmio_len);
> > -		ret = -EBUSY;
> > +	ret = pci_request_region(pdev, 1, DRV_NAME);
> > +	if (ret < 0)
> >  		goto pci_out;
> > -	}
> >  
> > -	if (request_region(ioaddr, iolen, DRV_NAME) == NULL) {
> > -		dev_err(&pdev->dev, "I/O resource 0x%lx @ 0x%lx busy\n",
> > -		       iolen, ioaddr);
> > -		ret = -EBUSY;
> > +	ret = pci_request_region(pdev, 0, DRV_NAME);
> > +	if (ret < 0)
> >  		goto mem_out;
> > -	}
> >  
> >  	platform_mmio = mmio_addr;
> >  	platform_mmiolen = mmio_len;
> > @@ -169,9 +162,9 @@ static int __devinit platform_pci_init(struct pci_dev 
> > *pdev,
> >  	return 0;
> >  
> >  out:
> > -	release_region(ioaddr, iolen);
> > +	pci_release_region(pdev, 0);
> >  mem_out:
> > -	release_mem_region(mmio_addr, mmio_len);
> > +	pci_release_region(pdev, 1);
> >  pci_out:
> >  	pci_disable_device(pdev);
> >  	return ret;
> 
> 
> 
>