| Message ID | 1410085687-11273-1-git-send-email-vidyas@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Anyone ??? On Sun, Sep 7, 2014 at 3:58 PM, Vidya Sagar <sagar.tv@gmail.com> wrote: > From: Vidya Sagar <sagar.tv@gmail.com> > > As per PCIe spec, fast back-to-back transactions feature > is not applicable to PCIe devices. Hence, do not print > that fast back-to-back trasactions are disabled when > there is a PCIe device found on the bus > > Signed-off-by: Vidya Sagar <sagar.tv@gmail.com> > --- > v4: > * initialized 'has_pcie_dev' to 'false' > > v3: > * removed KERN_INFO from pr_info() which was not removed by mistake in previous patch > > v2: > * Modified has_pcie_dev type to bool and used pci_is_pcie() instead of pci_pcie_cap() > * replaced printk with pr_info > > arch/arm/kernel/bios32.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > index 17a26c1..03c56ba 100644 > --- a/arch/arm/kernel/bios32.c > +++ b/arch/arm/kernel/bios32.c > @@ -290,6 +290,7 @@ void pcibios_fixup_bus(struct pci_bus *bus) > { > struct pci_dev *dev; > u16 features = PCI_COMMAND_SERR | PCI_COMMAND_PARITY | PCI_COMMAND_FAST_BACK; > + bool has_pcie_dev = false; > > /* > * Walk the devices on this bus, working out what we can > @@ -298,6 +299,8 @@ void pcibios_fixup_bus(struct pci_bus *bus) > list_for_each_entry(dev, &bus->devices, bus_list) { > u16 status; > > + if (!has_pcie_dev) > + has_pcie_dev = pci_is_pcie(dev); > pci_read_config_word(dev, PCI_STATUS, &status); > > /* > @@ -354,9 +357,11 @@ void pcibios_fixup_bus(struct pci_bus *bus) > > /* > * Report what we did for this bus > + * (only if the bus doesn't have any PCIe devices) > */ > - printk(KERN_INFO "PCI: bus%d: Fast back to back transfers %sabled\n", > - bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis"); > + if (!has_pcie_dev) > + pr_info("PCI: bus%d: Fast back to back transfers %sabled\n", > + bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis"); > } > EXPORT_SYMBOL(pcibios_fixup_bus); > > -- > 1.8.1.5 >
On Sun, Sep 7, 2014 at 4:28 AM, Vidya Sagar <sagar.tv@gmail.com> wrote: > From: Vidya Sagar <sagar.tv@gmail.com> > > As per PCIe spec, fast back-to-back transactions feature > is not applicable to PCIe devices. Hence, do not print > that fast back-to-back trasactions are disabled when > there is a PCIe device found on the bus > > Signed-off-by: Vidya Sagar <sagar.tv@gmail.com> > --- > v4: > * initialized 'has_pcie_dev' to 'false' > > v3: > * removed KERN_INFO from pr_info() which was not removed by mistake in previous patch > > v2: > * Modified has_pcie_dev type to bool and used pci_is_pcie() instead of pci_pcie_cap() > * replaced printk with pr_info > > arch/arm/kernel/bios32.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) This doesn't require any coordination with the PCI core, so I was just leaving this up to the arch. But I guess I can at least give you my opinion :) > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > index 17a26c1..03c56ba 100644 > --- a/arch/arm/kernel/bios32.c > +++ b/arch/arm/kernel/bios32.c > @@ -290,6 +290,7 @@ void pcibios_fixup_bus(struct pci_bus *bus) > { > struct pci_dev *dev; > u16 features = PCI_COMMAND_SERR | PCI_COMMAND_PARITY | PCI_COMMAND_FAST_BACK; > + bool has_pcie_dev = false; > > /* > * Walk the devices on this bus, working out what we can > @@ -298,6 +299,8 @@ void pcibios_fixup_bus(struct pci_bus *bus) > list_for_each_entry(dev, &bus->devices, bus_list) { > u16 status; > > + if (!has_pcie_dev) > + has_pcie_dev = pci_is_pcie(dev); > pci_read_config_word(dev, PCI_STATUS, &status); > > /* > @@ -354,9 +357,11 @@ void pcibios_fixup_bus(struct pci_bus *bus) > > /* > * Report what we did for this bus > + * (only if the bus doesn't have any PCIe devices) > */ > - printk(KERN_INFO "PCI: bus%d: Fast back to back transfers %sabled\n", > - bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis"); > + if (!has_pcie_dev) > + pr_info("PCI: bus%d: Fast back to back transfers %sabled\n", > + bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis"); My first choice would be to just drop the printk altogether. If we want to keep the printk, it should be enough to test "bus->self && !pci_is_pcie(bus->self)" to see whether Fast Back-to-Back can be enabled. Personally, I would like to see everything in the file converted to use dev_printk so it's consistent with the PCI core. That would be a separate patch, and there might be other instances under arch/arm, too. > } > EXPORT_SYMBOL(pcibios_fixup_bus); > > -- > 1.8.1.5 >
On Tue, Sep 23, 2014 at 07:56:01AM -0600, Bjorn Helgaas wrote: > This doesn't require any coordination with the PCI core, so I was just > leaving this up to the arch. But I guess I can at least give you my > opinion :) However, PCI core people have more knowledge of the issues here than I do. > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > > index 17a26c1..03c56ba 100644 > > --- a/arch/arm/kernel/bios32.c > > +++ b/arch/arm/kernel/bios32.c > > @@ -290,6 +290,7 @@ void pcibios_fixup_bus(struct pci_bus *bus) > > { > > struct pci_dev *dev; > > u16 features = PCI_COMMAND_SERR | PCI_COMMAND_PARITY | PCI_COMMAND_FAST_BACK; > > + bool has_pcie_dev = false; > > > > /* > > * Walk the devices on this bus, working out what we can > > @@ -298,6 +299,8 @@ void pcibios_fixup_bus(struct pci_bus *bus) > > list_for_each_entry(dev, &bus->devices, bus_list) { > > u16 status; > > > > + if (!has_pcie_dev) > > + has_pcie_dev = pci_is_pcie(dev); > > pci_read_config_word(dev, PCI_STATUS, &status); > > > > /* > > @@ -354,9 +357,11 @@ void pcibios_fixup_bus(struct pci_bus *bus) > > > > /* > > * Report what we did for this bus > > + * (only if the bus doesn't have any PCIe devices) > > */ > > - printk(KERN_INFO "PCI: bus%d: Fast back to back transfers %sabled\n", > > - bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis"); > > + if (!has_pcie_dev) > > + pr_info("PCI: bus%d: Fast back to back transfers %sabled\n", > > + bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis"); > > My first choice would be to just drop the printk altogether. It can be useful information. > If we want to keep the printk, it should be enough to test "bus->self > && !pci_is_pcie(bus->self)" to see whether Fast Back-to-Back can be > enabled. This is exactly the kind of issue that needs to be picked up by core PCI people. I've not looked at PCI stuff for a long time, because PCI isn't that relevent to me anymore, so I'm not up to speed with any PCI API changes since about 2.6.xx days, and I'm certainly not knowledgable of PCIe. To a certain extent, that can be blamed on ARM's eval boards either having fundamentally fscked and unusable PCIe, or ARM not bothering to supply me PCI backplanes to be able to use them. Isn't bus->self NULL for the PCI root bus, which would be one of the buses which we /do/ want to print this information for. So, wouldn't: !bus->self || !pci_is_pcie(bus->self) be more correct? > Personally, I would like to see everything in the file converted to > use dev_printk so it's consistent with the PCI core. That would be a > separate patch, and there might be other instances under arch/arm, > too. It /was/ consistent with the PCI core, because the PCI core used to use this formatting. If you wish to keep it consistent, please submit a patch to make it consistent /again/ with the core code.
On Tue, Sep 23, 2014 at 03:06:35PM +0100, Russell King - ARM Linux wrote: > On Tue, Sep 23, 2014 at 07:56:01AM -0600, Bjorn Helgaas wrote: > > This doesn't require any coordination with the PCI core, so I was just > > leaving this up to the arch. But I guess I can at least give you my > > opinion :) > > However, PCI core people have more knowledge of the issues here than I do. > > > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > > > index 17a26c1..03c56ba 100644 > > > --- a/arch/arm/kernel/bios32.c > > > +++ b/arch/arm/kernel/bios32.c > > > @@ -290,6 +290,7 @@ void pcibios_fixup_bus(struct pci_bus *bus) > > > { > > > struct pci_dev *dev; > > > u16 features = PCI_COMMAND_SERR | PCI_COMMAND_PARITY | PCI_COMMAND_FAST_BACK; > > > + bool has_pcie_dev = false; > > > > > > /* > > > * Walk the devices on this bus, working out what we can > > > @@ -298,6 +299,8 @@ void pcibios_fixup_bus(struct pci_bus *bus) > > > list_for_each_entry(dev, &bus->devices, bus_list) { > > > u16 status; > > > > > > + if (!has_pcie_dev) > > > + has_pcie_dev = pci_is_pcie(dev); > > > pci_read_config_word(dev, PCI_STATUS, &status); > > > > > > /* > > > @@ -354,9 +357,11 @@ void pcibios_fixup_bus(struct pci_bus *bus) > > > > > > /* > > > * Report what we did for this bus > > > + * (only if the bus doesn't have any PCIe devices) > > > */ > > > - printk(KERN_INFO "PCI: bus%d: Fast back to back transfers %sabled\n", > > > - bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis"); > > > + if (!has_pcie_dev) > > > + pr_info("PCI: bus%d: Fast back to back transfers %sabled\n", > > > + bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis"); > > > > My first choice would be to just drop the printk altogether. > > It can be useful information. > > > If we want to keep the printk, it should be enough to test "bus->self > > && !pci_is_pcie(bus->self)" to see whether Fast Back-to-Back can be > > enabled. > > This is exactly the kind of issue that needs to be picked up by core > PCI people. I agree. Wouldn't it be more useful to move this into the core? Disabling fast back-to-back transfers hardly seems like an ARM- specific "fixup" to me. Thierry
On Tue, Sep 23, 2014 at 9:04 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Sep 23, 2014 at 03:06:35PM +0100, Russell King - ARM Linux wrote: >> On Tue, Sep 23, 2014 at 07:56:01AM -0600, Bjorn Helgaas wrote: >> > This doesn't require any coordination with the PCI core, so I was just >> > leaving this up to the arch. But I guess I can at least give you my >> > opinion :) >> >> However, PCI core people have more knowledge of the issues here than I do. >> >> > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c >> > > index 17a26c1..03c56ba 100644 >> > > --- a/arch/arm/kernel/bios32.c >> > > +++ b/arch/arm/kernel/bios32.c >> > > @@ -290,6 +290,7 @@ void pcibios_fixup_bus(struct pci_bus *bus) >> > > { >> > > struct pci_dev *dev; >> > > u16 features = PCI_COMMAND_SERR | PCI_COMMAND_PARITY | PCI_COMMAND_FAST_BACK; >> > > + bool has_pcie_dev = false; >> > > >> > > /* >> > > * Walk the devices on this bus, working out what we can >> > > @@ -298,6 +299,8 @@ void pcibios_fixup_bus(struct pci_bus *bus) >> > > list_for_each_entry(dev, &bus->devices, bus_list) { >> > > u16 status; >> > > >> > > + if (!has_pcie_dev) >> > > + has_pcie_dev = pci_is_pcie(dev); >> > > pci_read_config_word(dev, PCI_STATUS, &status); >> > > >> > > /* >> > > @@ -354,9 +357,11 @@ void pcibios_fixup_bus(struct pci_bus *bus) >> > > >> > > /* >> > > * Report what we did for this bus >> > > + * (only if the bus doesn't have any PCIe devices) >> > > */ >> > > - printk(KERN_INFO "PCI: bus%d: Fast back to back transfers %sabled\n", >> > > - bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis"); >> > > + if (!has_pcie_dev) >> > > + pr_info("PCI: bus%d: Fast back to back transfers %sabled\n", >> > > + bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis"); >> > >> > My first choice would be to just drop the printk altogether. >> >> It can be useful information. >> >> > If we want to keep the printk, it should be enough to test "bus->self >> > && !pci_is_pcie(bus->self)" to see whether Fast Back-to-Back can be >> > enabled. >> >> This is exactly the kind of issue that needs to be picked up by core >> PCI people. > > I agree. Wouldn't it be more useful to move this into the core? > Disabling fast back-to-back transfers hardly seems like an ARM- > specific "fixup" to me. I would definitely support moving this into the core.
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c index 17a26c1..03c56ba 100644 --- a/arch/arm/kernel/bios32.c +++ b/arch/arm/kernel/bios32.c @@ -290,6 +290,7 @@ void pcibios_fixup_bus(struct pci_bus *bus) { struct pci_dev *dev; u16 features = PCI_COMMAND_SERR | PCI_COMMAND_PARITY | PCI_COMMAND_FAST_BACK; + bool has_pcie_dev = false; /* * Walk the devices on this bus, working out what we can @@ -298,6 +299,8 @@ void pcibios_fixup_bus(struct pci_bus *bus) list_for_each_entry(dev, &bus->devices, bus_list) { u16 status; + if (!has_pcie_dev) + has_pcie_dev = pci_is_pcie(dev); pci_read_config_word(dev, PCI_STATUS, &status); /* @@ -354,9 +357,11 @@ void pcibios_fixup_bus(struct pci_bus *bus) /* * Report what we did for this bus + * (only if the bus doesn't have any PCIe devices) */ - printk(KERN_INFO "PCI: bus%d: Fast back to back transfers %sabled\n", - bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis"); + if (!has_pcie_dev) + pr_info("PCI: bus%d: Fast back to back transfers %sabled\n", + bus->number, (features & PCI_COMMAND_FAST_BACK) ? "en" : "dis"); } EXPORT_SYMBOL(pcibios_fixup_bus);