[PATCH] ARM: versatile: fix build failure in pci.c

[PATCH] ARM: versatile: fix build failure in pci.c

[Paul Gortmaker]

commit 9f786d033d025ab7d2c4d1b959aa81d935eb9e19

    "arm/PCI: get rid of device resource fixups"

causes this failure on the versatile:

arch/arm/mach-versatile/pci.c: In function 'pci_versatile_setup_resources':
arch/arm/mach-versatile/pci.c:221: error: 'sys' undeclared (first use in this function)

because the versatile wasn't passing in the full struct pci_sys_data
but only the resource sub-field.  Change it to pass in the full
struct so that sys will be in scope.

Reported-by: Bruce Ashfield <bruce.ashfield@windriver.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

diff --git a/arch/arm/mach-versatile/pci.c b/arch/arm/mach-versatile/pci.c
index a6e23f4..d2268be 100644
--- a/arch/arm/mach-versatile/pci.c
+++ b/arch/arm/mach-versatile/pci.c
@@ -190,7 +190,7 @@ static struct resource pre_mem = {
 	.flags	= IORESOURCE_MEM | IORESOURCE_PREFETCH,
 };
 
-static int __init pci_versatile_setup_resources(struct list_head *resources)
+static int __init pci_versatile_setup_resources(struct pci_sys_data *sys)
 {
 	int ret = 0;
 
@@ -218,9 +218,9 @@ static int __init pci_versatile_setup_resources(struct list_head *resources)
 	 * the mem resource for this bus
 	 * the prefetch mem resource for this bus
 	 */
-	pci_add_resource_offset(resources, &io_mem, sys->io_offset);
-	pci_add_resource_offset(resources, &non_mem, sys->mem_offset);
-	pci_add_resource_offset(resources, &pre_mem, sys->mem_offset);
+	pci_add_resource_offset(&sys->resources, &io_mem, sys->io_offset);
+	pci_add_resource_offset(&sys->resources, &non_mem, sys->mem_offset);
+	pci_add_resource_offset(&sys->resources, &pre_mem, sys->mem_offset);
 
 	goto out;
 
@@ -249,7 +249,7 @@ int __init pci_versatile_setup(int nr, struct pci_sys_data *sys)
 
 	if (nr == 0) {
 		sys->mem_offset = 0;
-		ret = pci_versatile_setup_resources(&sys->resources);
+		ret = pci_versatile_setup_resources(sys);
 		if (ret < 0) {
 			printk("pci_versatile_setup: resources... oops?\n");
 			goto out;
-- 
1.7.9.1

[PATCH] ARM: versatile: fix build failure in pci.c

[Fabio Estevam]

Hi Paul.

On Mon, Apr 2, 2012 at 8:48 PM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:
> commit 9f786d033d025ab7d2c4d1b959aa81d935eb9e19
>
> ? ?"arm/PCI: get rid of device resource fixups"
>
> causes this failure on the versatile:
>
> arch/arm/mach-versatile/pci.c: In function 'pci_versatile_setup_resources':
> arch/arm/mach-versatile/pci.c:221: error: 'sys' undeclared (first use in this function)
>
> because the versatile wasn't passing in the full struct pci_sys_data
> but only the resource sub-field. ?Change it to pass in the full
> struct so that sys will be in scope.
>
> Reported-by: Bruce Ashfield <bruce.ashfield@windriver.com>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

I have also sent a similar patch:
http://marc.info/?l=linux-arm-kernel&m=133107329108262&w=2

Regards,

Fabio Estevam

[PATCH] ARM: versatile: fix build failure in pci.c

[Paul Gortmaker]

On Mon, Apr 2, 2012 at 8:03 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Hi Paul.
>
> On Mon, Apr 2, 2012 at 8:48 PM, Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
>> commit 9f786d033d025ab7d2c4d1b959aa81d935eb9e19
>>
>> ? ?"arm/PCI: get rid of device resource fixups"
>>
>> causes this failure on the versatile:
>>
>> arch/arm/mach-versatile/pci.c: In function 'pci_versatile_setup_resources':
>> arch/arm/mach-versatile/pci.c:221: error: 'sys' undeclared (first use in this function)
>>
>> because the versatile wasn't passing in the full struct pci_sys_data
>> but only the resource sub-field. ?Change it to pass in the full
>> struct so that sys will be in scope.
>>
>> Reported-by: Bruce Ashfield <bruce.ashfield@windriver.com>
>> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>
> I have also sent a similar patch:
> http://marc.info/?l=linux-arm-kernel&m=133107329108262&w=2

I see.  Well, there weren't any follow ups to it but I would say that
passing both sys->resources and sys both side-by-side at the same
time doesn't really make sense.  I think just passing in sys by itself
as I did is a cleaner way to go, wouldn't you agree?

Thanks,
Paul.

>
> Regards,
>
> Fabio Estevam
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] ARM: versatile: fix build failure in pci.c

[Fabio Estevam]

On Mon, Apr 2, 2012 at 10:01 PM, Paul Gortmaker
<paul.gortmaker@windriver.com> wrote:

> I see. ?Well, there weren't any follow ups to it but I would say that
> passing both sys->resources and sys both side-by-side at the same
> time doesn't really make sense. ?I think just passing in sys by itself
> as I did is a cleaner way to go, wouldn't you agree?

Yes, I agree.

[PATCH] ARM: versatile: fix build failure in pci.c

[Olof Johansson]

On Mon, Apr 02, 2012 at 07:48:25PM -0400, Paul Gortmaker wrote:
> commit 9f786d033d025ab7d2c4d1b959aa81d935eb9e19
> 
>     "arm/PCI: get rid of device resource fixups"
> 
> causes this failure on the versatile:
> 
> arch/arm/mach-versatile/pci.c: In function 'pci_versatile_setup_resources':
> arch/arm/mach-versatile/pci.c:221: error: 'sys' undeclared (first use in this function)
> 
> because the versatile wasn't passing in the full struct pci_sys_data
> but only the resource sub-field.  Change it to pass in the full
> struct so that sys will be in scope.
> 
> Reported-by: Bruce Ashfield <bruce.ashfield@windriver.com>
> Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>

Thanks, applied to fixes.


-Olof

[PATCH] ARM: versatile: fix build failure in pci.c

[Arnd Bergmann]

On Tuesday 03 April 2012, Fabio Estevam wrote:
> On Mon, Apr 2, 2012 at 10:01 PM, Paul Gortmaker
> <paul.gortmaker@windriver.com> wrote:
> 
> > I see.  Well, there weren't any follow ups to it but I would say that
> > passing both sys->resources and sys both side-by-side at the same
> > time doesn't really make sense.  I think just passing in sys by itself
> > as I did is a cleaner way to go, wouldn't you agree?
> 
> Yes, I agree.

But that doesn't work: struct pci_sys_data is ARM specific, while
pci_add_resource_offset is common code.

I think it would be much more sensible here to just use pci_add_resource(),
which is the same as the offsets are all zero for versatile.

	Arnd

[PATCH] ARM: versatile: fix build failure in pci.c

[Paul Gortmaker]

[adding Bjorn Helgaas to CC; I overlooked doing that earlier]

On 12-04-03 04:19 AM, Arnd Bergmann wrote:
> On Tuesday 03 April 2012, Fabio Estevam wrote:
>> On Mon, Apr 2, 2012 at 10:01 PM, Paul Gortmaker
>> <paul.gortmaker@windriver.com> wrote:
>>
>>> I see.  Well, there weren't any follow ups to it but I would say that
>>> passing both sys->resources and sys both side-by-side at the same
>>> time doesn't really make sense.  I think just passing in sys by itself
>>> as I did is a cleaner way to go, wouldn't you agree?
>>
>> Yes, I agree.
> 
> But that doesn't work: struct pci_sys_data is ARM specific, while
> pci_add_resource_offset is common code.

I'm not sure I'm following you here. I'm not feeding the
struct pci_sys_data to pci_add_resource_offset.  Instead it
is getting subfields of it, consistent with the rest of the
original commit from Bjorn.

-	pci_add_resource_offset(resources, &io_mem, sys->io_offset);
-	pci_add_resource_offset(resources, &non_mem, sys->mem_offset);
-	pci_add_resource_offset(resources, &pre_mem, sys->mem_offset);
+	pci_add_resource_offset(&sys->resources, &io_mem, sys->io_offset);
+	pci_add_resource_offset(&sys->resources, &non_mem, sys->mem_offset);
+	pci_add_resource_offset(&sys->resources, &pre_mem, sys->mem_offset);

As a side note, the fact that struct pci_sys_data is ARM specific
does make one think it would be nice if the name somehow did
reflect that...  wonder if it is worth changing.  Anyway, that is
a separate topic.

Thanks,
Paul.
--

> 
> I think it would be much more sensible here to just use pci_add_resource(),
> which is the same as the offsets are all zero for versatile.
> 
> 	Arnd

[PATCH] ARM: versatile: fix build failure in pci.c

[Arnd Bergmann]

On Tuesday 03 April 2012, Paul Gortmaker wrote:
> I'm not sure I'm following you here. I'm not feeding the
> struct pci_sys_data to pci_add_resource_offset.  Instead it
> is getting subfields of it, consistent with the rest of the
> original commit from Bjorn.

Sorry, my fault, I thought the suggestion was to pass sys into
pci_add_resource_offset.

However, I would still prefer just reverting the versatile
part of Bjorn's patch because versatile uses a zero offset.
I think we should only pass the offset in cases where it's
actually required because there are multiple buses or they
have interesting mappings to the physical address space.

Those seem to be the minority on ARM anyway.

	Arnd

[PATCH] ARM: versatile: fix build failure in pci.c

[Bjorn Helgaas]

On Tue, Apr 3, 2012 at 8:26 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 03 April 2012, Paul Gortmaker wrote:
>> I'm not sure I'm following you here. I'm not feeding the
>> struct pci_sys_data to pci_add_resource_offset. ?Instead it
>> is getting subfields of it, consistent with the rest of the
>> original commit from Bjorn.
>
> Sorry, my fault, I thought the suggestion was to pass sys into
> pci_add_resource_offset.
>
> However, I would still prefer just reverting the versatile
> part of Bjorn's patch because versatile uses a zero offset.
> I think we should only pass the offset in cases where it's
> actually required because there are multiple buses or they
> have interesting mappings to the physical address space.

I used pci_add_resource_offset() on *all* arm platforms because the
offsets are defined in struct pci_sys_data, which is not specific to
any platform type, and I thought it was easier and more future-proof
to pay attention to those offsets.

I was also thinking that the actual pci_add_resource_offset() calls
and the request_resource() or allocate_resource() calls usually found
near them in the arm .setup() functions are really not
platform-specific, so it might make sense to float them up into
something like pcibios_init_hw() someday.

But if you prefer to use the offset only for platforms that actually
assign non-zero values to the pci_sys_data offsets, that's fine with
me.

Since this only affects arm, do you want to handle the fix entirely in
the arm tree?  If you want me to push something through PCI, I can do
that, too.  Sorry for the breakage.

Bjorn

[PATCH] ARM: versatile: fix build failure in pci.c

[Paul Gortmaker]

On 12-04-03 12:44 PM, Bjorn Helgaas wrote:

[...]

> 
> Since this only affects arm, do you want to handle the fix entirely in
> the arm tree?  If you want me to push something through PCI, I can do
> that, too.  Sorry for the breakage.

Olof already applied my patch to fixes, so I don't
think there is any requirement for immediate action.

You and Arnd can work out whether you want to use
offsets or not on the versatile at your leisure.

Thanks,
Paul.

> 
> Bjorn

[PATCH] ARM: versatile: fix build failure in pci.c

[Arnd Bergmann]

On Tuesday 03 April 2012, Paul Gortmaker wrote:
> On 12-04-03 12:44 PM, Bjorn Helgaas wrote:
> 
> [...]
> 
> > 
> > Since this only affects arm, do you want to handle the fix entirely in
> > the arm tree?  If you want me to push something through PCI, I can do
> > that, too.  Sorry for the breakage.
> 
> Olof already applied my patch to fixes, so I don't
> think there is any requirement for immediate action.

Yes, and this is the important part for now, making this look nice can
wait. My preference would be to get rid of the io_offset and mem_offset
in as many places as possible in the long run, but we don't have to do
that for v3.4.

> You and Arnd can work out whether you want to use
> offsets or not on the versatile at your leisure.

I have a patch set for versatile PCI that I should have sent a long
time ago. When I resubmit that, I'll take care of this, too.

	Arnd

[PATCH] ARM: versatile: fix build failure in pci.c

[Russell King - ARM Linux]

On Tue, Apr 03, 2012 at 09:47:13AM -0400, Paul Gortmaker wrote:
> As a side note, the fact that struct pci_sys_data is ARM specific
> does make one think it would be nice if the name somehow did
> reflect that...  wonder if it is worth changing.  Anyway, that is
> a separate topic.

And what's wrong with the existing name when it's limited to only
ARM specific files?

[PATCH] ARM: versatile: fix build failure in pci.c

[Russell King - ARM Linux]

On Tue, Apr 03, 2012 at 02:26:22PM +0000, Arnd Bergmann wrote:
> On Tuesday 03 April 2012, Paul Gortmaker wrote:
> > I'm not sure I'm following you here. I'm not feeding the
> > struct pci_sys_data to pci_add_resource_offset.  Instead it
> > is getting subfields of it, consistent with the rest of the
> > original commit from Bjorn.
> 
> Sorry, my fault, I thought the suggestion was to pass sys into
> pci_add_resource_offset.
> 
> However, I would still prefer just reverting the versatile
> part of Bjorn's patch because versatile uses a zero offset.
> I think we should only pass the offset in cases where it's
> actually required because there are multiple buses or they
> have interesting mappings to the physical address space.

Or they do things the right way with IO space.

[PATCH] ARM: versatile: fix build failure in pci.c

[Paul Gortmaker]

On 12-04-04 05:07 AM, Russell King - ARM Linux wrote:
> On Tue, Apr 03, 2012 at 09:47:13AM -0400, Paul Gortmaker wrote:
>> As a side note, the fact that struct pci_sys_data is ARM specific
>> does make one think it would be nice if the name somehow did
>> reflect that...  wonder if it is worth changing.  Anyway, that is
>> a separate topic.
> 
> And what's wrong with the existing name when it's limited to only
> ARM specific files?

Just in the context of Arnd's original comment, he thought I
screwed up by feeding an ARM specific struct into generic
PCI code, because I wasn't aware it was ARM specific.

That wasn't the case, but he's right that something like that
could happen  A minor nit, sure, but if it was something like
pci_arm_data, the name itself would convey it was arm specific,
even in reduced context scenarios (like grep output etc).

P.

[PATCH] ARM: versatile: fix build failure in pci.c

[Russell King - ARM Linux]

On Wed, Apr 04, 2012 at 09:53:08AM -0400, Paul Gortmaker wrote:
> On 12-04-04 05:07 AM, Russell King - ARM Linux wrote:
> > On Tue, Apr 03, 2012 at 09:47:13AM -0400, Paul Gortmaker wrote:
> >> As a side note, the fact that struct pci_sys_data is ARM specific
> >> does make one think it would be nice if the name somehow did
> >> reflect that...  wonder if it is worth changing.  Anyway, that is
> >> a separate topic.
> > 
> > And what's wrong with the existing name when it's limited to only
> > ARM specific files?
> 
> Just in the context of Arnd's original comment, he thought I
> screwed up by feeding an ARM specific struct into generic
> PCI code, because I wasn't aware it was ARM specific.
> 
> That wasn't the case, but he's right that something like that
> could happen  A minor nit, sure, but if it was something like
> pci_arm_data, the name itself would convey it was arm specific,
> even in reduced context scenarios (like grep output etc).

If the struct ends up being fed into non-ARM code, then non-ARM code
isn't going to be able to dereference it because there isn't a definition
for it outside of arch/arm/.

The risk which exists is that generic code invents its own pci_sys_data.
At that point, we end up with two definitions for the struct, and the
compiler will error out.

We really don't need the churn of changing this.  It ain't broken, so
don't try to fix it.  Moreover, you'll only end up with conflicts to
deal with if the ixp2xxx stuff is removed (and other people will have
similar problems if they're dealing with code in this area too.)