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

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

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


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

>>> 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 but half of the potentially incorrect use of "long" for
mmio_addr.

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.

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;




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

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


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

>>> On 11.01.11 at 14:01, Ian Campbell
<Ian.Campbell@eu.citrix.com> wrote:
> 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.
Was it perhaps broken somewhere between -rc5 and -final, but
not in -final itself?
>> 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.
I checked back to .5, where they exist; the question is - how far
back do we care?

Jan


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

On Tue, 2011-01-11 at 14:10 +0000, Jan Beulich wrote:
> >>> On 11.01.11 at 14:01, Ian Campbell
<Ian.Campbell@eu.citrix.com> wrote:
> > 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.
> 
> Was it perhaps broken somewhere between -rc5 and -final, but
> not in -final itself?
No I''m working with 2.6.37 final and it is broken there.
> >> 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.
> 
> I checked back to .5, where they exist; the question is - how far
> back do we care?
You mean 2.6.5? That''s old enough to be the cut off IMHO.

I''m not sure how much actual use these drivers get these days though.

Ian.



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

>>> On 11.01.11 at 15:18, Ian Campbell
<Ian.Campbell@eu.citrix.com> wrote:
>> >> 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.
>> 
>> I checked back to .5, where they exist; the question is - how far
>> back do we care?
> 
> You mean 2.6.5? That''s old enough to be the cut off IMHO.
> 
> I''m not sure how much actual use these drivers get these days
though.
We''re building ours from that code.

Are you going to submit this for -unstable then too, or are you
expecting me to do so?

Jan


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

On Tue, 2011-01-11 at 14:27 +0000, Jan Beulich wrote:
> >>> On 11.01.11 at 15:18, Ian Campbell
<Ian.Campbell@eu.citrix.com> wrote:
> >> >> 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.
> >> 
> >> I checked back to .5, where they exist; the question is - how far
> >> back do we care?
> > 
> > You mean 2.6.5? That''s old enough to be the cut off IMHO.
> > 
> > I''m not sure how much actual use these drivers get these days
though.
> 
> We''re building ours from that code.
OK.
> Are you going to submit this for -unstable then too, or are you
> expecting me to do so?
Since you can build/test it for the cases you care about I think it
would be better if you did?

Ian.



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