ARM: versatile: fix build failure in pci.c

PATCH New Paul Gortmaker 6 participants 15 messages Apr 2, 2012 – Apr 4, 2012

Subsystems (4)

Arm (32-bit) Arm sub-architectures Arm/versatile express platform Arm integrator, versatile and realview support

lore

Share & tools

lore

Paul Gortmaker

author

2012-04-02 23:48 UTC #0 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

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 <[email protected]>

Signed-off-by Paul Gortmaker <[email protected]>

arch/arm/mach-versatile/pci.c +5 -5

index a6e23f4..d2268be 100644
@@ -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;

Signature · 1 line

1.7.9.1

Fabio Estevam

2012-04-03 00:03 UTC #1 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

Hi Paul.

Quoted 9 lines · 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)

Quoted 6 lines

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 <[email protected]>
Signed-off-by Paul Gortmaker <[email protected]>

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

Regards,

Fabio Estevam

Paul Gortmaker

author

2012-04-03 01:01 UTC #2 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

Quoted 17 lines · Fabio Estevam

Hi Paul.
Quoted 13 lines · 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.

Quoted 5 lines

Quoted 2 lines
Reported-by Bruce Ashfield <[email protected]>
Signed-off-by Paul Gortmaker <[email protected]>
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.

Quoted 9 lines

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

Fabio Estevam

2012-04-03 01:57 UTC #3 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

Quoted 4 lines · Paul Gortmaker

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.

Arnd Bergmann

2012-04-03 08:19 UTC #4 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

Quoted 9 lines · Fabio Estevam

Quoted 4 lines · Paul Gortmaker
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

Paul Gortmaker

author

2012-04-03 13:47 UTC #5 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

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

Quoted 9 lines · Arnd Bergmann

Quoted 8 lines · Fabio Estevam
Quoted 4 lines · Paul Gortmaker
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?

Quoted 4 lines

Quoted 1 line
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.
--

Quoted 5 lines

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

Arnd Bergmann

2012-04-03 14:26 UTC #6 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

Quoted 4 lines · Paul Gortmaker

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

Bjorn Helgaas

2012-04-03 16:44 UTC #7 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

Quoted 6 lines · Arnd Bergmann

Quoted 4 lines · Paul Gortmaker
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.

Quoted 8 lines

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

Paul Gortmaker

author

2012-04-03 19:09 UTC #8 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

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

[...]

Quoted 4 lines

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.

Quoted 2 lines

Bjorn

Arnd Bergmann

2012-04-03 19:40 UTC #9 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

Quoted 11 lines · Paul Gortmaker

On 12-04-03 12:44 PM, Bjorn Helgaas wrote:
[...]
Quoted 4 lines
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.

Quoted 2 lines

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

Russell King - ARM Linux

2012-04-04 09:08 UTC #10 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

In reply to Arnd Bergmann

Quoted 6 lines · Arnd Bergmann

Quoted 4 lines · Paul Gortmaker
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.

Quoted 8 lines

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.

Russell King - ARM Linux

2012-04-04 09:07 UTC #11 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

In reply to Paul Gortmaker

Quoted 4 lines · Paul Gortmaker

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?

Paul Gortmaker

author

2012-04-04 13:53 UTC #12 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

Quoted 8 lines · Russell King - ARM Linux

Quoted 4 lines · Paul Gortmaker
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).

Russell King - ARM Linux

2012-04-04 14:26 UTC #13 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

Quoted 10 lines · Paul Gortmaker

Quoted 8 lines · Russell King - ARM Linux
Quoted 4 lines · Paul Gortmaker
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?

Quoted 8 lines

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

Olof Johansson

Applied

2012-04-03 04:58 UTC #14 lore raw

PATCH 1/1 ARM: versatile: fix build failure in pci.c

In reply to Paul Gortmaker

Quoted 9 lines · 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)

Quoted 6 lines

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 <[email protected]>
Signed-off-by Paul Gortmaker <[email protected]>

Thanks, applied to fixes.

-Olof