* [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
@ 2015-02-17 23:14 ` Feng Kan
0 siblings, 0 replies; 20+ messages in thread
From: Feng Kan @ 2015-02-17 23:14 UTC (permalink / raw)
To: linux-arm-kernel
The generic accessor functions for pci-xgene uses map_bus
call that returns the base address but did not add the additional
offset.
Signed-off-by: Feng Kan <fkan@apm.com>
---
drivers/pci/host/pci-xgene.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
index aab5547..ee082c0 100644
--- a/drivers/pci/host/pci-xgene.c
+++ b/drivers/pci/host/pci-xgene.c
@@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
return false;
}
-static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
+static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
int offset)
{
struct xgene_pcie_port *port = bus->sysdata;
@@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
return NULL;
xgene_pcie_set_rtdid_reg(bus, devfn);
- return xgene_pcie_get_cfg_base(bus);
+ return xgene_pcie_get_cfg_base(bus) + offset;
}
static struct pci_ops xgene_pcie_ops = {
--
1.9.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
2015-02-17 23:14 ` Feng Kan
@ 2015-02-19 22:53 ` Tanmay Inamdar
-1 siblings, 0 replies; 20+ messages in thread
From: Tanmay Inamdar @ 2015-02-19 22:53 UTC (permalink / raw)
To: Feng Kan
Cc: patches, linux-pci@vger.kernel.org, linux-kernel, robh,
linux-arm-kernel
On Tue, Feb 17, 2015 at 3:14 PM, Feng Kan <fkan@apm.com> wrote:
>
> The generic accessor functions for pci-xgene uses map_bus
> call that returns the base address but did not add the additional
> offset.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
> drivers/pci/host/pci-xgene.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index aab5547..ee082c0 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
> return false;
> }
>
> -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> int offset)
> {
> struct xgene_pcie_port *port = bus->sysdata;
> @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> return NULL;
>
> xgene_pcie_set_rtdid_reg(bus, devfn);
> - return xgene_pcie_get_cfg_base(bus);
> + return xgene_pcie_get_cfg_base(bus) + offset;
> }
>
> static struct pci_ops xgene_pcie_ops = {
Acked-by: Tanmay Inamdar <tinamdar@apm.com>
>
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
@ 2015-02-19 22:53 ` Tanmay Inamdar
0 siblings, 0 replies; 20+ messages in thread
From: Tanmay Inamdar @ 2015-02-19 22:53 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Feb 17, 2015 at 3:14 PM, Feng Kan <fkan@apm.com> wrote:
>
> The generic accessor functions for pci-xgene uses map_bus
> call that returns the base address but did not add the additional
> offset.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
> drivers/pci/host/pci-xgene.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index aab5547..ee082c0 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
> return false;
> }
>
> -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> int offset)
> {
> struct xgene_pcie_port *port = bus->sysdata;
> @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> return NULL;
>
> xgene_pcie_set_rtdid_reg(bus, devfn);
> - return xgene_pcie_get_cfg_base(bus);
> + return xgene_pcie_get_cfg_base(bus) + offset;
> }
>
> static struct pci_ops xgene_pcie_ops = {
Acked-by: Tanmay Inamdar <tinamdar@apm.com>
>
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
2015-02-17 23:14 ` Feng Kan
@ 2015-02-20 22:09 ` Rob Herring
-1 siblings, 0 replies; 20+ messages in thread
From: Rob Herring @ 2015-02-20 22:09 UTC (permalink / raw)
To: Feng Kan
Cc: patches@apm.com, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Tanmay Inamdar
On Tue, Feb 17, 2015 at 5:14 PM, Feng Kan <fkan@apm.com> wrote:
> The generic accessor functions for pci-xgene uses map_bus
> call that returns the base address but did not add the additional
> offset.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
> drivers/pci/host/pci-xgene.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Thanks.
Acked-by: Rob Herring <robh@kernel.org>
>
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index aab5547..ee082c0 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
> return false;
> }
>
> -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> int offset)
> {
> struct xgene_pcie_port *port = bus->sysdata;
> @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> return NULL;
>
> xgene_pcie_set_rtdid_reg(bus, devfn);
> - return xgene_pcie_get_cfg_base(bus);
> + return xgene_pcie_get_cfg_base(bus) + offset;
> }
>
> static struct pci_ops xgene_pcie_ops = {
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
@ 2015-02-20 22:09 ` Rob Herring
0 siblings, 0 replies; 20+ messages in thread
From: Rob Herring @ 2015-02-20 22:09 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Feb 17, 2015 at 5:14 PM, Feng Kan <fkan@apm.com> wrote:
> The generic accessor functions for pci-xgene uses map_bus
> call that returns the base address but did not add the additional
> offset.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
> drivers/pci/host/pci-xgene.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Thanks.
Acked-by: Rob Herring <robh@kernel.org>
>
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index aab5547..ee082c0 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
> return false;
> }
>
> -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> int offset)
> {
> struct xgene_pcie_port *port = bus->sysdata;
> @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> return NULL;
>
> xgene_pcie_set_rtdid_reg(bus, devfn);
> - return xgene_pcie_get_cfg_base(bus);
> + return xgene_pcie_get_cfg_base(bus) + offset;
> }
>
> static struct pci_ops xgene_pcie_ops = {
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
2015-02-17 23:14 ` Feng Kan
@ 2015-02-27 0:21 ` Bjorn Helgaas
-1 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2015-02-27 0:21 UTC (permalink / raw)
To: Feng Kan; +Cc: patches, linux-pci, linux-kernel, robh, linux-arm-kernel,
tinamdar
On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
> The generic accessor functions for pci-xgene uses map_bus
> call that returns the base address but did not add the additional
> offset.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
> drivers/pci/host/pci-xgene.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index aab5547..ee082c0 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
> return false;
> }
>
> -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> int offset)
> {
> struct xgene_pcie_port *port = bus->sysdata;
> @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> return NULL;
>
> xgene_pcie_set_rtdid_reg(bus, devfn);
> - return xgene_pcie_get_cfg_base(bus);
> + return xgene_pcie_get_cfg_base(bus) + offset;
Where's the locking here? ECAM doesn't need locking because the
bus/dev/fn/offset is all encoded in the MMIO address. But it looks
like X-Gene doesn't work that way and bus/dev/fn is in the RTDID register.
So it seems like X-Gene needs locking that not everybody needs. Are you
relying on higher-level locking somewhere?
> }
>
> static struct pci_ops xgene_pcie_ops = {
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
@ 2015-02-27 0:21 ` Bjorn Helgaas
0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2015-02-27 0:21 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
> The generic accessor functions for pci-xgene uses map_bus
> call that returns the base address but did not add the additional
> offset.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
> drivers/pci/host/pci-xgene.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index aab5547..ee082c0 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
> return false;
> }
>
> -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> int offset)
> {
> struct xgene_pcie_port *port = bus->sysdata;
> @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> return NULL;
>
> xgene_pcie_set_rtdid_reg(bus, devfn);
> - return xgene_pcie_get_cfg_base(bus);
> + return xgene_pcie_get_cfg_base(bus) + offset;
Where's the locking here? ECAM doesn't need locking because the
bus/dev/fn/offset is all encoded in the MMIO address. But it looks
like X-Gene doesn't work that way and bus/dev/fn is in the RTDID register.
So it seems like X-Gene needs locking that not everybody needs. Are you
relying on higher-level locking somewhere?
> }
>
> static struct pci_ops xgene_pcie_ops = {
> --
> 1.9.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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
2015-02-27 0:21 ` Bjorn Helgaas
@ 2015-03-05 16:38 ` Bjorn Helgaas
-1 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2015-03-05 16:38 UTC (permalink / raw)
To: Feng Kan
Cc: patches, linux-pci, linux-kernel, robh, linux-arm-kernel,
tinamdar, Mark Salter
[+cc Mark]
On Thu, Feb 26, 2015 at 06:21:51PM -0600, Bjorn Helgaas wrote:
> On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
> > The generic accessor functions for pci-xgene uses map_bus
> > call that returns the base address but did not add the additional
> > offset.
> >
> > Signed-off-by: Feng Kan <fkan@apm.com>
> > ---
> > drivers/pci/host/pci-xgene.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> > index aab5547..ee082c0 100644
> > --- a/drivers/pci/host/pci-xgene.c
> > +++ b/drivers/pci/host/pci-xgene.c
> > @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
> > return false;
> > }
> >
> > -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> > +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> > int offset)
> > {
> > struct xgene_pcie_port *port = bus->sysdata;
> > @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> > return NULL;
> >
> > xgene_pcie_set_rtdid_reg(bus, devfn);
> > - return xgene_pcie_get_cfg_base(bus);
> > + return xgene_pcie_get_cfg_base(bus) + offset;
>
> Where's the locking here? ECAM doesn't need locking because the
> bus/dev/fn/offset is all encoded in the MMIO address. But it looks
> like X-Gene doesn't work that way and bus/dev/fn is in the RTDID register.
>
> So it seems like X-Gene needs locking that not everybody needs. Are you
> relying on higher-level locking somewhere?
Ping, what's going on here? I've gotten at least three patches for this
offset issue, so we need to get it resolved.
If there's no locking problem, I can just apply this and we'll be finished.
Actually, I think Mark's patch is better, because it correctly returns NULL
(failure) if xgene_pcie_get_cfg_base() fails. So please review and ack
that one or explain why this one is better.
But if there *is* a locking problem, we should fix that, too. That should
be a separate patch, so I guess I can apply the one to fix the offset
problem first, and we'll at least be no worse off with respect to locking
than we are today.
Please help me out.
Bjorn
> > }
> >
> > static struct pci_ops xgene_pcie_ops = {
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
@ 2015-03-05 16:38 ` Bjorn Helgaas
0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2015-03-05 16:38 UTC (permalink / raw)
To: linux-arm-kernel
[+cc Mark]
On Thu, Feb 26, 2015 at 06:21:51PM -0600, Bjorn Helgaas wrote:
> On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
> > The generic accessor functions for pci-xgene uses map_bus
> > call that returns the base address but did not add the additional
> > offset.
> >
> > Signed-off-by: Feng Kan <fkan@apm.com>
> > ---
> > drivers/pci/host/pci-xgene.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> > index aab5547..ee082c0 100644
> > --- a/drivers/pci/host/pci-xgene.c
> > +++ b/drivers/pci/host/pci-xgene.c
> > @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
> > return false;
> > }
> >
> > -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> > +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> > int offset)
> > {
> > struct xgene_pcie_port *port = bus->sysdata;
> > @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> > return NULL;
> >
> > xgene_pcie_set_rtdid_reg(bus, devfn);
> > - return xgene_pcie_get_cfg_base(bus);
> > + return xgene_pcie_get_cfg_base(bus) + offset;
>
> Where's the locking here? ECAM doesn't need locking because the
> bus/dev/fn/offset is all encoded in the MMIO address. But it looks
> like X-Gene doesn't work that way and bus/dev/fn is in the RTDID register.
>
> So it seems like X-Gene needs locking that not everybody needs. Are you
> relying on higher-level locking somewhere?
Ping, what's going on here? I've gotten at least three patches for this
offset issue, so we need to get it resolved.
If there's no locking problem, I can just apply this and we'll be finished.
Actually, I think Mark's patch is better, because it correctly returns NULL
(failure) if xgene_pcie_get_cfg_base() fails. So please review and ack
that one or explain why this one is better.
But if there *is* a locking problem, we should fix that, too. That should
be a separate patch, so I guess I can apply the one to fix the offset
problem first, and we'll at least be no worse off with respect to locking
than we are today.
Please help me out.
Bjorn
> > }
> >
> > static struct pci_ops xgene_pcie_ops = {
> > --
> > 1.9.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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
2015-03-05 16:38 ` Bjorn Helgaas
@ 2015-03-05 16:53 ` Feng Kan
-1 siblings, 0 replies; 20+ messages in thread
From: Feng Kan @ 2015-03-05 16:53 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: patches, linux-pci, linux-kernel@vger.kernel.org, Rob Herring,
linux-arm-kernel@lists.infradead.org, Tanmay Inamdar, Mark Salter
Please take Mark's patch if you think it is better.
On Thu, Mar 5, 2015 at 8:38 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Mark]
>
> On Thu, Feb 26, 2015 at 06:21:51PM -0600, Bjorn Helgaas wrote:
>> On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
>> > The generic accessor functions for pci-xgene uses map_bus
>> > call that returns the base address but did not add the additional
>> > offset.
>> >
>> > Signed-off-by: Feng Kan <fkan@apm.com>
>> > ---
>> > drivers/pci/host/pci-xgene.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
>> > index aab5547..ee082c0 100644
>> > --- a/drivers/pci/host/pci-xgene.c
>> > +++ b/drivers/pci/host/pci-xgene.c
>> > @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
>> > return false;
>> > }
>> >
>> > -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>> > +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>> > int offset)
>> > {
>> > struct xgene_pcie_port *port = bus->sysdata;
>> > @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>> > return NULL;
>> >
>> > xgene_pcie_set_rtdid_reg(bus, devfn);
>> > - return xgene_pcie_get_cfg_base(bus);
>> > + return xgene_pcie_get_cfg_base(bus) + offset;
>>
>> Where's the locking here? ECAM doesn't need locking because the
>> bus/dev/fn/offset is all encoded in the MMIO address. But it looks
>> like X-Gene doesn't work that way and bus/dev/fn is in the RTDID register.
>>
>> So it seems like X-Gene needs locking that not everybody needs. Are you
>> relying on higher-level locking somewhere?
>
> Ping, what's going on here? I've gotten at least three patches for this
> offset issue, so we need to get it resolved.
>
> If there's no locking problem, I can just apply this and we'll be finished.
> Actually, I think Mark's patch is better, because it correctly returns NULL
> (failure) if xgene_pcie_get_cfg_base() fails. So please review and ack
> that one or explain why this one is better.
>
> But if there *is* a locking problem, we should fix that, too. That should
> be a separate patch, so I guess I can apply the one to fix the offset
> problem first, and we'll at least be no worse off with respect to locking
> than we are today.
>
> Please help me out.
>
> Bjorn
>
>> > }
>> >
>> > static struct pci_ops xgene_pcie_ops = {
>> > --
>> > 1.9.1
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
@ 2015-03-05 16:53 ` Feng Kan
0 siblings, 0 replies; 20+ messages in thread
From: Feng Kan @ 2015-03-05 16:53 UTC (permalink / raw)
To: linux-arm-kernel
Please take Mark's patch if you think it is better.
On Thu, Mar 5, 2015 at 8:38 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Mark]
>
> On Thu, Feb 26, 2015 at 06:21:51PM -0600, Bjorn Helgaas wrote:
>> On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
>> > The generic accessor functions for pci-xgene uses map_bus
>> > call that returns the base address but did not add the additional
>> > offset.
>> >
>> > Signed-off-by: Feng Kan <fkan@apm.com>
>> > ---
>> > drivers/pci/host/pci-xgene.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
>> > index aab5547..ee082c0 100644
>> > --- a/drivers/pci/host/pci-xgene.c
>> > +++ b/drivers/pci/host/pci-xgene.c
>> > @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
>> > return false;
>> > }
>> >
>> > -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>> > +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>> > int offset)
>> > {
>> > struct xgene_pcie_port *port = bus->sysdata;
>> > @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>> > return NULL;
>> >
>> > xgene_pcie_set_rtdid_reg(bus, devfn);
>> > - return xgene_pcie_get_cfg_base(bus);
>> > + return xgene_pcie_get_cfg_base(bus) + offset;
>>
>> Where's the locking here? ECAM doesn't need locking because the
>> bus/dev/fn/offset is all encoded in the MMIO address. But it looks
>> like X-Gene doesn't work that way and bus/dev/fn is in the RTDID register.
>>
>> So it seems like X-Gene needs locking that not everybody needs. Are you
>> relying on higher-level locking somewhere?
>
> Ping, what's going on here? I've gotten at least three patches for this
> offset issue, so we need to get it resolved.
>
> If there's no locking problem, I can just apply this and we'll be finished.
> Actually, I think Mark's patch is better, because it correctly returns NULL
> (failure) if xgene_pcie_get_cfg_base() fails. So please review and ack
> that one or explain why this one is better.
>
> But if there *is* a locking problem, we should fix that, too. That should
> be a separate patch, so I guess I can apply the one to fix the offset
> problem first, and we'll at least be no worse off with respect to locking
> than we are today.
>
> Please help me out.
>
> Bjorn
>
>> > }
>> >
>> > static struct pci_ops xgene_pcie_ops = {
>> > --
>> > 1.9.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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
2015-03-05 16:53 ` Feng Kan
@ 2015-03-06 4:12 ` Bjorn Helgaas
-1 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2015-03-06 4:12 UTC (permalink / raw)
To: Feng Kan
Cc: patches, linux-pci, linux-kernel@vger.kernel.org, Rob Herring,
linux-arm-kernel@lists.infradead.org, Tanmay Inamdar, Mark Salter
On Thu, Mar 05, 2015 at 08:53:38AM -0800, Feng Kan wrote:
> Please take Mark's patch if you think it is better.
>
>
>
> On Thu, Mar 5, 2015 at 8:38 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > [+cc Mark]
> >
> > On Thu, Feb 26, 2015 at 06:21:51PM -0600, Bjorn Helgaas wrote:
> >> On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
> >> > The generic accessor functions for pci-xgene uses map_bus
> >> > call that returns the base address but did not add the additional
> >> > offset.
> >> >
> >> > Signed-off-by: Feng Kan <fkan@apm.com>
> >> > ---
> >> > drivers/pci/host/pci-xgene.c | 4 ++--
> >> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> >> > index aab5547..ee082c0 100644
> >> > --- a/drivers/pci/host/pci-xgene.c
> >> > +++ b/drivers/pci/host/pci-xgene.c
> >> > @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
> >> > return false;
> >> > }
> >> >
> >> > -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> >> > +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> >> > int offset)
> >> > {
> >> > struct xgene_pcie_port *port = bus->sysdata;
> >> > @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> >> > return NULL;
> >> >
> >> > xgene_pcie_set_rtdid_reg(bus, devfn);
> >> > - return xgene_pcie_get_cfg_base(bus);
> >> > + return xgene_pcie_get_cfg_base(bus) + offset;
> >>
> >> Where's the locking here? ECAM doesn't need locking because the
> >> bus/dev/fn/offset is all encoded in the MMIO address. But it looks
> >> like X-Gene doesn't work that way and bus/dev/fn is in the RTDID register.
> >>
> >> So it seems like X-Gene needs locking that not everybody needs. Are you
> >> relying on higher-level locking somewhere?
> >
> > Ping, what's going on here? I've gotten at least three patches for this
> > offset issue, so we need to get it resolved.
> >
> > If there's no locking problem, I can just apply this and we'll be finished.
> > Actually, I think Mark's patch is better, because it correctly returns NULL
> > (failure) if xgene_pcie_get_cfg_base() fails. So please review and ack
> > that one or explain why this one is better.
Huh, I could swear I saw a failure path in xgene_pcie_get_cfg_base(). But
I don't see a way it can fail, so I don't think it matters which way we fix
this.
Bjorn
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
@ 2015-03-06 4:12 ` Bjorn Helgaas
0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2015-03-06 4:12 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Mar 05, 2015 at 08:53:38AM -0800, Feng Kan wrote:
> Please take Mark's patch if you think it is better.
>
>
>
> On Thu, Mar 5, 2015 at 8:38 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > [+cc Mark]
> >
> > On Thu, Feb 26, 2015 at 06:21:51PM -0600, Bjorn Helgaas wrote:
> >> On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
> >> > The generic accessor functions for pci-xgene uses map_bus
> >> > call that returns the base address but did not add the additional
> >> > offset.
> >> >
> >> > Signed-off-by: Feng Kan <fkan@apm.com>
> >> > ---
> >> > drivers/pci/host/pci-xgene.c | 4 ++--
> >> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> >> > index aab5547..ee082c0 100644
> >> > --- a/drivers/pci/host/pci-xgene.c
> >> > +++ b/drivers/pci/host/pci-xgene.c
> >> > @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
> >> > return false;
> >> > }
> >> >
> >> > -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> >> > +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> >> > int offset)
> >> > {
> >> > struct xgene_pcie_port *port = bus->sysdata;
> >> > @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> >> > return NULL;
> >> >
> >> > xgene_pcie_set_rtdid_reg(bus, devfn);
> >> > - return xgene_pcie_get_cfg_base(bus);
> >> > + return xgene_pcie_get_cfg_base(bus) + offset;
> >>
> >> Where's the locking here? ECAM doesn't need locking because the
> >> bus/dev/fn/offset is all encoded in the MMIO address. But it looks
> >> like X-Gene doesn't work that way and bus/dev/fn is in the RTDID register.
> >>
> >> So it seems like X-Gene needs locking that not everybody needs. Are you
> >> relying on higher-level locking somewhere?
> >
> > Ping, what's going on here? I've gotten at least three patches for this
> > offset issue, so we need to get it resolved.
> >
> > If there's no locking problem, I can just apply this and we'll be finished.
> > Actually, I think Mark's patch is better, because it correctly returns NULL
> > (failure) if xgene_pcie_get_cfg_base() fails. So please review and ack
> > that one or explain why this one is better.
Huh, I could swear I saw a failure path in xgene_pcie_get_cfg_base(). But
I don't see a way it can fail, so I don't think it matters which way we fix
this.
Bjorn
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
2015-03-05 16:38 ` Bjorn Helgaas
@ 2015-03-05 20:57 ` Rob Herring
-1 siblings, 0 replies; 20+ messages in thread
From: Rob Herring @ 2015-03-05 20:57 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Feng Kan, patches@apm.com, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Tanmay Inamdar, Mark Salter
On Thu, Mar 5, 2015 at 10:38 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Mark]
>
> On Thu, Feb 26, 2015 at 06:21:51PM -0600, Bjorn Helgaas wrote:
>> On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
>> > The generic accessor functions for pci-xgene uses map_bus
>> > call that returns the base address but did not add the additional
>> > offset.
>> >
>> > Signed-off-by: Feng Kan <fkan@apm.com>
>> > ---
>> > drivers/pci/host/pci-xgene.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
>> > index aab5547..ee082c0 100644
>> > --- a/drivers/pci/host/pci-xgene.c
>> > +++ b/drivers/pci/host/pci-xgene.c
>> > @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
>> > return false;
>> > }
>> >
>> > -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>> > +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>> > int offset)
>> > {
>> > struct xgene_pcie_port *port = bus->sysdata;
>> > @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>> > return NULL;
>> >
>> > xgene_pcie_set_rtdid_reg(bus, devfn);
>> > - return xgene_pcie_get_cfg_base(bus);
>> > + return xgene_pcie_get_cfg_base(bus) + offset;
>>
>> Where's the locking here? ECAM doesn't need locking because the
>> bus/dev/fn/offset is all encoded in the MMIO address. But it looks
>> like X-Gene doesn't work that way and bus/dev/fn is in the RTDID register.
>>
>> So it seems like X-Gene needs locking that not everybody needs. Are you
>> relying on higher-level locking somewhere?
>
> Ping, what's going on here? I've gotten at least three patches for this
> offset issue, so we need to get it resolved.
>
> If there's no locking problem, I can just apply this and we'll be finished.
> Actually, I think Mark's patch is better, because it correctly returns NULL
> (failure) if xgene_pcie_get_cfg_base() fails. So please review and ack
> that one or explain why this one is better.
>
> But if there *is* a locking problem, we should fix that, too. That should
> be a separate patch, so I guess I can apply the one to fix the offset
> problem first, and we'll at least be no worse off with respect to locking
> than we are today.
There's no locking problem. The config accesses are all within the
pci_lock spinlock and nothing else touches that register.
Rob
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
@ 2015-03-05 20:57 ` Rob Herring
0 siblings, 0 replies; 20+ messages in thread
From: Rob Herring @ 2015-03-05 20:57 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Mar 5, 2015 at 10:38 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> [+cc Mark]
>
> On Thu, Feb 26, 2015 at 06:21:51PM -0600, Bjorn Helgaas wrote:
>> On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
>> > The generic accessor functions for pci-xgene uses map_bus
>> > call that returns the base address but did not add the additional
>> > offset.
>> >
>> > Signed-off-by: Feng Kan <fkan@apm.com>
>> > ---
>> > drivers/pci/host/pci-xgene.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
>> > index aab5547..ee082c0 100644
>> > --- a/drivers/pci/host/pci-xgene.c
>> > +++ b/drivers/pci/host/pci-xgene.c
>> > @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
>> > return false;
>> > }
>> >
>> > -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>> > +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>> > int offset)
>> > {
>> > struct xgene_pcie_port *port = bus->sysdata;
>> > @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
>> > return NULL;
>> >
>> > xgene_pcie_set_rtdid_reg(bus, devfn);
>> > - return xgene_pcie_get_cfg_base(bus);
>> > + return xgene_pcie_get_cfg_base(bus) + offset;
>>
>> Where's the locking here? ECAM doesn't need locking because the
>> bus/dev/fn/offset is all encoded in the MMIO address. But it looks
>> like X-Gene doesn't work that way and bus/dev/fn is in the RTDID register.
>>
>> So it seems like X-Gene needs locking that not everybody needs. Are you
>> relying on higher-level locking somewhere?
>
> Ping, what's going on here? I've gotten at least three patches for this
> offset issue, so we need to get it resolved.
>
> If there's no locking problem, I can just apply this and we'll be finished.
> Actually, I think Mark's patch is better, because it correctly returns NULL
> (failure) if xgene_pcie_get_cfg_base() fails. So please review and ack
> that one or explain why this one is better.
>
> But if there *is* a locking problem, we should fix that, too. That should
> be a separate patch, so I guess I can apply the one to fix the offset
> problem first, and we'll at least be no worse off with respect to locking
> than we are today.
There's no locking problem. The config accesses are all within the
pci_lock spinlock and nothing else touches that register.
Rob
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
2015-03-05 20:57 ` Rob Herring
@ 2015-03-06 4:53 ` Bjorn Helgaas
-1 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2015-03-06 4:53 UTC (permalink / raw)
To: Rob Herring
Cc: Feng Kan, patches@apm.com, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Tanmay Inamdar, Mark Salter
On Thu, Mar 05, 2015 at 02:57:55PM -0600, Rob Herring wrote:
> On Thu, Mar 5, 2015 at 10:38 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > [+cc Mark]
> >
> > On Thu, Feb 26, 2015 at 06:21:51PM -0600, Bjorn Helgaas wrote:
> >> On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
> >> > The generic accessor functions for pci-xgene uses map_bus
> >> > call that returns the base address but did not add the additional
> >> > offset.
> >> >
> >> > Signed-off-by: Feng Kan <fkan@apm.com>
> >> > ...
> >> > @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> >> > return NULL;
> >> >
> >> > xgene_pcie_set_rtdid_reg(bus, devfn);
> >> > - return xgene_pcie_get_cfg_base(bus);
> >> > + return xgene_pcie_get_cfg_base(bus) + offset;
> >>
> >> Where's the locking here? ECAM doesn't need locking because the
> >> bus/dev/fn/offset is all encoded in the MMIO address. But it looks
> >> like X-Gene doesn't work that way and bus/dev/fn is in the RTDID register.
> >>
> >> So it seems like X-Gene needs locking that not everybody needs. Are you
> >> relying on higher-level locking somewhere?
> >> ...
>
> There's no locking problem. The config accesses are all within the
> pci_lock spinlock and nothing else touches that register.
Mmmmm. Yes, you're right. pci_bus_{read,write}_config_{byte,word,dword}()
all acquire pci_lock. For anybody following along at home, here's the
path I was concerned about:
pci_read_config_byte
pci_bus_read_config_byte
lock(&pci_lock) # acquire pci_lock
bus->ops->read/write # struct pci_ops
pci_generic_config_read # gen_pci_ops
bus->ops->map_bus
xgene_pcie_map_bus # xgene_pcie_ops
xgene_pcie_set_rtdid_reg
writel # requires mutex
readb # config read
I'm not exactly sure *why* we do locking there, other than we're just
too scared to change it. As far as I know, methods like ECAM shouldn't
require that lock, so it's sort of a shame to do it at the top level
like that.
Some of the low-level routines, like pci_{conf1,conf2,bios}, also use a
lock (pci_config_lock in these cases). We do need it there because a
few paths do call the low-level routines directly.
Here's a typical path on x86:
pci_read_config_byte
pci_bus_read_config_byte
lock(&pci_lock) # acquire pci_lock
bus->ops->read/write # struct pci_ops
pci_read # x86 pci_root_ops
raw_pci_read
raw_pci_ops->read
pci_conf1_read # x86 raw_pci_ops
lock(&pci_config_lock) # acquire pci_config_lock
And here's a path on x86 that uses the low-level routines directly and
requires the locking there:
acpi_os_read_pci_configuration
raw_pci_read
raw_pci_ops->read
pci_conf1_read
lock(&pci_config_lock)
So ideally I think the locking would be done in the low-level routines
that need it, and we could do without pci_lock. But I don't know
whether that's practical at this point or not.
Bjorn
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
@ 2015-03-06 4:53 ` Bjorn Helgaas
0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2015-03-06 4:53 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Mar 05, 2015 at 02:57:55PM -0600, Rob Herring wrote:
> On Thu, Mar 5, 2015 at 10:38 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > [+cc Mark]
> >
> > On Thu, Feb 26, 2015 at 06:21:51PM -0600, Bjorn Helgaas wrote:
> >> On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
> >> > The generic accessor functions for pci-xgene uses map_bus
> >> > call that returns the base address but did not add the additional
> >> > offset.
> >> >
> >> > Signed-off-by: Feng Kan <fkan@apm.com>
> >> > ...
> >> > @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> >> > return NULL;
> >> >
> >> > xgene_pcie_set_rtdid_reg(bus, devfn);
> >> > - return xgene_pcie_get_cfg_base(bus);
> >> > + return xgene_pcie_get_cfg_base(bus) + offset;
> >>
> >> Where's the locking here? ECAM doesn't need locking because the
> >> bus/dev/fn/offset is all encoded in the MMIO address. But it looks
> >> like X-Gene doesn't work that way and bus/dev/fn is in the RTDID register.
> >>
> >> So it seems like X-Gene needs locking that not everybody needs. Are you
> >> relying on higher-level locking somewhere?
> >> ...
>
> There's no locking problem. The config accesses are all within the
> pci_lock spinlock and nothing else touches that register.
Mmmmm. Yes, you're right. pci_bus_{read,write}_config_{byte,word,dword}()
all acquire pci_lock. For anybody following along at home, here's the
path I was concerned about:
pci_read_config_byte
pci_bus_read_config_byte
lock(&pci_lock) # acquire pci_lock
bus->ops->read/write # struct pci_ops
pci_generic_config_read # gen_pci_ops
bus->ops->map_bus
xgene_pcie_map_bus # xgene_pcie_ops
xgene_pcie_set_rtdid_reg
writel # requires mutex
readb # config read
I'm not exactly sure *why* we do locking there, other than we're just
too scared to change it. As far as I know, methods like ECAM shouldn't
require that lock, so it's sort of a shame to do it at the top level
like that.
Some of the low-level routines, like pci_{conf1,conf2,bios}, also use a
lock (pci_config_lock in these cases). We do need it there because a
few paths do call the low-level routines directly.
Here's a typical path on x86:
pci_read_config_byte
pci_bus_read_config_byte
lock(&pci_lock) # acquire pci_lock
bus->ops->read/write # struct pci_ops
pci_read # x86 pci_root_ops
raw_pci_read
raw_pci_ops->read
pci_conf1_read # x86 raw_pci_ops
lock(&pci_config_lock) # acquire pci_config_lock
And here's a path on x86 that uses the low-level routines directly and
requires the locking there:
acpi_os_read_pci_configuration
raw_pci_read
raw_pci_ops->read
pci_conf1_read
lock(&pci_config_lock)
So ideally I think the locking would be done in the low-level routines
that need it, and we could do without pci_lock. But I don't know
whether that's practical at this point or not.
Bjorn
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
2015-02-17 23:14 ` Feng Kan
@ 2015-03-06 4:54 ` Bjorn Helgaas
-1 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2015-03-06 4:54 UTC (permalink / raw)
To: Feng Kan; +Cc: patches, linux-pci, linux-kernel, robh, linux-arm-kernel,
tinamdar
On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
> The generic accessor functions for pci-xgene uses map_bus
> call that returns the base address but did not add the additional
> offset.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
Applied to for-linus for v4.0, with acks from Tanmay and Rob. Thanks!
> ---
> drivers/pci/host/pci-xgene.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index aab5547..ee082c0 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
> return false;
> }
>
> -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> int offset)
> {
> struct xgene_pcie_port *port = bus->sysdata;
> @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> return NULL;
>
> xgene_pcie_set_rtdid_reg(bus, devfn);
> - return xgene_pcie_get_cfg_base(bus);
> + return xgene_pcie_get_cfg_base(bus) + offset;
> }
>
> static struct pci_ops xgene_pcie_ops = {
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 20+ messages in thread* [PATCH] pci: host: xgene: fix incorrectly returned address by map_bus
@ 2015-03-06 4:54 ` Bjorn Helgaas
0 siblings, 0 replies; 20+ messages in thread
From: Bjorn Helgaas @ 2015-03-06 4:54 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Feb 17, 2015 at 03:14:00PM -0800, Feng Kan wrote:
> The generic accessor functions for pci-xgene uses map_bus
> call that returns the base address but did not add the additional
> offset.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
Applied to for-linus for v4.0, with acks from Tanmay and Rob. Thanks!
> ---
> drivers/pci/host/pci-xgene.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c
> index aab5547..ee082c0 100644
> --- a/drivers/pci/host/pci-xgene.c
> +++ b/drivers/pci/host/pci-xgene.c
> @@ -127,7 +127,7 @@ static bool xgene_pcie_hide_rc_bars(struct pci_bus *bus, int offset)
> return false;
> }
>
> -static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> +static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> int offset)
> {
> struct xgene_pcie_port *port = bus->sysdata;
> @@ -137,7 +137,7 @@ static int xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn,
> return NULL;
>
> xgene_pcie_set_rtdid_reg(bus, devfn);
> - return xgene_pcie_get_cfg_base(bus);
> + return xgene_pcie_get_cfg_base(bus) + offset;
> }
>
> static struct pci_ops xgene_pcie_ops = {
> --
> 1.9.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
^ permalink raw reply [flat|nested] 20+ messages in thread