| Message ID | 1424214840-26498-1-git-send-email-fkan@apm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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 > -- 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
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 -- 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
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 -- 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
[+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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 = {
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(-)