[GIT PULL] PCI updates for v3.11

[GIT PULL] PCI updates for v3.11

[Bjorn Helgaas]

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(-)

Re: [GIT PULL] PCI updates for v3.11

[Linus Torvalds]

On Fri, Aug 2, 2013 at 11:17 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> 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

Re: [GIT PULL] PCI updates for v3.11

[Linus Torvalds]

On Fri, Aug 2, 2013 at 1:28 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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

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

[Michal Marek]

On Fri, Aug 02, 2013 at 01:28:25PM -0700, Linus Torvalds wrote:
> On Fri, Aug 2, 2013 at 11:17 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >
> > 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.

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?

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index c55c227..8c90835 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -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

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

[Yann E. MORIN]

Michal, All,

On Monday 05 August 2013 10:39:43 Michal Marek wrote:
> On Fri, Aug 02, 2013 at 01:28:25PM -0700, Linus Torvalds wrote:
> > On Fri, Aug 2, 2013 at 11:17 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > >
> > > 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.
> 
> 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?

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.

> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index c55c227..8c90835 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -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  | 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.  |
'------------------------------'-------'------------------'--------------------'

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

[Yann E. MORIN]

Michal, All,

On 2013-08-05 10:39 +0200, Michal Marek spake thusly:
> On Fri, Aug 02, 2013 at 01:28:25PM -0700, Linus Torvalds wrote:
> > On Fri, Aug 2, 2013 at 11:17 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > >
> > > 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.
> 
> 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?
> 
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index c55c227..8c90835 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -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;
>  

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.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

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

[Michal Marek]

On 6.8.2013 00:58, Yann E. MORIN wrote:
> On 2013-08-05 10:39 +0200, Michal Marek spake thusly:
>> 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?
>>
>> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
>> index c55c227..8c90835 100644
>> --- a/scripts/kconfig/confdata.c
>> +++ b/scripts/kconfig/confdata.c
>> @@ -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;
[...]
> 
> 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.

[...]
> 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.

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


> 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