| Message ID | 5AE9B5BB.2080003@kontron.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 02, 2018 at 12:57:31PM +0000, Gilles Buloz wrote: > Hi Bjorn, > See attached patch (tested ok this morning) This looks good. Minor comments below. I can fix minor things myself, but I do need a signed-off-by from you before applying (see https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst) Please add a changelog, too, and include the patch inline (as opposed to as an attachment) if possible. > --- include/linux/pci.h.orig 2018-03-26 16:51:18.050000000 +0000 > +++ include/linux/pci.h 2018-04-30 18:29:14.140000000 +0000 > @@ -193,6 +193,7 @@ > enum pci_bus_flags { > PCI_BUS_FLAGS_NO_MSI = (__force pci_bus_flags_t) 1, > PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2, > + PCI_BUS_FLAGS_NO_EXTCFG = (__force pci_bus_flags_t) 4, Best if you can rebase this to v4.17-rc1. > }; > > /* These values come from the PCI Express Spec */ > --- drivers/pci/probe.c.orig 2018-01-22 09:29:52.000000000 +0000 > +++ drivers/pci/probe.c 2018-05-02 13:44:35.530000000 +0000 > @@ -664,6 +664,23 @@ > } > } > > +static bool pci_bridge_child_bus_ext_cfg_accessible(struct pci_dev *bridge) > +{ > + int pos; > + u32 status; > + > + if (!pci_is_pcie(bridge) || /* PCI/PCI bridge */ > + (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */ > + (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)) { /* PCIe/PCI bridge in reverse mode */ > + pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX); > + if (pos) > + pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status); > + return pos && (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)); > + } Please arrange this so everything fits in 80 columns. If you can split it into several simpler "if" statements rather than one with a complicated expression, that would also be good. > + > + return true; > +} > + > static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, > struct pci_dev *bridge, int busnr) > { > @@ -725,6 +742,19 @@ > /* Create legacy_io and legacy_mem files for this bus */ > pci_create_legacy_files(child); > > + /* > + * if bus_flags inherited from parent bus do not already report lack of extended config > + * space support, check if supported by child bus by checking its parent bridge > + */ Wrap to fit in 80 columns. > + if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)) { The double negative makes this a little bit hard to read. Maybe it could be improved by reversing the sense of something? > + if (!pci_bridge_child_bus_ext_cfg_accessible(bridge)) { > + child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG; > + dev_info(&child->dev, "extended config space not accessible due to parent bridge\n"); In v4.17-rc1, there's a pci_info() that should work here (instead of dev_info()). > + } > + } else { > + dev_info(&child->dev, "extended config space not accessible due to parent bus\n"); > + } > + > return child; > } > > @@ -1084,6 +1114,9 @@ > u32 status; > u16 class; > > + if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG) > + return PCI_CFG_SPACE_SIZE; > + > class = dev->class >> 8; > if (class == PCI_CLASS_BRIDGE_HOST) > return pci_cfg_space_size_ext(dev);
Le 02/05/2018 15:26, Bjorn Helgaas a écrit : > On Wed, May 02, 2018 at 12:57:31PM +0000, Gilles Buloz wrote: >> Hi Bjorn, >> See attached patch (tested ok this morning) > This looks good. Minor comments below. > > I can fix minor things myself, but I do need a signed-off-by from you > before applying (see > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst) > > Please add a changelog, too, and include the patch inline (as opposed > to as an attachment) if possible. > >> --- include/linux/pci.h.orig 2018-03-26 16:51:18.050000000 +0000 >> +++ include/linux/pci.h 2018-04-30 18:29:14.140000000 +0000 >> @@ -193,6 +193,7 @@ >> enum pci_bus_flags { >> PCI_BUS_FLAGS_NO_MSI = (__force pci_bus_flags_t) 1, >> PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2, >> + PCI_BUS_FLAGS_NO_EXTCFG = (__force pci_bus_flags_t) 4, > Best if you can rebase this to v4.17-rc1. > >> }; >> >> /* These values come from the PCI Express Spec */ >> --- drivers/pci/probe.c.orig 2018-01-22 09:29:52.000000000 +0000 >> +++ drivers/pci/probe.c 2018-05-02 13:44:35.530000000 +0000 >> @@ -664,6 +664,23 @@ >> } >> } >> >> +static bool pci_bridge_child_bus_ext_cfg_accessible(struct pci_dev *bridge) >> +{ >> + int pos; >> + u32 status; >> + >> + if (!pci_is_pcie(bridge) || /* PCI/PCI bridge */ >> + (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */ >> + (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)) { /* PCIe/PCI bridge in reverse mode */ >> + pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX); >> + if (pos) >> + pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status); >> + return pos && (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)); >> + } > Please arrange this so everything fits in 80 columns. > > If you can split it into several simpler "if" statements rather > than one with a complicated expression, that would also be good. > >> + >> + return true; >> +} >> + >> static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, >> struct pci_dev *bridge, int busnr) >> { >> @@ -725,6 +742,19 @@ >> /* Create legacy_io and legacy_mem files for this bus */ >> pci_create_legacy_files(child); >> >> + /* >> + * if bus_flags inherited from parent bus do not already report lack of extended config >> + * space support, check if supported by child bus by checking its parent bridge >> + */ > Wrap to fit in 80 columns. > >> + if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)) { > The double negative makes this a little bit hard to read. Maybe it > could be improved by reversing the sense of something? > >> + if (!pci_bridge_child_bus_ext_cfg_accessible(bridge)) { >> + child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG; >> + dev_info(&child->dev, "extended config space not accessible due to parent bridge\n"); > In v4.17-rc1, there's a pci_info() that should work here (instead of > dev_info()). > >> + } >> + } else { >> + dev_info(&child->dev, "extended config space not accessible due to parent bus\n"); >> + } >> + >> return child; >> } >> >> @@ -1084,6 +1114,9 @@ >> u32 status; >> u16 class; >> >> + if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG) >> + return PCI_CFG_SPACE_SIZE; >> + >> class = dev->class >> 8; >> if (class == PCI_CLASS_BRIDGE_HOST) >> return pci_cfg_space_size_ext(dev); > . > OK I'm going to learn about signing (sorry this is my first "official" patch). I'll download kernel v4.17-rc1 and write the patch for it; however I hope I'll be able to test it on my platform without the freescale addons I have on 4.1.35, because I don't want to send an untested patch. For "if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG))", I don't understand what you mean with "double negative", as I only have one "!" Do you think it's worth keeping the two dev_info() ? The code would be smaller without; however this may help to have it for debug. Maybe use _dbg instead of _info ?
On Wed, May 02, 2018 at 01:48:27PM +0000, Gilles Buloz wrote: > Le 02/05/2018 15:26, Bjorn Helgaas a écrit : > > On Wed, May 02, 2018 at 12:57:31PM +0000, Gilles Buloz wrote: > >> Hi Bjorn, > >> See attached patch (tested ok this morning) > > This looks good. Minor comments below. > > > > I can fix minor things myself, but I do need a signed-off-by from you > > before applying (see > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst) > > > > Please add a changelog, too, and include the patch inline (as opposed > > to as an attachment) if possible. > > > >> --- include/linux/pci.h.orig 2018-03-26 16:51:18.050000000 +0000 > >> +++ include/linux/pci.h 2018-04-30 18:29:14.140000000 +0000 > >> @@ -193,6 +193,7 @@ > >> enum pci_bus_flags { > >> PCI_BUS_FLAGS_NO_MSI = (__force pci_bus_flags_t) 1, > >> PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2, > >> + PCI_BUS_FLAGS_NO_EXTCFG = (__force pci_bus_flags_t) 4, > > Best if you can rebase this to v4.17-rc1. > > > >> }; > >> > >> /* These values come from the PCI Express Spec */ > >> --- drivers/pci/probe.c.orig 2018-01-22 09:29:52.000000000 +0000 > >> +++ drivers/pci/probe.c 2018-05-02 13:44:35.530000000 +0000 > >> @@ -664,6 +664,23 @@ > >> } > >> } > >> > >> +static bool pci_bridge_child_bus_ext_cfg_accessible(struct pci_dev *bridge) > >> +{ > >> + int pos; > >> + u32 status; > >> + > >> + if (!pci_is_pcie(bridge) || /* PCI/PCI bridge */ > >> + (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */ > >> + (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)) { /* PCIe/PCI bridge in reverse mode */ > >> + pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX); > >> + if (pos) > >> + pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status); > >> + return pos && (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)); > >> + } > > Please arrange this so everything fits in 80 columns. > > > > If you can split it into several simpler "if" statements rather > > than one with a complicated expression, that would also be good. > > > >> + > >> + return true; > >> +} > >> + > >> static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, > >> struct pci_dev *bridge, int busnr) > >> { > >> @@ -725,6 +742,19 @@ > >> /* Create legacy_io and legacy_mem files for this bus */ > >> pci_create_legacy_files(child); > >> > >> + /* > >> + * if bus_flags inherited from parent bus do not already report lack of extended config > >> + * space support, check if supported by child bus by checking its parent bridge > >> + */ > > Wrap to fit in 80 columns. > > > >> + if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)) { > > The double negative makes this a little bit hard to read. Maybe it > > could be improved by reversing the sense of something? > > > >> + if (!pci_bridge_child_bus_ext_cfg_accessible(bridge)) { > >> + child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG; > >> + dev_info(&child->dev, "extended config space not accessible due to parent bridge\n"); > > In v4.17-rc1, there's a pci_info() that should work here (instead of > > dev_info()). > > > >> + } > >> + } else { > >> + dev_info(&child->dev, "extended config space not accessible due to parent bus\n"); > >> + } > >> + > >> return child; > >> } > >> > >> @@ -1084,6 +1114,9 @@ > >> u32 status; > >> u16 class; > >> > >> + if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG) > >> + return PCI_CFG_SPACE_SIZE; > >> + > >> class = dev->class >> 8; > >> if (class == PCI_CLASS_BRIDGE_HOST) > >> return pci_cfg_space_size_ext(dev); > > . > > > OK I'm going to learn about signing (sorry this is my first > "official" patch). Great, welcome! The signoff is no big deal -- it's just plain text (no crypto signature or anything) and it's basically just an assertion that you wrote it and have the right to contribute it. > I'll download kernel v4.17-rc1 and write the patch for it; however I > hope I'll be able to test it on my platform without the freescale > addons I have on 4.1.35, because I don't want to send an untested > patch. Don't worry too much about the 4.1 vs 4.17 issue. If you tested it on 4.1.35 that should be fine. > For "if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG))", > I don't understand what you mean with "double negative", as I only > have one "!" The "!" and the "NO" part of "NO_EXTCFG" is what I meant. E.g., maybe the flag could be something like "COMPAT_CFG_ONLY" so there's no negation in the test at all. > Do you think it's worth keeping the two dev_info() ? The code would > be smaller without; however this may help to have it for debug. > Maybe use _dbg instead of _info ? Probably one pci_info() is enough as a clue that extended config space isn't available below this point in the hierarchy. I personally don't like the _dbg() version because it's so complicated to figure out when the output is enabled. Bjorn
--- include/linux/pci.h.orig 2018-03-26 16:51:18.050000000 +0000 +++ include/linux/pci.h 2018-04-30 18:29:14.140000000 +0000 @@ -193,6 +193,7 @@ enum pci_bus_flags { PCI_BUS_FLAGS_NO_MSI = (__force pci_bus_flags_t) 1, PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2, + PCI_BUS_FLAGS_NO_EXTCFG = (__force pci_bus_flags_t) 4, }; /* These values come from the PCI Express Spec */ --- drivers/pci/probe.c.orig 2018-01-22 09:29:52.000000000 +0000 +++ drivers/pci/probe.c 2018-05-02 13:44:35.530000000 +0000 @@ -664,6 +664,23 @@ } } +static bool pci_bridge_child_bus_ext_cfg_accessible(struct pci_dev *bridge) +{ + int pos; + u32 status; + + if (!pci_is_pcie(bridge) || /* PCI/PCI bridge */ + (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCIE_BRIDGE) || /* PCIe/PCI bridge in forward mode */ + (pci_pcie_type(bridge) == PCI_EXP_TYPE_PCI_BRIDGE)) { /* PCIe/PCI bridge in reverse mode */ + pos = pci_find_capability(bridge, PCI_CAP_ID_PCIX); + if (pos) + pci_read_config_dword(bridge, pos + PCI_X_STATUS, &status); + return pos && (status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)); + } + + return true; +} + static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, struct pci_dev *bridge, int busnr) { @@ -725,6 +742,19 @@ /* Create legacy_io and legacy_mem files for this bus */ pci_create_legacy_files(child); + /* + * if bus_flags inherited from parent bus do not already report lack of extended config + * space support, check if supported by child bus by checking its parent bridge + */ + if (bridge && !(child->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG)) { + if (!pci_bridge_child_bus_ext_cfg_accessible(bridge)) { + child->bus_flags |= PCI_BUS_FLAGS_NO_EXTCFG; + dev_info(&child->dev, "extended config space not accessible due to parent bridge\n"); + } + } else { + dev_info(&child->dev, "extended config space not accessible due to parent bus\n"); + } + return child; } @@ -1084,6 +1114,9 @@ u32 status; u16 class; + if (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_EXTCFG) + return PCI_CFG_SPACE_SIZE; + class = dev->class >> 8; if (class == PCI_CLASS_BRIDGE_HOST) return pci_cfg_space_size_ext(dev);