[PATCH] drivers: pci: remove unused pci_sys_data structures

[PATCH] drivers: pci: remove unused pci_sys_data structures

[Lorenzo Pieralisi]

Commit b3a72384fe29 ("ARM/PCI: Replace pci_sys_data->align_resource
with global function pointer") removed the struct pci_sys_data
dependency from the ARM pcibios functions that are part of the
common ARM PCI arch back-end (eg pcibios_align_resource()), so that
struct pci_sys_data has now become data that is only used internally
by the ARM bios32 layer (ie pci_common_init_dev()) and by host
controllers drivers callbacks (eg pci_sys_data.setup) that rely on the
ARM bios32 API to probe.

PCI host controller drivers that do not rely on ARM bios32 calls to
probe do not need to have the pci_bus.sysdata pointer field pointing
at a struct pci_sys_data anymore, therefore it can be removed from the
respective drivers data structures.

This patch removes the pci_sys_data structures from the host
controller drivers that do not rely on ARM bios32 interface to
scan the PCI bus, completing the pci_sys_data clean-up and removing
the related dependency on arch/arm specific data.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh@kernel.org>
---
Rob,

I could not test it on versatile, only compile tested, so
please have an additional look.

Tested the PCI host generic on 32-bit and 64-bit guests,
through kvmtool.

Thanks,
Lorenzo

 drivers/pci/host/pci-host-generic.c | 9 ---------
 drivers/pci/host/pci-versatile.c    | 5 +----
 2 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 5434c90..1652bc7 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -38,16 +38,7 @@ struct gen_pci_cfg_windows {
 	struct gen_pci_cfg_bus_ops		*ops;
 };
 
-/*
- * ARM pcibios functions expect the ARM struct pci_sys_data as the PCI
- * sysdata.  Add pci_sys_data as the first element in struct gen_pci so
- * that when we use a gen_pci pointer as sysdata, it is also a pointer to
- * a struct pci_sys_data.
- */
 struct gen_pci {
-#ifdef CONFIG_ARM
-	struct pci_sys_data			sys;
-#endif
 	struct pci_host_bridge			host;
 	struct gen_pci_cfg_windows		cfg;
 	struct list_head			resources;
diff --git a/drivers/pci/host/pci-versatile.c b/drivers/pci/host/pci-versatile.c
index 0863d9c..f843a72 100644
--- a/drivers/pci/host/pci-versatile.c
+++ b/drivers/pci/host/pci-versatile.c
@@ -125,9 +125,6 @@ out_release_res:
 	return err;
 }
 
-/* Unused, temporary to satisfy ARM arch code */
-struct pci_sys_data sys;
-
 static int versatile_pci_probe(struct platform_device *pdev)
 {
 	struct resource *res;
@@ -208,7 +205,7 @@ static int versatile_pci_probe(struct platform_device *pdev)
 	pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
 	pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
 
-	bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, &sys, &pci_res);
+	bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, NULL, &pci_res);
 	if (!bus)
 		return -ENOMEM;
 
-- 
2.5.1

[PATCH] drivers: pci: remove unused pci_sys_data structures

[Arnd Bergmann]

On Monday 09 November 2015 18:57:39 Lorenzo Pieralisi wrote:
> Commit b3a72384fe29 ("ARM/PCI: Replace pci_sys_data->align_resource
> with global function pointer") removed the struct pci_sys_data
> dependency from the ARM pcibios functions that are part of the
> common ARM PCI arch back-end (eg pcibios_align_resource()), so that
> struct pci_sys_data has now become data that is only used internally
> by the ARM bios32 layer (ie pci_common_init_dev()) and by host
> controllers drivers callbacks (eg pci_sys_data.setup) that rely on the
> ARM bios32 API to probe.
> 
> PCI host controller drivers that do not rely on ARM bios32 calls to
> probe do not need to have the pci_bus.sysdata pointer field pointing
> at a struct pci_sys_data anymore, therefore it can be removed from the
> respective drivers data structures.
> 
> This patch removes the pci_sys_data structures from the host
> controller drivers that do not rely on ARM bios32 interface to
> scan the PCI bus, completing the pci_sys_data clean-up and removing
> the related dependency on arch/arm specific data.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh@kernel.org>
> 

Nice!

Acked-by: Arnd Bergmann <arnd@arndb.de>

On the downside, I just saw how b3a72384fe29 ("ARM/PCI: Replace
pci_sys_data->align_resource with global function pointer") manages
to get rid of the last user of this, and how this is not the way
we had planned for it to be done.

It doesn't matter that much, because there is only a single
user of hw->align_resource, but this is now unportable code and
cannot easily be separated from hw_pci. It also breaks if we ever
get a machine with two different host controllers on ARM that
don't use the same pointer here.

I really would have hoped we could put that function pointer
into 'struct pci_host_bridge' instead, and eventually killed
off the architecture specific pcibios_align_resource function
as well.

Any idea why it was done with a global function pointer for
ARM rather than a proper solution?

	Arnd

[PATCH] drivers: pci: remove unused pci_sys_data structures

[Gabriele Paoloni]

Hi Arnd

[...]
> 
> Nice!
> 
> Acked-by: Arnd Bergmann <arnd@arndb.de>
> 
> On the downside, I just saw how b3a72384fe29 ("ARM/PCI: Replace
> pci_sys_data->align_resource with global function pointer") manages
> to get rid of the last user of this, and how this is not the way
> we had planned for it to be done.
> 
> It doesn't matter that much, because there is only a single
> user of hw->align_resource, but this is now unportable code and
> cannot easily be separated from hw_pci. It also breaks if we ever
> get a machine with two different host controllers on ARM that
> don't use the same pointer here.
> 
> I really would have hoped we could put that function pointer
> into 'struct pci_host_bridge' instead, and eventually killed
> off the architecture specific pcibios_align_resource function
> as well.

say we move that in pci_host_bridge struct,

Are you suggesting to modify pcibios_init_hw to assign the 
align_resource function pointer at the end after
pci_scan_root_bus_msi() has created the host bridge structure;
then we can retrieve it in pcibios_align_resource using
pci_find_host_bridge(dev->bus)....?

> 
> Any idea why it was done with a global function pointer for
> ARM rather than a proper solution?

Well just because I didn't think about the solution above...

If you think the above one can work I can send out another
patch to fix this...

Many Thanks

Gab


> 
> 	Arnd

[PATCH] drivers: pci: remove unused pci_sys_data structures

[Arnd Bergmann]

On Tuesday 10 November 2015 06:19:17 Gabriele Paoloni wrote:
> > Acked-by: Arnd Bergmann <arnd@arndb.de>
> > 
> > On the downside, I just saw how b3a72384fe29 ("ARM/PCI: Replace
> > pci_sys_data->align_resource with global function pointer") manages
> > to get rid of the last user of this, and how this is not the way
> > we had planned for it to be done.
> > 
> > It doesn't matter that much, because there is only a single
> > user of hw->align_resource, but this is now unportable code and
> > cannot easily be separated from hw_pci. It also breaks if we ever
> > get a machine with two different host controllers on ARM that
> > don't use the same pointer here.
> > 
> > I really would have hoped we could put that function pointer
> > into 'struct pci_host_bridge' instead, and eventually killed
> > off the architecture specific pcibios_align_resource function
> > as well.
> 
> say we move that in pci_host_bridge struct,
> 
> Are you suggesting to modify pcibios_init_hw to assign the 
> align_resource function pointer at the end after
> pci_scan_root_bus_msi() has created the host bridge structure;
> then we can retrieve it in pcibios_align_resource using
> pci_find_host_bridge(dev->bus)....?

What I had in mind was more along the lines of removing the
any mention of the align_resource callback from the ARM bios32.c
code and to have the pci-mvebu driver set the pointer manually
at an appropriate point (I did not look what would be that
point). What you suggest above would work too, just doesn't go
as far.

In the long run, I think it would be good to migrate all drivers
in drivers/pci/host/ away from calling pcibios_init_hw() at
all, and just use architecture-independent interfaces and leave
the hw_pci/pci_sys_data based implementation for the traditional
PCI host implementations that we have in arch/arm/mach-* for
platforms that do not use devicetree. This is not something I'm
asking you to do at this point, though any help here is appreciated.

> > Any idea why it was done with a global function pointer for
> > ARM rather than a proper solution?
> 
> Well just because I didn't think about the solution above...
> 
> If you think the above one can work I can send out another
> patch to fix this...

That would be great, yes. As I said, it's currently harmless
but it would be better to get it right.

	Arnd

[PATCH] drivers: pci: remove unused pci_sys_data structures

[Bjorn Helgaas]

On Mon, Nov 09, 2015 at 06:57:39PM +0000, Lorenzo Pieralisi wrote:
> Commit b3a72384fe29 ("ARM/PCI: Replace pci_sys_data->align_resource
> with global function pointer") removed the struct pci_sys_data
> dependency from the ARM pcibios functions that are part of the
> common ARM PCI arch back-end (eg pcibios_align_resource()), so that
> struct pci_sys_data has now become data that is only used internally
> by the ARM bios32 layer (ie pci_common_init_dev()) and by host
> controllers drivers callbacks (eg pci_sys_data.setup) that rely on the
> ARM bios32 API to probe.
> 
> PCI host controller drivers that do not rely on ARM bios32 calls to
> probe do not need to have the pci_bus.sysdata pointer field pointing
> at a struct pci_sys_data anymore, therefore it can be removed from the
> respective drivers data structures.
> 
> This patch removes the pci_sys_data structures from the host
> controller drivers that do not rely on ARM bios32 interface to
> scan the PCI bus, completing the pci_sys_data clean-up and removing
> the related dependency on arch/arm specific data.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh@kernel.org>

Applied with Arnd's ack to pci/host-generic for v4.5, thanks!

> ---
> Rob,
> 
> I could not test it on versatile, only compile tested, so
> please have an additional look.
> 
> Tested the PCI host generic on 32-bit and 64-bit guests,
> through kvmtool.
> 
> Thanks,
> Lorenzo
> 
>  drivers/pci/host/pci-host-generic.c | 9 ---------
>  drivers/pci/host/pci-versatile.c    | 5 +----
>  2 files changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 5434c90..1652bc7 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -38,16 +38,7 @@ struct gen_pci_cfg_windows {
>  	struct gen_pci_cfg_bus_ops		*ops;
>  };
>  
> -/*
> - * ARM pcibios functions expect the ARM struct pci_sys_data as the PCI
> - * sysdata.  Add pci_sys_data as the first element in struct gen_pci so
> - * that when we use a gen_pci pointer as sysdata, it is also a pointer to
> - * a struct pci_sys_data.
> - */
>  struct gen_pci {
> -#ifdef CONFIG_ARM
> -	struct pci_sys_data			sys;
> -#endif
>  	struct pci_host_bridge			host;
>  	struct gen_pci_cfg_windows		cfg;
>  	struct list_head			resources;
> diff --git a/drivers/pci/host/pci-versatile.c b/drivers/pci/host/pci-versatile.c
> index 0863d9c..f843a72 100644
> --- a/drivers/pci/host/pci-versatile.c
> +++ b/drivers/pci/host/pci-versatile.c
> @@ -125,9 +125,6 @@ out_release_res:
>  	return err;
>  }
>  
> -/* Unused, temporary to satisfy ARM arch code */
> -struct pci_sys_data sys;
> -
>  static int versatile_pci_probe(struct platform_device *pdev)
>  {
>  	struct resource *res;
> @@ -208,7 +205,7 @@ static int versatile_pci_probe(struct platform_device *pdev)
>  	pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
>  	pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
>  
> -	bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, &sys, &pci_res);
> +	bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, NULL, &pci_res);
>  	if (!bus)
>  		return -ENOMEM;
>  
> -- 
> 2.5.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html