Re: [PATCH v4] ARM: fix debug prints relevant to PCI devices
On Tue, Sep 23, 2014 at 9:04 AM, Thierry Reding
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] ARM: fix debug prints relevant to PCI devices
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
pgphI_uhs2GhX.pgp
Description: PGP signature
Re: [PATCH v4] ARM: fix debug prints relevant to PCI devices
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.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] ARM: fix debug prints relevant to PCI devices
On Sun, Sep 7, 2014 at 4:28 AM, Vidya Sagar wrote:
> From: Vidya Sagar
>
> 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
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] ARM: fix debug prints relevant to PCI devices
Anyone ???
On Sun, Sep 7, 2014 at 3:58 PM, Vidya Sagar wrote:
> From: Vidya Sagar
>
> 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
> ---
> 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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
