PCI updates for v3.11

GIT PULL Bjorn Helgaas 4 participants 7 messages Aug 2, 2013 – Aug 6, 2013

Subsystems (3)

Kbuild PCI Kconfig

lore

Share & tools

lore

Bjorn Helgaas

author maintainer

2013-08-02 18:17 UTC #0 lore raw

Hi Linus,

Here are some fixes for v3.11. Yinghai fixed a couple regressions:
one resource assignment problem introduced in v3.10 that showed up with SR-IOV on powerpc, and another SR-IOV hot-remove issue related to refcounting changes we merged for v3.11.

Yinghai is still working on another SR-IOV-related fix or two, which will be simpler if pciehp is non-modular, so I included the Kconfig changes now to get them in earlier.

Finally, a minor fix for the ARM Marvell EBU host bridge driver that was merged for v3.11.

Bjorn

The following changes since commit 3b2f64d00c46e1e4e9bd0bb9bb12619adac27a4b:

Linux 3.11-rc2 (2013-07-21 12:05:29 -0700)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git tags/pci-v3.11-fixes-1

for you to fetch changes up to 36dd1f3e02a4aed850a7b7318d7abd4f4d50528c:

PCI: mvebu: Disable prefetchable memory support in PCI-to-PCI bridge (2013-08-01 14:47:54 -0600)

----------------------------------------------------------------
PCI updates for v3.11:

Hotplug
PCI: pciehp: Fix null pointer deref when hot-removing SR-IOV device
PCI: hotplug: Convert to be builtin only, not modular
PCI: pciehp: Convert pciehp to be builtin only, not modular
Resource allocation
PCI: Retry allocation of only the resource type that failed
ARM
PCI: mvebu: Disable prefetchable memory support in PCI-to-PCI bridge

----------------------------------------------------------------
Bjorn Helgaas (2):
PCI: hotplug: Convert to be builtin only, not modular
PCI: pciehp: Convert pciehp to be builtin only, not modular

Thomas Petazzoni (1):
PCI: mvebu: Disable prefetchable memory support in PCI-to-PCI bridge

Yinghai Lu (2):
PCI: pciehp: Fix null pointer deref when hot-removing SR-IOV device
PCI: Retry allocation of only the resource type that failed

 arch/ia64/configs/generic_defconfig    |  2 +-
 arch/ia64/configs/gensparse_defconfig  |  2 +-
 arch/ia64/configs/tiger_defconfig      |  2 +-
 arch/ia64/configs/xen_domu_defconfig   |  2 +-
 arch/powerpc/configs/ppc64_defconfig   |  2 +-
 arch/powerpc/configs/ppc64e_defconfig  |  2 +-
 arch/powerpc/configs/pseries_defconfig |  2 +-
 arch/sh/configs/sh03_defconfig         |  2 +-
 drivers/pci/host/pci-mvebu.c           | 27 +------------
 drivers/pci/hotplug/Kconfig            |  5 +--
 drivers/pci/hotplug/pciehp_pci.c       |  9 ++++-
 drivers/pci/pcie/Kconfig               |  5 +--
 drivers/pci/setup-bus.c                | 69 +++++++++++++++++++++++++++++++++-
 13 files changed, 87 insertions(+), 44 deletions(-)

Linus Torvalds

2013-08-02 20:28 UTC #1 lore raw

Quoted 4 lines · Bjorn Helgaas

Yinghai is still working on another SR-IOV-related fix or two, which will be simpler if pciehp is non-modular, so I included the Kconfig changes now to get them in earlier.

Hmm. Doing a trivial "make allmoconfig" for testing, I get

include/config/auto.conf:3014:warning: symbol value 'm' invalid for
HOTPLUG_PCI_PCIE
include/config/auto.conf:4711:warning: symbol value 'm' invalid for
HOTPLUG_PCI

but that may be a build system issue with stale data from the *previous* "make allmodconfig". Regardless, that makes me worried.

Adding Michal Marek to the discussion. I'm currently doing a new "make allmodconfig" after having done a "git clean -dqfx" to see if the error remains. If it does, I will unpull. If it is gone, I'm going to assume the Kconfig changes are ok, but that our build system is missing some dependency.

Michal? I haven't pushed things out yet (and that allmodconfig build from scratch will take 20 min or so), but you can recreate my tree by pulling that pci thing locally from:

git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git
tags/pci-v3.11-fixes-1

Ho humm..

Linus

Linus Torvalds

2013-08-02 21:24 UTC #2 lore raw

Quoted 4 lines · Linus Torvalds

Michal? I haven't pushed things out yet (and that allmodconfig build from scratch will take 20 min or so), but you can recreate my tree by pulling that pci thing locally from:

Yeah, it looks like if you start with an allmodconfig build from before that pull, and then re-do "make allmodconfig" after the pull without cleaning the build tree you get that warning.

[ Music from "Jaws" ]

Scary.

Linus

Michal Marek

2013-08-05 08:39 UTC #3 lore raw

Warnings from silentoldconfig (Re: [GIT PULL] PCI updates for v3.11)

In reply to Linus Torvalds

Quoted 13 lines · Linus Torvalds

Quoted 4 lines · Bjorn Helgaas
Yinghai is still working on another SR-IOV-related fix or two, which will be simpler if pciehp is non-modular, so I included the Kconfig changes now to get them in earlier.
Hmm. Doing a trivial "make allmoconfig" for testing, I get
include/config/auto.conf:3014:warning: symbol value 'm' invalid for
HOTPLUG_PCI_PCIE
include/config/auto.conf:4711:warning: symbol value 'm' invalid for
HOTPLUG_PCI

Quoted 8 lines

but that may be a build system issue with stale data from the *previous* "make allmodconfig". Regardless, that makes me worried.
Adding Michal Marek to the discussion. I'm currently doing a new "make allmodconfig" after having done a "git clean -dqfx" to see if the error remains. If it does, I will unpull. If it is gone, I'm going to assume the Kconfig changes are ok, but that our build system is missing some dependency.

Added Yann and the linux-kbuild list to CC. Reproducer:

git checkout 1fe0135
make mrproper
make allmodconfig
make silentoldconfig
git checkout aa8032b
make allmodconfig
make silentoldconfig

conf_write_autoconf() first calls conf_split_config() to generate the include/config/**.h hierarchy, then generates include/config/auto.conf. For some reason, conf_split_config() reads include/config/auto.conf, which may not exist yet or may be out of date. Yann, can anything break if we simply do not read that file from conf_split_config(), like this?

scripts/kconfig/confdata.c +0 -4

index c55c227..8c90835 100644
@@ -829,16 +829,12 @@ next:
 
 static int conf_split_config(void)
 {
-	const char *name;
 	char path[PATH_MAX+1];
 	char *s, *d, c;
 	struct symbol *sym;
 	struct stat sb;
 	int res, i, fd;
 
-	name = conf_get_autoconfig_name();
-	conf_read_simple(name, S_DEF_AUTO);
-
 	if (chdir("include/config"))
 		return 1;

Thanks,
Michal

Yann E. MORIN

2013-08-05 09:26 UTC #4 lore raw

Re: Warnings from silentoldconfig (Re: [GIT PULL] PCI updates for v3.11)

Michal, All,

Quoted 33 lines · On Monday 05 August 2013 10:39:43 Michal Marek wrote:

Quoted 13 lines · Linus Torvalds
Quoted 4 lines · Bjorn Helgaas
Yinghai is still working on another SR-IOV-related fix or two, which will be simpler if pciehp is non-modular, so I included the Kconfig changes now to get them in earlier.
Hmm. Doing a trivial "make allmoconfig" for testing, I get
include/config/auto.conf:3014:warning: symbol value 'm' invalid for
HOTPLUG_PCI_PCIE
include/config/auto.conf:4711:warning: symbol value 'm' invalid for
HOTPLUG_PCI
Quoted 8 lines
but that may be a build system issue with stale data from the *previous* "make allmodconfig". Regardless, that makes me worried.
Adding Michal Marek to the discussion. I'm currently doing a new "make allmodconfig" after having done a "git clean -dqfx" to see if the error remains. If it does, I will unpull. If it is gone, I'm going to assume the Kconfig changes are ok, but that our build system is missing some dependency.
Added Yann and the linux-kbuild list to CC. Reproducer:
git checkout 1fe0135
make mrproper
make allmodconfig
make silentoldconfig
git checkout aa8032b
make allmodconfig
make silentoldconfig

Quoted 5 lines

conf_write_autoconf() first calls conf_split_config() to generate the include/config/**.h hierarchy, then generates include/config/auto.conf. For some reason, conf_split_config() reads include/config/auto.conf, which may not exist yet or may be out of date. Yann, can anything break if we simply do not read that file from conf_split_config(), like this?

Sorry, that's a part I'm not sure about, and seems non-trivial. I'll look at it tonight (UTC+2) when I'm back home.

Regards,
Yann E. MORIN.

Quoted 24 lines

scripts/kconfig/confdata.c +0 -4

index c55c227..8c90835 100644
@@ -829,16 +829,12 @@ next:
 
 static int conf_split_config(void)
 {
-	const char *name;
 	char path[PATH_MAX+1];
 	char *s, *d, c;
 	struct symbol *sym;
 	struct stat sb;
 	int res, i, fd;
 
-	name = conf_get_autoconfig_name();
-	conf_read_simple(name, S_DEF_AUTO);
-
 	if (chdir("include/config"))
 		return 1;

Thanks,
Michal

Signature · 6 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +0/33 662376056 | Software  Designer | \ / CAMPAIGN     |   ^                |
| --==< O_o >==-- '------------.-------:  X  AGAINST      |  /e\  There is no  |
| http://ymorin.is-a-geek.org/ | (*_*) | / \ HTML MAIL    |  """  conspiracy.  |
'------------------------------'-------'------------------'--------------------'

Yann E. MORIN

2013-08-05 22:58 UTC #5 lore raw

Re: Warnings from silentoldconfig (Re: [GIT PULL] PCI updates for v3.11)

In reply to Michal Marek

Michal, All,

On 2013-08-05 10:39 +0200, Michal Marek spake thusly:

Quoted 50 lines · Linus Torvalds

Quoted 13 lines · Bjorn Helgaas
Quoted 4 lines
Yinghai is still working on another SR-IOV-related fix or two, which will be simpler if pciehp is non-modular, so I included the Kconfig changes now to get them in earlier.
Hmm. Doing a trivial "make allmoconfig" for testing, I get
include/config/auto.conf:3014:warning: symbol value 'm' invalid for
HOTPLUG_PCI_PCIE
include/config/auto.conf:4711:warning: symbol value 'm' invalid for
HOTPLUG_PCI
Quoted 8 lines
but that may be a build system issue with stale data from the *previous* "make allmodconfig". Regardless, that makes me worried.
Adding Michal Marek to the discussion. I'm currently doing a new "make allmodconfig" after having done a "git clean -dqfx" to see if the error remains. If it does, I will unpull. If it is gone, I'm going to assume the Kconfig changes are ok, but that our build system is missing some dependency.
Added Yann and the linux-kbuild list to CC. Reproducer:
git checkout 1fe0135
make mrproper
make allmodconfig
make silentoldconfig
git checkout aa8032b
make allmodconfig
make silentoldconfig
conf_write_autoconf() first calls conf_split_config() to generate the include/config/**.h hierarchy, then generates include/config/auto.conf. For some reason, conf_split_config() reads include/config/auto.conf, which may not exist yet or may be out of date. Yann, can anything break if we simply do not read that file from conf_split_config(), like this?
scripts/kconfig/confdata.c +0 -1
index c55c227..8c90835 100644
@@ -829,16 +829,12 @@ next:
 
 static int conf_split_config(void)
 {
-	const char *name;
 	char path[PATH_MAX+1];
 	char *s, *d, c;

scripts/kconfig/confdata.c next: · line 840

Quoted 10 lines

scripts/kconfig/confdata.c +0 -3

 	struct symbol *sym;
 	struct stat sb;
 	int res, i, fd;
 
-	name = conf_get_autoconfig_name();
-	conf_read_simple(name, S_DEF_AUTO);
-
 	if (chdir("include/config"))
 		return 1;

So, first off, this is not something I am comfortable with. So I've done some (quick) research on this, and did some testing (see below).

So, looking at .config after 'make allmodconfig', the HOTPLUG_PCI and HOTPLUG_PCI_PCIE symbols are indeed set to 'y' in .config, which is expected and correct. include/config/auto.conf still has them set to 'm', which is not wrong since it has not been re-generated.

Finally, after make silentoldconfig, they are both set to 'y' both in .config and in include/config/auto.conf. Expected and correct.

This means the warning just informs us that a symbol's type has changed to a more restricted type (eg. tristate -> bool), and is not really a problem, since both .config and auto.conf are correct afterward.

Then, as Michal explained, the warning happens while we are in conf_write_autoconf(), which is called after we have dealt with all symbols, and they are expected to be properly set now. But then, we read include/config/auto.conf, and I get a bit lost there.

What I understand of the code is that, we're reading auto.conf with the S_DEF_AUTO flag, which is expected to set the SYMBOL_DEF_AUTO flags to symbols read from auto.conf.

This is then used by conf_split_config to detect symbols that have changed.

Basically, it loops over all symbols as such:

    for_all_symbols(...) {
        if (!has_changed(symbol))
            continue
        touch include/config/symbol.h
    }

has_changed(symbol) checks the old value is the same (or isn't) as the new value. Is it's the same, nothing happens. Otherwise, as the value has changed, the empty symbol.h file is generated to trigger dependency tracking in the build-system.

So I checked if the .h files in include/config/ were touched.

The two following tests do not invlolve the HOTPLUG_PCI_.* symbols on purpose, and involve a pristine master c095ba7:

    $ make allmodconfig
    $ make silentoldconfig
    $ make silentoldconfig
    $ make silentoldconfig

- Without Michal's proposed change, unaffected symbols are not touched. For example, mtime and ctime for zswap.h are not changed after the second and third silentoldconfig.

- With Michal's proposed change, then mtime and ctime for zswap.h are changed after each silentoldconfig, even though CONFIG_ZSWAP was not changed.

So, I'm afraid this change is incorrect, since it would mean dependency tracking would detect targets not to be up-to-date, and everything would be rebuilt.

Also, the issue has pre-existed for a long time and is only detected now. I think the tristate->bool conversions in this thread just exposed the issue, and are not the root cause.

One solution would be to read auto.conf *before* we fuzz around symbols, not after. We should probably read auto.conf just after we read .config.

I've spent my whole evening on this issue, it's past 00:45 here, and I'm slowly drifting away now... ;-)

I believe the warning to be just spurious, so it can bear one more day before I can tackle this again.

In any case, I've found the kconfig code to be relatively difficult to read and follow... :-( It's mostly an agglomerate of orgnic changes accumulated over time, it is missing coments in the right places, is rather complex with a lot of special cases spread out all over, and I fell like wanting to knit after a slew of kitten has played with tens of balls of wool... :-(

Anyway, with time, I'm eventually sorting things out one at a time, but Woo... the headache... ;-]

Regards,
Yann E. MORIN.

Signature · 6 lines

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Michal Marek

2013-08-06 08:20 UTC #6 lore raw

Re: Warnings from silentoldconfig (Re: [GIT PULL] PCI updates for v3.11)

Quoted 37 lines · Yann E. MORIN

On 2013-08-05 10:39 +0200, Michal Marek spake thusly:
Quoted 36 lines
Added Yann and the linux-kbuild list to CC. Reproducer:
git checkout 1fe0135
make mrproper
make allmodconfig
make silentoldconfig
git checkout aa8032b
make allmodconfig
make silentoldconfig
conf_write_autoconf() first calls conf_split_config() to generate the include/config/**.h hierarchy, then generates include/config/auto.conf. For some reason, conf_split_config() reads include/config/auto.conf, which may not exist yet or may be out of date. Yann, can anything break if we simply do not read that file from conf_split_config(), like this?
scripts/kconfig/confdata.c +0 -4
index c55c227..8c90835 100644
@@ -829,16 +829,12 @@ next:
 
 static int conf_split_config(void)
 {
-	const char *name;
 	char path[PATH_MAX+1];
 	char *s, *d, c;
 	struct symbol *sym;
 	struct stat sb;
 	int res, i, fd;
 
-	name = conf_get_autoconfig_name();
-	conf_read_simple(name, S_DEF_AUTO);
-
 	if (chdir("include/config"))
 		return 1;

[...]

Quoted 7 lines

What I understand of the code is that, we're reading auto.conf with the S_DEF_AUTO flag, which is expected to set the SYMBOL_DEF_AUTO flags to symbols read from auto.conf.
This is then used by conf_split_config to detect symbols that have changed.

Ah, _that_ is why it's there.

[...]

Quoted 8 lines

The two following tests do not invlolve the HOTPLUG_PCI_.* symbols on purpose, and involve a pristine master c095ba7:
    $ make allmodconfig
    $ make silentoldconfig
    $ make silentoldconfig
    $ make silentoldconfig

Quoted 7 lines

- Without Michal's proposed change, unaffected symbols are not touched. For example, mtime and ctime for zswap.h are not changed after the second and third silentoldconfig.
- With Michal's proposed change, then mtime and ctime for zswap.h are changed after each silentoldconfig, even though CONFIG_ZSWAP was not changed.

Yes, I only did a simple diff -rq between two generated include/config directories and did not think about mtimes.

Quoted 2 lines

One solution would be to read auto.conf *before* we fuzz around symbols, not after. We should probably read auto.conf just after we read .config.

Maybe the safest solution would be to silence the warning if def == S_DEF_AUTO. Because auto.conf is only read by kconfig in conf_split_config(), when it is expected to be out of date.

Michal