On Tue, Jul 07, 2015 at 06:44:01AM +0100, Zhou Wang wrote:
> On 2015/7/3 1:50, Liviu Dudau wrote:
> > On Wed, Jul 01, 2015 at 10:43:33AM +0100, Zhou Wang wrote:
> >> This patch had added by Arnd Bergmann during last reviewing of v1
> >> patchset[1].
> >>
> >> PCI core codes call pcibios_align_resource(). In ARM specific one, it will
> >> dereference pci_sys_data and call sys->align_resource. If we try to unify
> >> ARM
> >> and ARM64 PCIe API in pcie-designware. it will bring kernel crash when run
> >> into
> >> this dereferencing.
> >>
> >> However, in ARM there is only pci-mvebu which implements align_resource. So
> >> add align_resource call back in pci_host_bridge structure and override
> >> pcibios_align_resource with it.
> >>
> >> Signed-off-by: Arnd Bergmann <[email protected]>
> >> Signed-off-by: Zhou Wang <[email protected]>
> >> Tested-by: Fabrice Gasnier <[email protected]>
> >>
> >> [1] http://www.spinics.net/lists/linux-pci/msg41671.html
> >> ---
> >> arch/arm/kernel/bios32.c | 6 ------
> >> drivers/pci/host/pci-mvebu.c | 47
> >> ++++++++++++++++++++++++++++----------------
> >> drivers/pci/setup-res.c | 27 ++++++++++++++++++++-----
> >> include/linux/pci.h | 3 +++
> >> 4 files changed, 55 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> >> index fcbbbb1..b01189f 100644
> >> --- a/arch/arm/kernel/bios32.c
> >> +++ b/arch/arm/kernel/bios32.c
> >> @@ -468,7 +468,6 @@ static void pcibios_init_hw(struct device *parent,
> >> struct hw_pci *hw,
> >> sys->busnr = busnr;
> >> sys->swizzle = hw->swizzle;
> >> sys->map_irq = hw->map_irq;
> >> - sys->align_resource = hw->align_resource;
> >> INIT_LIST_HEAD(&sys->resources);
> >>
> >> if (hw->private_data)
> >> @@ -588,8 +587,6 @@ char * __init pcibios_setup(char *str)
> >> resource_size_t pcibios_align_resource(void *data, const struct resource
> >> *res,
> >> resource_size_t size, resource_size_t align)
> >> {
> >> - struct pci_dev *dev = data;
> >> - struct pci_sys_data *sys = dev->sysdata;
> >> resource_size_t start = res->start;
> >>
> >> if (res->flags & IORESOURCE_IO && start & 0x300)
> >> @@ -597,9 +594,6 @@ resource_size_t pcibios_align_resource(void *data,
> >> const struct resource *res,
> >>
> >> start = (start + align - 1) & ~(align - 1);
> >>
> >> - if (sys->align_resource)
> >> - return sys->align_resource(dev, res, start, size, align);
> >> -
> >> return start;
> >> }
> >>
> >> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> >> index 1ab8635..155d05f 100644
> >> --- a/drivers/pci/host/pci-mvebu.c
> >> +++ b/drivers/pci/host/pci-mvebu.c
> >> @@ -22,6 +22,8 @@
> >> #include <linux/of_pci.h>
> >> #include <linux/of_platform.h>
> >>
> >> +#include "../pci.h" /* HACK to see pci_find_host_bridge */
> >> +
> >> /*
> >> * PCIe unit register offsets.
> >> */
> >> @@ -751,27 +753,20 @@ static int mvebu_pcie_setup(int nr, struct
> >> pci_sys_data *sys)
> >> return 1;
> >> }
> >>
> >> -static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data
> >> *sys)
> >> +static resource_size_t mvebu_pcie_align_resource(void *data,
> >> + const struct resource *res,
> >> + resource_size_t size,
> >> + resource_size_t align)
> >> {
> >> - struct mvebu_pcie *pcie = sys_to_pcie(sys);
> >> - struct pci_bus *bus;
> >> + struct pci_dev *dev = data;
> >>
> >> - bus = pci_create_root_bus(&pcie->pdev->dev, sys->busnr,
> >> - &mvebu_pcie_ops, sys, &sys->resources);
> >> - if (!bus)
> >> - return NULL;
> >> + resource_size_t start = res->start;
> >>
> >> - pci_scan_child_bus(bus);
> >> + if (res->flags & IORESOURCE_IO && start & 0x300)
> >> + start = (start + 0x3ff) & ~0x3ff;
> >>
> >> - return bus;
> >> -}
> >> + start = (start + align - 1) & ~(align - 1);
> >
> > Honestly, I don't see here anything that is mvebu specific. Could you move
>
> What I mean is that there is only mvebu who implemented sys->align_resource
> callback in ARM
> arch.
>
> > this function in the generic pci/host area and have a flag in the
> > pci_host_bridge
> > structure whether the function should be called or not? I know that in a way
> > that looks very close to the existing implementation which uses
> > pcibios_align_resource,
>
> I am confused about "the existing implementation". Now pcibios_align_resource
> are
> implemented by each arch code and are called directly. Did I miss anything
> about
> pcibios_align_resource?
Sorry, I meant existing implementation_s_ and yes, I was reffering to the arch
specific
code. If you look at the cris, frv/mb93090-mb00, m68k, microblaze, powerpc,
unicore32
and x86 they all share the code with your version, plus or minus additional
checks.
I was thinking that for those arch versions that match your version you can
create just
one version and use it as the alignf function.
Best regards,
Liviu
>
> Best rgards,
> Zhou
>
> > but the problem with pcibios_ version is that it is arch specific and not
> > driver
> > specific the way we want.
> >
> > Having this version as a generic implementation would also remove at least
> > 2 more
> > arch version, possibly more after testing by the arch maintainers.
> >
> > Best regards,
> > Liviu
> >
> >>
> >> -static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev,
> >> - const struct resource *res,
> >> - resource_size_t start,
> >> - resource_size_t size,
> >> - resource_size_t align)
> >> -{
> >> if (dev->bus->number != 0)
> >> return start;
> >>
> >> @@ -796,6 +791,25 @@ static resource_size_t
> >> mvebu_pcie_align_resource(struct pci_dev *dev,
> >> return start;
> >> }
> >>
> >> +static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data
> >> *sys)
> >> +{
> >> + struct mvebu_pcie *pcie = sys_to_pcie(sys);
> >> + struct pci_host_bridge *phb;
> >> + struct pci_bus *bus;
> >> +
> >> + bus = pci_create_root_bus(&pcie->pdev->dev, sys->busnr,
> >> + &mvebu_pcie_ops, sys, &sys->resources);
> >> + if (!bus)
> >> + return NULL;
> >> +
> >> + phb = pci_find_host_bridge(bus);
> >> + phb->align_resource = mvebu_pcie_align_resource;
> >> +
> >> + pci_scan_child_bus(bus);
> >> +
> >> + return bus;
> >> +}
> >> +
> >> static void mvebu_pcie_enable(struct mvebu_pcie *pcie)
> >> {
> >> struct hw_pci hw;
> >> @@ -812,7 +826,6 @@ static void mvebu_pcie_enable(struct mvebu_pcie *pcie)
> >> hw.scan = mvebu_pcie_scan_bus;
> >> hw.map_irq = of_irq_parse_and_map_pci;
> >> hw.ops = &mvebu_pcie_ops;
> >> - hw.align_resource = mvebu_pcie_align_resource;
> >>
> >> pci_common_init(&hw);
> >> }
> >> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> >> index 232f925..73abca7 100644
> >> --- a/drivers/pci/setup-res.c
> >> +++ b/drivers/pci/setup-res.c
> >> @@ -200,7 +200,11 @@ static int pci_revert_fw_address(struct resource
> >> *res, struct pci_dev *dev,
> >> }
> >>
> >> static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev,
> >> - int resno, resource_size_t size, resource_size_t align)
> >> + int resno, resource_size_t size, resource_size_t align,
> >> + resource_size_t (*alignf)(void *,
> >> + const struct resource *,
> >> + resource_size_t,
> >> + resource_size_t))
> >> {
> >> struct resource *res = dev->resource + resno;
> >> resource_size_t min;
> >> @@ -217,7 +221,7 @@ static int __pci_assign_resource(struct pci_bus *bus,
> >> struct pci_dev *dev,
> >> */
> >> ret = pci_bus_alloc_resource(bus, res, size, align, min,
> >> IORESOURCE_PREFETCH | IORESOURCE_MEM_64,
> >> - pcibios_align_resource, dev);
> >> + alignf, dev);
> >> if (ret == 0)
> >> return 0;
> >>
> >> @@ -229,7 +233,7 @@ static int __pci_assign_resource(struct pci_bus *bus,
> >> struct pci_dev *dev,
> >> (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) {
> >> ret = pci_bus_alloc_resource(bus, res, size, align, min,
> >> IORESOURCE_PREFETCH,
> >> - pcibios_align_resource, dev);
> >> + alignf, dev);
> >> if (ret == 0)
> >> return 0;
> >> }
> >> @@ -242,7 +246,7 @@ static int __pci_assign_resource(struct pci_bus *bus,
> >> struct pci_dev *dev,
> >> */
> >> if (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64))
> >> ret = pci_bus_alloc_resource(bus, res, size, align, min, 0,
> >> - pcibios_align_resource, dev);
> >> + alignf, dev);
> >>
> >> return ret;
> >> }
> >> @@ -251,10 +255,23 @@ static int _pci_assign_resource(struct pci_dev *dev,
> >> int resno,
> >> resource_size_t size, resource_size_t min_align)
> >> {
> >> struct pci_bus *bus;
> >> + struct pci_host_bridge *phb;
> >> + resource_size_t (*alignf)(void *,
> >> + const struct resource *,
> >> + resource_size_t,
> >> + resource_size_t);
> >> int ret;
> >>
> >> bus = dev->bus;
> >> - while ((ret = __pci_assign_resource(bus, dev, resno, size, min_align)))
> >> {
> >> + phb = pci_find_host_bridge(bus);
> >> +
> >> + if (phb->align_resource)
> >> + alignf = phb->align_resource;
> >> + else
> >> + alignf = pcibios_align_resource;
> >> +
> >> + while ((ret = __pci_assign_resource(bus, dev, resno, size,
> >> + min_align, alignf))) {
> >> if (!bus->parent || !bus->self->transparent)
> >> break;
> >> bus = bus->parent;
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index 353db8d..39e48fc 100644
> >> --- a/include/linux/pci.h
> >> +++ b/include/linux/pci.h
> >> @@ -404,6 +404,9 @@ struct pci_host_bridge {
> >> struct device dev;
> >> struct pci_bus *bus; /* root bus */
> >> struct list_head windows; /* resource_entry */
> >> + resource_size_t (*align_resource)(void *data,
> >> + const struct resource *res,
> >> + resource_size_t size, resource_size_t align);
> >> void (*release_fn)(struct pci_host_bridge *);
> >> void *release_data;
> >> unsigned int ignore_reset_delay:1; /* for entire hierarchy */
> >> --
> >> 1.9.1
> >>
> >
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
