* Re: Build regressions/improvements in v3.10-rc5
From: Anca Emanuel @ 2013-06-10 14:37 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Linux/PPC Development, Linux Kernel Development
In-Reply-To: <alpine.DEB.2.00.1306100924570.22024@ayla.of.borg>
The relevant maintainers do not get a copy of this.
On Mon, Jun 10, 2013 at 10:25 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, 10 Jun 2013, Geert Uytterhoeven wrote:
>> JFYI, when comparing v3.10-rc5 to v3.10-rc4[3], the summaries are:
>> - build errors: +19/-10
>> [1] http://kisskb.ellerman.id.au/kisskb/head/6308/ (all 120 configs)
>
> + arch/powerpc/kernel/cacheinfo.c: error: 'associativity' may be used uninitialized in this function [-Werror=uninitialized]: => 571:16
> + arch/powerpc/kernel/cacheinfo.c: error: 'size_kb' may be used uninitialized in this function [-Werror=uninitialized]: => 522:16
> + arch/powerpc/kernel/pci_dn.c: error: 'ret' may be used uninitialized in this function [-Werror=uninitialized]: => 97:8
> + arch/powerpc/kvm/book3s_32_mmu.c: error: 'sr' may be used uninitialized in this function [-Werror=uninitialized]: => 63:2
> + arch/powerpc/kvm/book3s_64_mmu.c: error: 'slb' may be used uninitialized in this function [-Werror=uninitialized]: => 485:3
> + arch/powerpc/mm/hash_native_64.c: error: 'a_size' may be used uninitialized in this function [-Werror=uninitialized]: => 67:31
> + arch/powerpc/platforms/cell/spufs/inode.c: error: incompatible types when assigning to type 'kgid_t' from type 'int': => 628:16
> + arch/powerpc/platforms/cell/spufs/inode.c: error: incompatible types when assigning to type 'kuid_t' from type 'int': => 623:16
> + arch/powerpc/platforms/pseries/msi.c: error: 'total' may be used uninitialized in this function [-Werror=uninitialized]: => 320:24
> + arch/powerpc/platforms/pseries/pci.c: error: 'pcie_link_speed_stats' may be used uninitialized in this function [-Werror=uninitialized]: => 150:31
>
> powerpc-randconfig
>
>> [3] http://kisskb.ellerman.id.au/kisskb/head/6290/ (all 120 configs)
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: Build regressions/improvements in v3.10-rc5
From: Geert Uytterhoeven @ 2013-06-10 17:00 UTC (permalink / raw)
To: Anca Emanuel; +Cc: Linux/PPC Development, Linux Kernel Development
In-Reply-To: <CAJL_dMtUXSCe3Bm=D4aYaX9NwhnaPjVsY4O+f8U9h5-NV9i0=A@mail.gmail.com>
On Mon, Jun 10, 2013 at 4:37 PM, Anca Emanuel <anca.emanuel@gmail.com> wrote:
> The relevant maintainers do not get a copy of this.
I do hope the PPC maintainers do read linuxppc-dev@lists.ozlabs.org.
> On Mon, Jun 10, 2013 at 10:25 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Mon, 10 Jun 2013, Geert Uytterhoeven wrote:
>>> JFYI, when comparing v3.10-rc5 to v3.10-rc4[3], the summaries are:
>>> - build errors: +19/-10
>>> [1] http://kisskb.ellerman.id.au/kisskb/head/6308/ (all 120 configs)
>>
>> + arch/powerpc/kernel/cacheinfo.c: error: 'associativity' may be used uninitialized in this function [-Werror=uninitialized]: => 571:16
>> + arch/powerpc/kernel/cacheinfo.c: error: 'size_kb' may be used uninitialized in this function [-Werror=uninitialized]: => 522:16
>> + arch/powerpc/kernel/pci_dn.c: error: 'ret' may be used uninitialized in this function [-Werror=uninitialized]: => 97:8
>> + arch/powerpc/kvm/book3s_32_mmu.c: error: 'sr' may be used uninitialized in this function [-Werror=uninitialized]: => 63:2
>> + arch/powerpc/kvm/book3s_64_mmu.c: error: 'slb' may be used uninitialized in this function [-Werror=uninitialized]: => 485:3
>> + arch/powerpc/mm/hash_native_64.c: error: 'a_size' may be used uninitialized in this function [-Werror=uninitialized]: => 67:31
>> + arch/powerpc/platforms/cell/spufs/inode.c: error: incompatible types when assigning to type 'kgid_t' from type 'int': => 628:16
>> + arch/powerpc/platforms/cell/spufs/inode.c: error: incompatible types when assigning to type 'kuid_t' from type 'int': => 623:16
>> + arch/powerpc/platforms/pseries/msi.c: error: 'total' may be used uninitialized in this function [-Werror=uninitialized]: => 320:24
>> + arch/powerpc/platforms/pseries/pci.c: error: 'pcie_link_speed_stats' may be used uninitialized in this function [-Werror=uninitialized]: => 150:31
>>
>> powerpc-randconfig
>>
>>> [3] http://kisskb.ellerman.id.au/kisskb/head/6290/ (all 120 configs)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [BUG] PCI related panic on powerpc based board with 3.10-rcX
From: Michael Guntsche @ 2013-06-10 17:07 UTC (permalink / raw)
To: Rojhalat Ibrahim; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <7927838.NMrLdRWptT@pcimr>
Good evening,
On Mon, Jun 10, 2013 at 1:41 PM, Rojhalat Ibrahim <imr@rtschenk.de> wrote:
> Hi Mike,
>
> could you please try this patch:
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-May/106624.html
> http://patchwork.ozlabs.org/patch/244515/
>
> Rojhalat
>
>
> On Saturday 08 June 2013 21:39:37 Michael Guntsche wrote:
>> After bisecting I found the responsible commit.
>>
>> 50d8f87d2b3: powerpc/fsl-pci Make PCIe hotplug work with Freescale
>> PCIe controllers
>>
>> Reverting this commit allowed my board to boot again.
>>
>> @Rojhalat: Please have a look at
>> http://marc.info/?l=linux-kernel&m=137071294204858&w=2
>> for my initial bugreport.
>>
>> What I do not understand at all is why this is affecting my platform.
>> AFAIK there is no PCIe hardware on it AND I completely disabled PCIe
>> support in config.
>>
>> Kind regards,
>> Mike
This patch does not fix the problem, during boot the kernel still
panics. I had a closer look at the commit and the following patch
fixes it for me....
diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 028ac1f..21b687f 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -814,7 +814,7 @@ int __init mpc83xx_add_bridge(struct device_node *dev)
if (ret)
goto err0;
} else {
- fsl_setup_indirect_pci(hose, rsrc_cfg.start,
+ setup_indirect_pci(hose, rsrc_cfg.start,
rsrc_cfg.start + 4, 0);
}
Apparently the mpc83xx platform code goes through great lengths to
guard the call to fsl_pcie_check_link on NON PCIe platforms, since
apparently this seems to cause the panic. Furthermore it also has its
own PCIe setup code. With this patch applied the system boots fine for
me.
The only problem I have with this patch is that the compiler fails
since -Werror is used and fsl_setup_indirect_pci now is never used.
For testing purposes I removed the function definition. Obviously the
proper fix would be to wrap it in an #ifdef but I did not know the
proper define to check against.
Kind regards,
Mike
^ permalink raw reply related
* [PATCH v2] powerpc/pci: Improve device hotplug initialization
From: Guenter Roeck @ 2013-06-10 17:18 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Yuanquan Chen, linuxppc-dev, linux-kernel, Guenter Roeck,
Hiroo Matsumoto
Commit 37f02195b (powerpc/pci: fix PCI-e devices rescan issue on powerpc
platform) fixes a problem with interrupt and DMA initialization on hot
plugged devices. With this commit, interrupt and DMA initialization for
hot plugged devices is handled in the pci device enable function.
This approach has a couple of drawbacks. First, it creates two code paths
for device initialization, one for hot plugged devices and another for devices
known during the initial PCI scan. Second, the initialization code for hot
plugged devices is only called when the device is enabled, ie typically
in the probe function. Also, the platform specific setup code is called each
time pci_enable_device() is called, not only once during device discovery,
meaning it is actually called multiple times, once for devices discovered
during the initial scan and again each time a driver is re-loaded.
The visible result is that interrupt pins are only assigned to hot plugged
devices when the device driver is loaded. Effectively this changes the PCI
probe API, since pci_dev->irq and the device's dma configuration will now
only be valid after pci_enable() was called at least once. A more subtle
change is that platform specific PCI device setup is moved from device
discovery into the driver's probe function, more specifically into the
pci_enable_device() call.
To fix the inconsistencies, add new function pcibios_add_device.
Call pcibios_setup_device from pcibios_setup_bus_devices if device setup
is not complete, and from pcibios_add_device if bus setup is complete.
With this change, device setup code is moved back into device initialization,
and called exactly once for both static and hot plugged devices.
Cc: Yuanquan Chen <Yuanquan.Chen@freescale.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Hiroo Matsumoto <matsumoto.hiroo@jp.fujitsu.com>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Ensure that PCI bus fixup code has been executed before calling
device setup code.
arch/powerpc/kernel/pci-common.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index 7f2273c..6909b13 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -992,7 +992,7 @@ void pcibios_setup_bus_self(struct pci_bus *bus)
ppc_md.pci_dma_bus_setup(bus);
}
-void pcibios_setup_device(struct pci_dev *dev)
+static void pcibios_setup_device(struct pci_dev *dev)
{
/* Fixup NUMA node as it may not be setup yet by the generic
* code and is needed by the DMA init
@@ -1013,6 +1013,17 @@ void pcibios_setup_device(struct pci_dev *dev)
ppc_md.pci_irq_fixup(dev);
}
+int pcibios_add_device(struct pci_dev *dev)
+{
+ /*
+ * We can only call pcibios_setup_device() after bus setup is complete,
+ * since some of the platform specific DMA setup code depends on it.
+ */
+ if (dev->bus->is_added)
+ pcibios_setup_device(dev);
+ return 0;
+}
+
void pcibios_setup_bus_devices(struct pci_bus *bus)
{
struct pci_dev *dev;
@@ -1467,10 +1478,6 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
if (ppc_md.pcibios_enable_device_hook(dev))
return -EINVAL;
- /* avoid pcie irq fix up impact on cardbus */
- if (dev->hdr_type != PCI_HEADER_TYPE_CARDBUS)
- pcibios_setup_device(dev);
-
return pci_enable_resources(dev, mask);
}
--
1.7.9.7
^ permalink raw reply related
* Re: [PATCH 1/2] perf/Power7: Save dcache_src fields in sample record.
From: Stephane Eranian @ 2013-06-10 19:34 UTC (permalink / raw)
To: Sukadev Bhattiprolu
Cc: Anton Blanchard, LKML, Linux PPC dev, Paul Mackerras, Ingo Molnar
In-Reply-To: <20130607204008.GA3281@us.ibm.com>
On Fri, Jun 7, 2013 at 10:40 PM, Sukadev Bhattiprolu
<sukadev@linux.vnet.ibm.com> wrote:
>
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Wed, 8 May 2013 22:59:29 -0700
> Subject: [PATCH 1/2] perf/Power7: Save dcache_src fields in sample record.
>
> Power7 saves the "perf-event vector" information in the mmcra register.
> Included in this event vector is a "data-cache source" field which
> identifies where in the memory-hierarchy the data for an instruction
> was found.
>
> Use the 'struct perf_mem_data_source' to export the "data-cache source"
> field to user space.
>
> The mapping between the Power7 hierarchy levels and the arch-neutral
> levels is, unfortunately, not trivial.
>
> Arch-neutral levels Power7 levels
> ---------------------------------------------------------
> local LVL_L2 local (same core) L2 (FROM_L2)
> local LVL_L3 local (same core) L3 (FROM_L3)
>
> 1-hop REM_CCE1 different core on same chip (FROM_L2.1, _L3.1)
> 2-hops REM_CCE2 remote (different chip, same node) (FROM_RL2L3)
> 3-hops REM_CCE3* distant (different node) (FROM_DL2L3)
>
> 1-hop REM_MEM1 unused
> 2-hops REM_MEM2 remote (different chip, same node) (FROM_RMEM)
> 3-hops REM_MEM3* distant (different node) (FROM_DMEM)
>
> * proposed "extended" levels.
>
> AFAICT, Power7 supports one extra level in the cache-hierarchy, so we propose
> to add a new cache level, REM_CCE3 shown above.
>
> To maintain consistency in terminology (i.e 2-hops = remote, 3-hops = distant),
> I propose leaving the REM_MEM1 unused and adding another level, REM_MEM3.
>
> Further, in the above REM_CCE1 case, Power7 can also identify if the data came
> from the L2 or L3 cache of another core on the same chip. To describe this to
> user space, we propose to set ->mem_lvl to:
>
> PERF_MEM_LVL_REM_CCE1|PERF_MEM_LVL_L2
>
> PERF_MEM_LVL_REM_CCE1|PERF_MEM_LVL_L3
Normally, that would be interpreted as:
- hit/miss on remote cache (1 hop) OR level 2 cache
But on PPC7, you're saying that this must be interpreted as:
- hit/miss on L2 cache of sibling core
How do you intend to document this interpretation?
>
>
> Either that or we could leave REM_CCE1 unused in Power and add two more levels:
>
> PERF_MEM_XLVL_REM_L2_CCE1
> PERF_MEM_XLVL_REM_L3_CCE1
>
> The former approach seems less confusing and this patch uses that approach.
>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/perf_event_server.h | 2 +
> arch/powerpc/perf/core-book3s.c | 4 +
> arch/powerpc/perf/power7-pmu.c | 81 ++++++++++++++++++++++++++
> include/uapi/linux/perf_event.h | 12 +++-
> 4 files changed, 97 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/perf_event_server.h b/arch/powerpc/include/asm/perf_event_server.h
> index f265049..f2d162b 100644
> --- a/arch/powerpc/include/asm/perf_event_server.h
> +++ b/arch/powerpc/include/asm/perf_event_server.h
> @@ -37,6 +37,8 @@ struct power_pmu {
> void (*config_bhrb)(u64 pmu_bhrb_filter);
> void (*disable_pmc)(unsigned int pmc, unsigned long mmcr[]);
> int (*limited_pmc_event)(u64 event_id);
> + void (*get_mem_data_src)(struct perf_sample_data *data,
> + struct pt_regs *regs);
> u32 flags;
> const struct attribute_group **attr_groups;
> int n_generic;
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index 426180b..7778fa9 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -1632,6 +1632,10 @@ static void record_and_restart(struct perf_event *event, unsigned long val,
> data.br_stack = &cpuhw->bhrb_stack;
> }
>
> + if (event->attr.sample_type & PERF_SAMPLE_DATA_SRC &&
> + ppmu->get_mem_data_src)
> + ppmu->get_mem_data_src(&data, regs);
> +
> if (perf_event_overflow(event, &data, regs))
> power_pmu_stop(event, 0);
> }
> diff --git a/arch/powerpc/perf/power7-pmu.c b/arch/powerpc/perf/power7-pmu.c
> index 3c475d6..af92bfe 100644
> --- a/arch/powerpc/perf/power7-pmu.c
> +++ b/arch/powerpc/perf/power7-pmu.c
> @@ -209,6 +209,85 @@ static int power7_get_alternatives(u64 event, unsigned int flags, u64 alt[])
> return nalt;
> }
>
> +#define POWER7_MMCRA_PEMPTY (0x1L << 63)
> +#define POWER7_MMCRA_FIN_STALL (0x1L << 62)
> +#define POWER7_MMCRA_CMPL_STALL (0x1L << 61)
> +#define POWER7_MMCRA_STALL_REASON_MASK (0xFL << 60)
> +
> +#define POWER7_MMCRA_DCACHE_MISS (0x1L << 55)
> +
> +#define POWER7_MMCRA_DCACHE_SRC_SHIFT 51
> +#define POWER7_MMCRA_DCACHE_SRC_MASK (0xFL << POWER7_MMCRA_DCACHE_SRC_SHIFT)
> +
> +#define POWER7_MMCRA_MDTLB_MISS (0x1L << 50)
> +
> +#define POWER7_MMCRA_MDTLB_SRC_SHIFT 46
> +#define POWER7_MMCRA_MDTLB_SRC_MASK (0xFL << POWER7_MMCRA_MDTLB_SRC_SHIFT)
> +
> +#define POWER7_MMCRA_MDERAT_MISS (0x1L<< 45)
> +#define POWER7_MMCRA_MLSU_REJ (0x1L<< 44)
> +
> +/* and so on */
> +
> +/*
> + * Map DCACHE_SRC fields to the Linux memory hierarchy levels.
> + *
> + * Bits 9..12 in the MMCRA indicate the source of a data-cache entry, with
> + * each of the 16 possible values referring to a specific source. Eg: if
> + * the 4-bits have the value 1 (0b0001), the dcache entry was found local
> + * L3 cache.
> + *
> + * We use the table, dcache_src_map, to map this value 1 to PERF_MEM_LVL_L3,
> + * the arch-neutral representation of the L3 cache.
> + *
> + * Similarly, in case of marked data TLB miss, bits 14..17 of the MMCRA
> + * indicate the load source of a marked DTLB entry. dtlb_src_map[] gives
> + * the mapping to the arch-neutral values of the TLB source.
> + *
> + * Architecture neutral to Power7 hierarchy levels:
> + * 1-hop = different core on same chip (L2.1 or L3.1)
> + * 2-hops = remote (different chip on same node)
> + * 3-hops = distant (different node)
> + */
> +static u64 dcache_src_map[] = {
> + PERF_MEM_S(LVL, L2), /* 00: FROM_L2 */
> + PERF_MEM_S(LVL, L3), /* 01: FROM_L3 */
> + PERF_MEM_S(LVL, NA), /* 02: Reserved */
> + PERF_MEM_S(LVL, NA), /* 03: Reserved */
> +
> + PERF_MEM_LVL_L2|PERF_MEM_LVL_REM_CCE1, /* 04: FROM_L2.1_SHR */
> + PERF_MEM_LVL_L2|PERF_MEM_LVL_REM_CCE1, /* 05: FROM_L3.1_MOD */
> + PERF_MEM_LVL_L3|PERF_MEM_LVL_REM_CCE1, /* 06: FROM_L2.1_SHR */
> + PERF_MEM_LVL_L3|PERF_MEM_LVL_REM_CCE1, /* 07: FROM_L3.1_MOD */
> +
> + PERF_MEM_S(LVL, REM_CCE2), /* 08: FROM_RL2L3_SHR */
> + PERF_MEM_S(LVL, REM_CCE2), /* 09: FROM_RL2L3_MOD */
> + PERF_MEM_S(XLVL, REM_CCE3), /* 10: FROM_DL2L3_SHR */
> + PERF_MEM_S(XLVL, REM_CCE3), /* 11: FROM_DL2L3_MOD */
> +
> + PERF_MEM_S(LVL, LOC_RAM), /* 12: FROM_LMEM */
> + PERF_MEM_S(LVL, REM_RAM2), /* 13: FROM_RMEM */
> + PERF_MEM_S(XLVL, REM_RAM3), /* 14: FROM_DMEM */
> +
> + PERF_MEM_S(LVL, NA), /* 15: Reserved */
> +};
> +
> +
> +static void power7_get_mem_data_src(struct perf_sample_data *data,
> + struct pt_regs *regs)
> +{
> + unsigned long idx;
> + unsigned long mmcra = regs->dsisr;
> + union perf_mem_data_src *dsrc = &data->data_src;
> +
> + if (mmcra & POWER7_MMCRA_DCACHE_MISS) {
> + idx = mmcra & POWER7_MMCRA_DCACHE_SRC_MASK;
> + idx >>= POWER7_MMCRA_DCACHE_SRC_SHIFT;
> +
> + dsrc->val |= dcache_src_map[idx];
> + }
> +}
> +
> /*
> * Returns 1 if event counts things relating to marked instructions
> * and thus needs the MMCRA_SAMPLE_ENABLE bit set, or 0 if not.
> @@ -438,6 +517,7 @@ static const struct attribute_group *power7_pmu_attr_groups[] = {
> NULL,
> };
>
> +
> static struct power_pmu power7_pmu = {
> .name = "POWER7",
> .n_counter = 6,
> @@ -447,6 +527,7 @@ static struct power_pmu power7_pmu = {
> .compute_mmcr = power7_compute_mmcr,
> .get_constraint = power7_get_constraint,
> .get_alternatives = power7_get_alternatives,
> + .get_mem_data_src = power7_get_mem_data_src,
> .disable_pmc = power7_disable_pmc,
> .flags = PPMU_ALT_SIPR,
> .attr_groups = power7_pmu_attr_groups,
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index fb104e5..f8d3269 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -627,7 +627,8 @@ union perf_mem_data_src {
> mem_snoop:5, /* snoop mode */
> mem_lock:2, /* lock instr */
> mem_dtlb:7, /* tlb access */
> - mem_rsvd:31;
> + mem_xlvl:2, /* extended memory levels */
> + mem_rsvd:29;
> };
> };
>
> @@ -654,7 +655,7 @@ union perf_mem_data_src {
> #define PERF_MEM_LVL_REM_CCE2 0x800 /* Remote Cache (2 hops) */
> #define PERF_MEM_LVL_IO 0x1000 /* I/O memory */
> #define PERF_MEM_LVL_UNC 0x2000 /* Uncached memory */
> -#define PERF_MEM_LVL_SHIFT 5
> +#define PERF_MEM_LVL_SHIFT 5 /* see also extended levels below */
>
> /* snoop mode */
> #define PERF_MEM_SNOOP_NA 0x01 /* not available */
> @@ -679,6 +680,13 @@ union perf_mem_data_src {
> #define PERF_MEM_TLB_OS 0x40 /* OS fault handler */
> #define PERF_MEM_TLB_SHIFT 26
>
> +#define PERF_MEM_XLVL_REM_RAM3 0x01 /* Remote memory (3 hops) */
> +#define PERF_MEM_XLVL_REM_CCE3 0x02 /* Remote cache (3 hops) */
> +#define PERF_MEM_XLVL_SHIFT 33
> +
You need to define a N/A bit there too.
Thats' necessary to indicate not available on non PPC architectures,
such as x86.
>
> +/* Miscellaneous flags */
> +#define PERF_MEM_MISC_CCE_MOD 0x4000 /* cache-hit, but entry was modified */
> +
Where is that flag used?
If internal, then it needs to be moved to the internal-only version of
the header.
> #define PERF_MEM_S(a, s) \
> (((u64)PERF_MEM_##a##_##s) << PERF_MEM_##a##_SHIFT)
>
> --
> 1.7.1
>
^ permalink raw reply
* Re: PowerPC assembler question
From: Benjamin Herrenschmidt @ 2013-06-10 21:43 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <20130610211439.a69d2b26ee216f34caf473bd@mega-nerd.com>
On Mon, 2013-06-10 at 21:14 +1000, Erik de Castro Lopo wrote:
> Benjamin Herrenschmidt wrote:
>
> > No, this loads a 32-bit value (16-bit would be lhz).
>
> My understanding so far (which may be wrong) is that it loads a
> 32 bit value but it loads it from a memory location that needs
> to be within +/- 32k of the instriction doing the load.
No, within that distance of the base register. IE. The offset field is
the one that is 16-bit (it's encoded in the instruction).
> The reason I think this is because when this generated code is
> compiled I get the error:
>
> /tmp/ghc2806_0/ghc2806_1.s:51766:0:
> Error: operand out of range (0x000000000000adf8 is not between
> 0xffffffffffff8000 and 0x0000000000007fff)
>
> which suggests a 16 bit offset.
>
> Btw, this is code generated by the Glasgow Haskell Compiler (GHC). The
> GHC bug is here:
>
> http://hackage.haskell.org/trac/ghc/ticket/7830
>
>
> > Note: It's more readable if you use the register names, ie:
> >
> > lwz %r30, .label - (1b)(%r31)
> >
> > The form of lwz is
> >
> > lwz dest_reg, offset(address_reg)
> >
> > So it will load a 32-bit value from memory at the address contained in
> > r31 offset by ".label - 1b" which is itself the difference between
> > two labels, "label", and the first "1:" label before the instruction
> >
> > (gcc supports numeric labels that can be referenced with the suffix "b"
> > for backward and "f" for forward which are handy for small
> > displacements)
>
> Ahh, that would be +/- 32k!
Yes. If you need more, then you can use a different form such as lwzx
which takes two registers for the address and adds them (beware that
the instruction is of the form rA||0 which means that if the "offset"
register is r0 it will use the value "0" instead.
> > So for example if 1: was the base of the structure and .label a field
> > in the structure, it would load the 32-bit value of that field for the
> > structure instance starting at %r31.
> >
> > In this case, this looks more like some kind of position-independent
> > code though.
>
> That would definitely make sense.
>
> Is there something I could replace this above lwz instruction with
> that would work for PIC with offsets greater than +/- 32k?
As I said above, you'd have to load another register with the offset
and use that, which would be something like 3 instructions instead of
one (and a register spill).
If the offset can be known at "generation" time (it should since it's
label differences but it depends how the compiler works), you might be
able to pick the most efficient instruction based on the value of
the offset.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/2] perf/Power7: Save dcache_src fields in sample record.
From: Sukadev Bhattiprolu @ 2013-06-10 21:48 UTC (permalink / raw)
To: Anshuman Khandual
Cc: Anton Blanchard, linux-kernel, Stephane Eranian, linuxppc-dev,
Paul Mackerras, mingo
In-Reply-To: <51B58843.5020809@linux.vnet.ibm.com>
Anshuman Khandual [khandual@linux.vnet.ibm.com] wrote:
| > The former approach seems less confusing and this patch uses that approach.
| >
|
| Yeah, the former approach is simpler and makes sense.
Ok. Seems to make sense at least on Power.
<snip>
| > + * We use the table, dcache_src_map, to map this value 1 to PERF_MEM_LVL_L3,
| > + * the arch-neutral representation of the L3 cache.
| > + *
| > + * Similarly, in case of marked data TLB miss, bits 14..17 of the MMCRA
| > + * indicate the load source of a marked DTLB entry. dtlb_src_map[] gives
| > + * the mapping to the arch-neutral values of the TLB source.
|
|
| Where did you define dtlb_src_map[] ?
Ah, the comment belongs in another patch that I am working on. That patch
maps the PERF_MEM_TLB* flags to Power7.
Thanks for the comments.
Sukadev
^ permalink raw reply
* Re: [BUG] PCI related panic on powerpc based board with 3.10-rcX
From: Scott Wood @ 2013-06-10 22:52 UTC (permalink / raw)
To: Michael Guntsche; +Cc: linuxppc-dev, Rojhalat Ibrahim, linux-kernel
In-Reply-To: <CALG0vJtjS2wP_s=-iyG1ABKw_ct=B9OdNXY9YCrnkhAnpaN1Zw@mail.gmail.com>
On 06/10/2013 12:07:43 PM, Michael Guntsche wrote:
> Good evening,
>=20
> On Mon, Jun 10, 2013 at 1:41 PM, Rojhalat Ibrahim <imr@rtschenk.de> =20
> wrote:
> > Hi Mike,
> >
> > could you please try this patch:
> > https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-May/106624.html
> > http://patchwork.ozlabs.org/patch/244515/
> >
> > Rojhalat
> >
> >
> > On Saturday 08 June 2013 21:39:37 Michael Guntsche wrote:
> >> After bisecting I found the responsible commit.
> >>
> >> 50d8f87d2b3: powerpc/fsl-pci Make PCIe hotplug work with Freescale
> >> PCIe controllers
> >>
> >> Reverting this commit allowed my board to boot again.
> >>
> >> @Rojhalat: Please have a look at
> >> http://marc.info/?l=3Dlinux-kernel&m=3D137071294204858&w=3D2
> >> for my initial bugreport.
> >>
> >> What I do not understand at all is why this is affecting my =20
> platform.
> >> AFAIK there is no PCIe hardware on it AND I completely disabled =20
> PCIe
> >> support in config.
> >>
> >> Kind regards,
> >> Mike
>=20
> This patch does not fix the problem, during boot the kernel still
> panics. I had a closer look at the commit and the following patch
> fixes it for me....
>=20
> diff --git a/arch/powerpc/sysdev/fsl_pci.c =20
> b/arch/powerpc/sysdev/fsl_pci.c
> index 028ac1f..21b687f 100644
> --- a/arch/powerpc/sysdev/fsl_pci.c
> +++ b/arch/powerpc/sysdev/fsl_pci.c
> @@ -814,7 +814,7 @@ int __init mpc83xx_add_bridge(struct device_node =20
> *dev)
> if (ret)
> goto err0;
> } else {
> - fsl_setup_indirect_pci(hose, rsrc_cfg.start,
> + setup_indirect_pci(hose, rsrc_cfg.start,
> rsrc_cfg.start + 4, 0);
> }
The only difference here is that you're not setting hose->ops to =20
fsl_indirect_pci_ops. Do you know why that is helping, and what =20
hose->ops is set to instead?
-Scott=
^ permalink raw reply
* Re: [PATCH 1/2] perf/Power7: Save dcache_src fields in sample record.
From: Sukadev Bhattiprolu @ 2013-06-10 23:08 UTC (permalink / raw)
To: Stephane Eranian
Cc: Anton Blanchard, LKML, Linux PPC dev, Paul Mackerras, Ingo Molnar
In-Reply-To: <CABPqkBTqt5cdD9AHvvzzqcMnvpiQ8iH=3cq0=Y8Edp09KiRPEA@mail.gmail.com>
Stephane Eranian [eranian@google.com] wrote:
| > Further, in the above REM_CCE1 case, Power7 can also identify if the data came
| > from the L2 or L3 cache of another core on the same chip. To describe this to
| > user space, we propose to set ->mem_lvl to:
| >
| > PERF_MEM_LVL_REM_CCE1|PERF_MEM_LVL_L2
| >
| > PERF_MEM_LVL_REM_CCE1|PERF_MEM_LVL_L3
|
|
| Normally, that would be interpreted as:
| - hit/miss on remote cache (1 hop) OR level 2 cache
|
| But on PPC7, you're saying that this must be interpreted as:
| - hit/miss on L2 cache of sibling core
Hmm, my proposed usage is an AND.
Did not realize it was strictly an OR of the levels. If so, we will have
to define and use the extra levels I guess.
BTW, are there architectures that use the OR interpretation - IOW, are
arbitrary combinations like local L1 or a remote 2-hop node's cache used ?
|
| How do you intend to document this interpretation?
Not sure yet as this is an early patch. 'perf report' man page would be
one place.
Do/should architectures have the flexibility of interpretation ?
Personally, if we cannot interpret them as the AND of two levels, I think
we would be better off defining the new levels below.
|
| >
| >
| > Either that or we could leave REM_CCE1 unused in Power and add two more levels:
| >
| > PERF_MEM_XLVL_REM_L2_CCE1
| > PERF_MEM_XLVL_REM_L3_CCE1
| >
| > The former approach seems less confusing and this patch uses that approach.
<snip>
| > @@ -654,7 +655,7 @@ union perf_mem_data_src {
| > #define PERF_MEM_LVL_REM_CCE2 0x800 /* Remote Cache (2 hops) */
| > #define PERF_MEM_LVL_IO 0x1000 /* I/O memory */
| > #define PERF_MEM_LVL_UNC 0x2000 /* Uncached memory */
| > -#define PERF_MEM_LVL_SHIFT 5
| > +#define PERF_MEM_LVL_SHIFT 5 /* see also extended levels below */
| >
| > /* snoop mode */
| > #define PERF_MEM_SNOOP_NA 0x01 /* not available */
| > @@ -679,6 +680,13 @@ union perf_mem_data_src {
| > #define PERF_MEM_TLB_OS 0x40 /* OS fault handler */
| > #define PERF_MEM_TLB_SHIFT 26
| >
| > +#define PERF_MEM_XLVL_REM_RAM3 0x01 /* Remote memory (3 hops) */
| > +#define PERF_MEM_XLVL_REM_CCE3 0x02 /* Remote cache (3 hops) */
| > +#define PERF_MEM_XLVL_SHIFT 33
| > +
|
| You need to define a N/A bit there too.
| Thats' necessary to indicate not available on non PPC architectures,
| such as x86.
Ok.
|
| >
| > +/* Miscellaneous flags */
| > +#define PERF_MEM_MISC_CCE_MOD 0x4000 /* cache-hit, but entry was modified */
| > +
| Where is that flag used?
| If internal, then it needs to be moved to the internal-only version of
| the header.
It is not internal, but the line snuck in when I was splitting a patch.
It refers to another feature in Power7 that I was trying to map into the
perf_mem_data_src hierarchy. Power7 also indicates whether the entry we
found in the cache was modified or shared.
Like with the HIT or MISS, it would/could be another state associated with
each of the levels:
PERF_MEM_LVL_REM_CCE1
PERF_MEM_LVL_REM_CCE2
PERF_MEM_XLVL_REM_CCE3
I was toying with the idea of setting
->mem_level = LVL_REM_CCE1|LVL_L2;
->mem_misc = PERF_MEM_MISC_CCE_MOD;
to say that the cache entry we found in the sibling's L2 core was modified/dirty.
(where ->mem_misc is carved out of the ->mem_rsvd bits).
Will come back to it after addressing the XLVL* part.
Thanks for the comments.
Sukadev
^ permalink raw reply
* Re: [RFC PATCH powerpc] Set cpu sibling mask before online cpu
From: Benjamin Herrenschmidt @ 2013-06-11 7:00 UTC (permalink / raw)
To: Li Zhong; +Cc: Paul Mackerras, PowerPC email list
In-Reply-To: <1368699626.2618.183.camel@ThinkPad-T5421>
On Thu, 2013-05-16 at 18:20 +0800, Li Zhong wrote:
> It seems following race is possible:
>
.../...
> vdso_getcpu_init();
> #endif
> - notify_cpu_starting(cpu);
> - set_cpu_online(cpu, true);
> /* Update sibling maps */
> base = cpu_first_thread_sibling(cpu);
> for (i = 0; i < threads_per_core; i++) {
> - if (cpu_is_offline(base + i))
> + if (cpu_is_offline(base + i) && (cpu != base + i))
> continue;
> cpumask_set_cpu(cpu, cpu_sibling_mask(base + i));
> cpumask_set_cpu(base + i, cpu_sibling_mask(cpu));
> @@ -667,6 +665,10 @@ __cpuinit void start_secondary(void *unused)
> }
> of_node_put(l2_cache);
>
> + smp_wmb();
> + notify_cpu_starting(cpu);
> + set_cpu_online(cpu, true);
> +
So we could have an online CPU with an empty sibling mask. Now we can
have a sibling that isn't online ... Is that ok ?
Or should we do a two pass mechanism:
- Pass 1, set the new cpu siblings
- set_cpu_online
- Pass 2, set other CPU sibling of this cpu
?
Cheers,
Ben.
^ permalink raw reply
* Re: [BUG] PCI related panic on powerpc based board with 3.10-rcX
From: Rojhalat Ibrahim @ 2013-06-11 7:24 UTC (permalink / raw)
To: Scott Wood, Michael Guntsche; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1370904753.18413.19@snotra>
On Monday 10 June 2013 17:52:33 Scott Wood wrote:
> On 06/10/2013 12:07:43 PM, Michael Guntsche wrote:
> > Good evening,
> >
> > On Mon, Jun 10, 2013 at 1:41 PM, Rojhalat Ibrahim <imr@rtschenk.de>
> >
> > wrote:
> > > Hi Mike,
> > >
> > > could you please try this patch:
> > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2013-May/106624.html
> > > http://patchwork.ozlabs.org/patch/244515/
> > >
> > > Rojhalat
> > >
> > > On Saturday 08 June 2013 21:39:37 Michael Guntsche wrote:
> > >> After bisecting I found the responsible commit.
> > >>
> > >> 50d8f87d2b3: powerpc/fsl-pci Make PCIe hotplug work with Freescale
> > >> PCIe controllers
> > >>
> > >> Reverting this commit allowed my board to boot again.
> > >>
> > >> @Rojhalat: Please have a look at
> > >> http://marc.info/?l=linux-kernel&m=137071294204858&w=2
> > >> for my initial bugreport.
> > >>
> > >> What I do not understand at all is why this is affecting my
> >
> > platform.
> >
> > >> AFAIK there is no PCIe hardware on it AND I completely disabled
> >
> > PCIe
> >
> > >> support in config.
> > >>
> > >> Kind regards,
> > >> Mike
> >
> > This patch does not fix the problem, during boot the kernel still
> > panics. I had a closer look at the commit and the following patch
> > fixes it for me....
> >
> > diff --git a/arch/powerpc/sysdev/fsl_pci.c
> > b/arch/powerpc/sysdev/fsl_pci.c
> > index 028ac1f..21b687f 100644
> > --- a/arch/powerpc/sysdev/fsl_pci.c
> > +++ b/arch/powerpc/sysdev/fsl_pci.c
> > @@ -814,7 +814,7 @@ int __init mpc83xx_add_bridge(struct device_node
> > *dev)
> >
> > if (ret)
> >
> > goto err0;
> >
> > } else {
> >
> > - fsl_setup_indirect_pci(hose, rsrc_cfg.start,
> > + setup_indirect_pci(hose, rsrc_cfg.start,
> >
> > rsrc_cfg.start + 4, 0);
> >
> > }
>
> The only difference here is that you're not setting hose->ops to
> fsl_indirect_pci_ops. Do you know why that is helping, and what
> hose->ops is set to instead?
>
> -Scott
The difference is only the read function in hose->ops, which is set to
indirect_read_config instead of fsl_indirect_read_config.
fsl_indirect_read_config calls fsl_pcie_check_link, which is where the Oops
occurs.
Mike, can you find out where exactly in fsl_pcie_check_link the bad access
happens? Enabling CONFIG_DEBUG_BUGVERBOSE might help.
Rojhalat
^ permalink raw reply
* Re: [PATCH 15/27] powerpc/eeh: I/O chip EEH state retrieval
From: Benjamin Herrenschmidt @ 2013-06-11 7:37 UTC (permalink / raw)
To: Gavin Shan; +Cc: linuxppc-dev
In-Reply-To: <1370417668-16832-16-git-send-email-shangw@linux.vnet.ibm.com>
On Wed, 2013-06-05 at 15:34 +0800, Gavin Shan wrote:
> The patch adds I/O chip backend to retrieve the state for the
> indicated PE. While the PE state is temperarily unavailable,
> we return the default wait time (1000ms).
>
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
> arch/powerpc/platforms/powernv/eeh-ioda.c | 102 ++++++++++++++++++++++++++++-
> 1 files changed, 101 insertions(+), 1 deletions(-)
>
> diff --git a/arch/powerpc/platforms/powernv/eeh-ioda.c b/arch/powerpc/platforms/powernv/eeh-ioda.c
> index e24622e..3c72321 100644
> --- a/arch/powerpc/platforms/powernv/eeh-ioda.c
> +++ b/arch/powerpc/platforms/powernv/eeh-ioda.c
> @@ -125,10 +125,110 @@ static int ioda_eeh_set_option(struct eeh_pe *pe, int option)
> return ret;
> }
>
> +/**
> + * ioda_eeh_get_state - Retrieve the state of PE
> + * @pe: EEH PE
> + * @state: return value
> + *
> + * The PE's state should be retrieved from the PEEV, PEST
> + * IODA tables. Since the OPAL has exported the function
> + * to do it, it'd better to use that.
> + */
> +static int ioda_eeh_get_state(struct eeh_pe *pe, int *state)
> +{
So everywhere you have this "state" argument which isn't a state but a delay ...
Moreover you only initialize it in one specific case and leave it otherwise
uninitialized....
At the very least, init it to 0 by default as to not leave a dangling
"return argument" like that. However, I still have a problem with it:
> + case OPAL_EEH_STOPPED_TEMP_UNAVAIL:
> + result |= EEH_STATE_UNAVAILABLE;
> + if (state)
> + *state = 1000;
> + break;
This is the *only* case where we return anything here. Why do we bother
then and not have the upper layer simply wait one second whenever it gets
a temp unavailable result (btw, you didn't differenciate temp unavailable
from permanently unavailable in your API).
This has impacts on patch 18/27 which I'll cover here:
> +/**
> + * powernv_eeh_set_option - Initialize EEH or MMIO/DMA reenable
> + * @pe: EEH PE
> + * @option: operation to be issued
> + *
> + * The function is used to control the EEH functionality globally.
> + * Currently, following options are support according to PAPR:
> + * Enable EEH, Disable EEH, Enable MMIO and Enable DMA
> + */
> +static int powernv_eeh_set_option(struct eeh_pe *pe, int option)
> +{
> + struct pci_controller *hose = pe->phb;
> + struct pnv_phb *phb = hose->private_data;
> + int ret = -EEXIST;
> +
> + /*
> + * What we need do is pass it down for hardware
> + * implementation to handle it.
> + */
> + if (phb->eeh_ops && phb->eeh_ops->set_option)
> + ret = phb->eeh_ops->set_option(pe, option);
> +
> + return ret;
> +}
Should we implement something here ? IE. Should we look into
disabling freezing in the PHB via the firmware ? Or we just don't care ?
> +/**
> + * powernv_eeh_get_pe_addr - Retrieve PE address
> + * @pe: EEH PE
> + *
> + * Retrieve the PE address according to the given tranditional
> + * PCI BDF (Bus/Device/Function) address.
> + */
> +static int powernv_eeh_get_pe_addr(struct eeh_pe *pe)
> +{
> + return pe->addr;
> +}
>
> +/**
> + * powernv_eeh_get_state - Retrieve PE state
> + * @pe: EEH PE
> + * @state: return value
> + *
> + * Retrieve the state of the specified PE. For IODA-compitable
> + * platform, it should be retrieved from IODA table. Therefore,
> + * we prefer passing down to hardware implementation to handle
> + * it.
> + */
> +static int powernv_eeh_get_state(struct eeh_pe *pe, int *state)
> +{
> + struct pci_controller *hose = pe->phb;
> + struct pnv_phb *phb = hose->private_data;
> + int ret = EEH_STATE_NOT_SUPPORT;
> +
> + if (phb->eeh_ops && phb->eeh_ops->get_state)
> + ret = phb->eeh_ops->get_state(pe, state);
> +
> + return ret;
> +}
Same comments about "state" which is really "delay" and is probably
not necessary at all ...
> +/**
> + * powernv_eeh_reset - Reset the specified PE
> + * @pe: EEH PE
> + * @option: reset option
> + *
> + * Reset the specified PE
> + */
> +static int powernv_eeh_reset(struct eeh_pe *pe, int option)
> +{
> + struct pci_controller *hose = pe->phb;
> + struct pnv_phb *phb = hose->private_data;
> + int ret = -EEXIST;
> +
> + if (phb->eeh_ops && phb->eeh_ops->reset)
> + ret = phb->eeh_ops->reset(pe, option);
> +
> + return ret;
> +}
> +
> +/**
> + * powernv_eeh_wait_state - Wait for PE state
> + * @pe: EEH PE
> + * @max_wait: maximal period in microsecond
> + *
> + * Wait for the state of associated PE. It might take some time
> + * to retrieve the PE's state.
> + */
> +static int powernv_eeh_wait_state(struct eeh_pe *pe, int max_wait)
> +{
> + int ret;
> + int mwait;
> +
> + while (1) {
> + ret = powernv_eeh_get_state(pe, &mwait);
> +
> + /*
> + * If the PE's state is temporarily unavailable,
> + * we have to wait for the specified time. Otherwise,
> + * the PE's state will be returned immediately.
> + */
> + if (ret != EEH_STATE_UNAVAILABLE)
> + return ret;
So here we do a compare, while ret is actually a bit mask ...
In fact, ret should be named state_mask or something like that for clarity
and you should do a bit test here. Also do you want to diffenciate
permanent unavailability from temp. unavailability ?
> + max_wait -= mwait;
You decrement max_wait but never test it or use it. You probably mean to
- Limit mwait to max_wait
- If mwait is 0, return
> + msleep(mwait);
> + }
> +
> + return EEH_STATE_NOT_SUPPORT;
> +}
> +
> +/**
> + * powernv_eeh_get_log - Retrieve error log
> + * @pe: EEH PE
> + * @severity: temporary or permanent error log
> + * @drv_log: driver log to be combined with retrieved error log
> + * @len: length of driver log
> + *
> + * Retrieve the temporary or permanent error from the PE.
> + */
> +static int powernv_eeh_get_log(struct eeh_pe *pe, int severity,
> + char *drv_log, unsigned long len)
> +{
> + struct pci_controller *hose = pe->phb;
> + struct pnv_phb *phb = hose->private_data;
> + int ret = -EEXIST;
> +
> + if (phb->eeh_ops && phb->eeh_ops->get_log)
> + ret = phb->eeh_ops->get_log(pe, severity, drv_log, len);
> +
> + return ret;
> +}
> +
> +/**
> + * powernv_eeh_configure_bridge - Configure PCI bridges in the indicated PE
> + * @pe: EEH PE
> + *
> + * The function will be called to reconfigure the bridges included
> + * in the specified PE so that the mulfunctional PE would be recovered
> + * again.
> + */
> +static int powernv_eeh_configure_bridge(struct eeh_pe *pe)
> +{
> + struct pci_controller *hose = pe->phb;
> + struct pnv_phb *phb = hose->private_data;
> + int ret = 0;
> +
> + if (phb->eeh_ops && phb->eeh_ops->configure_bridge)
> + ret = phb->eeh_ops->configure_bridge(pe);
> +
> + return ret;
> +}
Ben.
^ permalink raw reply
* Re: [PATCH 17/27] powerpc/eeh: I/O chip PE log and bridge setup
From: Benjamin Herrenschmidt @ 2013-06-11 7:37 UTC (permalink / raw)
To: Gavin Shan; +Cc: linuxppc-dev
In-Reply-To: <1370417668-16832-18-git-send-email-shangw@linux.vnet.ibm.com>
On Wed, 2013-06-05 at 15:34 +0800, Gavin Shan wrote:
> The patch adds backends to retrieve error log and configure p2p
> bridges for the indicated PE.
>
> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
> +/**
> + * ioda_eeh_configure_bridge - Configure the PCI bridges for the indicated PE
> + * @pe: EEH PE
> + *
> + * For particular PE, it might have included PCI bridges. In order
> + * to make the PE work properly, those PCI bridges should be configured
> + * correctly. However, we need do nothing on P7IOC since the reset
> + * function will do everything that should be covered by the function.
> + */
> +static int ioda_eeh_configure_bridge(struct eeh_pe *pe)
> +{
> + return 0;
Does it now ?
IE. Who reconfigures the windows and other config space bits of P2P
bridges ? Or is this handled elsewhere in Linux or in the upper levels
of EEH ? Or is that only needed for the PHB ?
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH v3 00/27] EEH Support for PowerNV platform
From: Benjamin Herrenschmidt @ 2013-06-11 7:46 UTC (permalink / raw)
To: Gavin Shan; +Cc: linuxppc-dev
In-Reply-To: <1370417668-16832-1-git-send-email-shangw@linux.vnet.ibm.com>
On Wed, 2013-06-05 at 15:34 +0800, Gavin Shan wrote:
> Initially, the series of patches is built based on 3.10.RC1 and the patchset
> doesn't intend to enable EEH functionality for PHB3 for now. Obviously, PHB3
> EEH support on PowerNV platform is something to do in future.
One thing missing here is a first patch that moves the eeh core out of
platform/pseries or things will simply not build if CONFIG_PPC_PSERIES
isn't enabled :-)
Move the whole lot to arch/powerpc/kernel
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 21/27] powerpc/eeh: Process interrupts caused by EEH
From: Benjamin Herrenschmidt @ 2013-06-11 8:13 UTC (permalink / raw)
To: Gavin Shan; +Cc: linuxppc-dev
In-Reply-To: <1370417668-16832-22-git-send-email-shangw@linux.vnet.ibm.com>
On Wed, 2013-06-05 at 15:34 +0800, Gavin Shan wrote:
> diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
> index d1fd5d4..68ac408 100644
> --- a/arch/powerpc/include/asm/eeh.h
> +++ b/arch/powerpc/include/asm/eeh.h
> @@ -209,6 +209,12 @@ void eeh_add_device_tree_late(struct pci_bus *);
> void eeh_add_sysfs_files(struct pci_bus *);
> void eeh_remove_bus_device(struct pci_dev *, int);
>
> +#ifdef CONFIG_PPC_POWERNV
> +void pci_err_release(void);
> +#else
> +static inline void pci_err_release(void) { }
> +#endif
That business of the EEH core calling back into the powernv code
directly is gross. We don't do that...
See below for a discussion...
.../...
> +static void pci_err_take(void)
> +{
> + down(&pci_err_seq_sem);
> +}
> +
> +/**
> + * pci_err_release - Enable error report for sending events
> + *
> + * We're hanlding the EEH event one by one. Each time, there only has
> + * one EEH event caused by error IRQ. The function is called to enable
> + * error report in order to send more EEH events.
> + */
> +void pci_err_release(void)
> +{
> + up(&pci_err_seq_sem);
> +}
So it's generally bad to keep a semaphore held like that for a long
time, taken in one corner of the kernel and released in another.
I think you need to do something else. I'm not 100% certain what but
that doesn't seem right to me.
Also you have two problems I see here:
- A given error will come potentially as both an interrupt and
a return of ffff's from MMIO. You don't know which one will get it first
and you end up going through two fairly different code path maybe. IE.
What happens if interrupts are off for a while on the CPU that is
targetted by the PHB interrupt and you "detect" a PHB fence as a result
of an MMIO on another CPU ? Will the normal EEH process clear the fence
and your interrupt completely miss logging any of those fancy messages
you added to this file ?
- You create another kthread ... we already have one in eeh_event.c,
why another ?
I think you need to rethink that part. My idea is that the EEH
interrupts coming from the OPAL notifier would cause you to queue up
EEH events just like the current ones.
IE. Everything (including get_next_error) should be done by the one EEH
thread. This also avoids the needs for those extra semaphores.
One option is to create an event without a PE pointer at all. When
eeh_event_handler() gets that, it would iterate a new hook,
eeh_ops->next_error() which returns the PE.
That way you can do your printing for fences etc... and return the
top-level PE for anything PHB-wide. You may also want to add a flag
maybe to return non-recoverable events and essentially make EEH just
remove the offending devices from the system instead of panic'ing (panic
is never a good idea, for all I know, the dead PHB or dead IOC wasn't
critical to the system operating normally and you may have killed my
ability to even recover the logs by panic'ing).
Think a bit about it. I know the RTAS model is fairly different than our
model here, but I like the idea that on powernv, even if we detect an
MMIO freeze, we don't directly tell the EEH core to process *that* PE
but instead do the whole next_error thing as well. If the freeze was the
result of a fence, there's no point trying to process that specific PE.
Something like a fence would thus look like that:
- [ Case 1 -> fence interrupt -> queue eeh_event with no PE ]
[ Case 2 -> MMIO freeze detected -> queue eeh event with no PE ]
- eeh_event_handler() sees no PE, loops around eeh_ops->get_next_error,
since we are single threaded in the EEH thread, it's ok for the IODA
backend to "cache" the current error data so that subsequent calls into
the backend know what we are doing.
- get_next_error sees the fence, returns the top-level PE and starts
the reset (don't wait)
- eeh_event_handler() calls the drivers for all devices on that PE
(including children) to notify them something's wrong (TODO: Add passing
by the upper level that this is a fatal error and don't attempt to
recover).
- It then calls wait_state() which knows it's waiting on a fence, and
do the appropriate waiting etc...
- Back to normal process...
Don't you think that might be cleaner ? Or do you see a gaping hole in
my description ?
> +static void pci_err_hub_diag_common(struct OpalIoP7IOCErrorData *data)
> +{
> + /* GEM */
> + pr_info(" GEM XFIR: %016llx\n", data->gemXfir);
> + pr_info(" GEM RFIR: %016llx\n", data->gemRfir);
> + pr_info(" GEM RIRQFIR: %016llx\n", data->gemRirqfir);
> + pr_info(" GEM Mask: %016llx\n", data->gemMask);
> + pr_info(" GEM RWOF: %016llx\n", data->gemRwof);
> +
> + /* LEM */
> + pr_info(" LEM FIR: %016llx\n", data->lemFir);
> + pr_info(" LEM Error Mask: %016llx\n", data->lemErrMask);
> + pr_info(" LEM Action 0: %016llx\n", data->lemAction0);
> + pr_info(" LEM Action 1: %016llx\n", data->lemAction1);
> + pr_info(" LEM WOF: %016llx\n", data->lemWof);
> +}
That's stuff is P7IOC specific. Make sure you make it clear in the
function name and that you check the diag data "type". IE. Use a new
diag_data2 function that returns a type. We can obsolete the old one.
> +static void pci_err_hub_diag_data(struct pci_controller *hose)
> +{
> + struct pnv_phb *phb = hose->private_data;
> + struct OpalIoP7IOCErrorData *data;
> + long ret;
> +
> + data = (struct OpalIoP7IOCErrorData *)pci_err_diag;
> + ret = opal_pci_get_hub_diag_data(phb->hub_id, data, PAGE_SIZE);
> + if (ret != OPAL_SUCCESS) {
> + pr_warning("%s: Failed to get HUB#%llx diag-data, ret=%ld\n",
> + __func__, phb->hub_id, ret);
> + return;
> + }
> +
> + /* Check the error type */
> + if (data->type <= OPAL_P7IOC_DIAG_TYPE_NONE ||
> + data->type >= OPAL_P7IOC_DIAG_TYPE_LAST) {
> + pr_warning("%s: Invalid type of HUB#%llx diag-data (%d)\n",
> + __func__, phb->hub_id, data->type);
> + return;
> + }
> +
> + switch (data->type) {
> + case OPAL_P7IOC_DIAG_TYPE_RGC:
> + pr_info("P7IOC diag-data for RGC\n\n");
> + pci_err_hub_diag_common(data);
> + pr_info(" RGC Status: %016llx\n", data->rgc.rgcStatus);
> + pr_info(" RGC LDCP: %016llx\n", data->rgc.rgcLdcp);
> + break;
> + case OPAL_P7IOC_DIAG_TYPE_BI:
> + pr_info("P7IOC diag-data for BI %s\n\n",
> + data->bi.biDownbound ? "Downbound" : "Upbound");
> + pci_err_hub_diag_common(data);
> + pr_info(" BI LDCP 0: %016llx\n", data->bi.biLdcp0);
> + pr_info(" BI LDCP 1: %016llx\n", data->bi.biLdcp1);
> + pr_info(" BI LDCP 2: %016llx\n", data->bi.biLdcp2);
> + pr_info(" BI Fence Status: %016llx\n", data->bi.biFenceStatus);
> + break;
> + case OPAL_P7IOC_DIAG_TYPE_CI:
> + pr_info("P7IOC diag-data for CI Port %d\\nn",
> + data->ci.ciPort);
> + pci_err_hub_diag_common(data);
> + pr_info(" CI Port Status: %016llx\n", data->ci.ciPortStatus);
> + pr_info(" CI Port LDCP: %016llx\n", data->ci.ciPortLdcp);
> + break;
> + case OPAL_P7IOC_DIAG_TYPE_MISC:
> + pr_info("P7IOC diag-data for MISC\n\n");
> + pci_err_hub_diag_common(data);
> + break;
> + case OPAL_P7IOC_DIAG_TYPE_I2C:
> + pr_info("P7IOC diag-data for I2C\n\n");
> + pci_err_hub_diag_common(data);
> + break;
> + }
> +}
> +
> +static void pci_err_phb_diag_data(struct pci_controller *hose)
> +{
> + struct pnv_phb *phb = hose->private_data;
> + struct OpalIoP7IOCPhbErrorData *data;
> + int i;
> + long ret;
> +
> + data = (struct OpalIoP7IOCPhbErrorData *)pci_err_diag;
> + ret = opal_pci_get_phb_diag_data2(phb->opal_id, data, PAGE_SIZE);
> + if (ret != OPAL_SUCCESS) {
> + pr_warning("%s: Failed to get diag-data for PHB#%x, ret=%ld\n",
> + __func__, hose->global_number, ret);
> + return;
> + }
> +
> + pr_info("PHB#%x Diag-data\n\n", hose->global_number);
> + pr_info(" brdgCtl: %08x\n", data->brdgCtl);
> +
> + pr_info(" portStatusReg: %08x\n", data->portStatusReg);
> + pr_info(" rootCmplxStatus: %08x\n", data->rootCmplxStatus);
> + pr_info(" busAgentStatus: %08x\n", data->busAgentStatus);
> +
> + pr_info(" deviceStatus: %08x\n", data->deviceStatus);
> + pr_info(" slotStatus: %08x\n", data->slotStatus);
> + pr_info(" linkStatus: %08x\n", data->linkStatus);
> + pr_info(" devCmdStatus: %08x\n", data->devCmdStatus);
> + pr_info(" devSecStatus: %08x\n", data->devSecStatus);
> +
> + pr_info(" rootErrorStatus: %08x\n", data->rootErrorStatus);
> + pr_info(" uncorrErrorStatus: %08x\n", data->uncorrErrorStatus);
> + pr_info(" corrErrorStatus: %08x\n", data->corrErrorStatus);
> + pr_info(" tlpHdr1: %08x\n", data->tlpHdr1);
> + pr_info(" tlpHdr2: %08x\n", data->tlpHdr2);
> + pr_info(" tlpHdr3: %08x\n", data->tlpHdr3);
> + pr_info(" tlpHdr4: %08x\n", data->tlpHdr4);
> + pr_info(" sourceId: %08x\n", data->sourceId);
> +
> + pr_info(" errorClass: %016llx\n", data->errorClass);
> + pr_info(" correlator: %016llx\n", data->correlator);
> + pr_info(" p7iocPlssr: %016llx\n", data->p7iocPlssr);
> + pr_info(" p7iocCsr: %016llx\n", data->p7iocCsr);
> + pr_info(" lemFir: %016llx\n", data->lemFir);
> + pr_info(" lemErrorMask: %016llx\n", data->lemErrorMask);
> + pr_info(" lemWOF: %016llx\n", data->lemWOF);
> + pr_info(" phbErrorStatus: %016llx\n", data->phbErrorStatus);
> + pr_info(" phbFirstErrorStatus: %016llx\n", data->phbFirstErrorStatus);
> + pr_info(" phbErrorLog0: %016llx\n", data->phbErrorLog0);
> + pr_info(" phbErrorLog1: %016llx\n", data->phbErrorLog1);
> + pr_info(" mmioErrorStatus: %016llx\n", data->mmioErrorStatus);
> + pr_info(" mmioFirstErrorStatus: %016llx\n", data->mmioFirstErrorStatus);
> + pr_info(" mmioErrorLog0: %016llx\n", data->mmioErrorLog0);
> + pr_info(" mmioErrorLog1: %016llx\n", data->mmioErrorLog1);
> + pr_info(" dma0ErrorStatus: %016llx\n", data->dma0ErrorStatus);
> + pr_info(" dma0FirstErrorStatus: %016llx\n", data->dma0FirstErrorStatus);
> + pr_info(" dma0ErrorLog0: %016llx\n", data->dma0ErrorLog0);
> + pr_info(" dma0ErrorLog1: %016llx\n", data->dma0ErrorLog1);
> + pr_info(" dma1ErrorStatus: %016llx\n", data->dma1ErrorStatus);
> + pr_info(" dma1FirstErrorStatus: %016llx\n", data->dma1FirstErrorStatus);
> + pr_info(" dma1ErrorLog0: %016llx\n", data->dma1ErrorLog0);
> + pr_info(" dma1ErrorLog1: %016llx\n", data->dma1ErrorLog1);
> +
> + for (i = 0; i < OPAL_P7IOC_NUM_PEST_REGS; i++) {
> + if ((data->pestA[i] >> 63) == 0 &&
> + (data->pestB[i] >> 63) == 0)
> + continue;
> +
> + pr_info(" PE[%3d] PESTA: %016llx\n", i, data->pestA[i]);
> + pr_info(" PESTB: %016llx\n", data->pestB[i]);
> + }
> +}
> +
> +/*
> + * Process PCI errors from IOC, PHB, or PE. Here's the list
> + * of expected error types and their severities, as well as
> + * the corresponding action.
> + *
> + * Type Severity Action
> + * OPAL_EEH_ERROR_IOC OPAL_EEH_SEV_IOC_DEAD panic
> + * OPAL_EEH_ERROR_IOC OPAL_EEH_SEV_INF diag_data
> + * OPAL_EEH_ERROR_PHB OPAL_EEH_SEV_PHB_DEAD panic
> + * OPAL_EEH_ERROR_PHB OPAL_EEH_SEV_PHB_FENCED eeh
> + * OPAL_EEH_ERROR_PHB OPAL_EEH_SEV_INF diag_data
> + * OPAL_EEH_ERROR_PE OPAL_EEH_SEV_PE_ER eeh
> + */
> +static void pci_err_process(struct pci_controller *hose,
> + u16 err_type, u16 severity, u16 pe_no)
> +{
> + PCI_ERR_DBG("PCI_ERR: Process error (%d, %d, %d) on PHB#%x\n",
> + err_type, severity, pe_no, hose->global_number);
> +
> + switch (err_type) {
> + case OPAL_EEH_IOC_ERROR:
> + if (severity == OPAL_EEH_SEV_IOC_DEAD)
> + panic("Dead IOC of PHB#%x", hose->global_number);
> + else if (severity == OPAL_EEH_SEV_INF) {
> + pci_err_hub_diag_data(hose);
> + pci_err_release();
> + }
> +
> + break;
> + case OPAL_EEH_PHB_ERROR:
> + if (severity == OPAL_EEH_SEV_PHB_DEAD)
> + panic("Dead PHB#%x", hose->global_number);
> + else if (severity == OPAL_EEH_SEV_PHB_FENCED)
> + pci_err_check_phb(hose);
> + else if (severity == OPAL_EEH_SEV_INF) {
> + pci_err_phb_diag_data(hose);
> + pci_err_release();
> + }
> +
> + break;
> + case OPAL_EEH_PE_ERROR:
> + pci_err_check_pe(hose, pe_no);
> + break;
> + }
> +}
> +
> +static int pci_err_handler(void *dummy)
> +{
> + struct pnv_phb *phb;
> + struct pci_controller *hose, *tmp;
> + u64 frozen_pe_no;
> + u16 err_type, severity;
> + long ret;
> +
> + while (!kthread_should_stop()) {
> + down(&pci_err_int_sem);
> + PCI_ERR_DBG("PCI_ERR: Get PCI error semaphore\n");
> +
> + list_for_each_entry_safe(hose, tmp, &hose_list, list_node) {
> + phb = hose->private_data;
> +restart:
> + pci_err_take();
> + ret = opal_pci_next_error(phb->opal_id,
> + &frozen_pe_no, &err_type, &severity);
> +
> + /* If OPAL API returns error, we needn't proceed */
> + if (ret != OPAL_SUCCESS) {
> + PCI_ERR_DBG("PCI_ERR: Invalid return value on "
> + "PHB#%x (0x%lx) from opal_pci_next_error",
> + hose->global_number, ret);
> + pci_err_release();
> + continue;
> + }
> +
> + /* If the PHB doesn't have error, stop processing */
> + if (err_type == OPAL_EEH_NO_ERROR ||
> + severity == OPAL_EEH_SEV_NO_ERROR) {
> + PCI_ERR_DBG("PCI_ERR: No error found on PHB#%x\n",
> + hose->global_number);
> + pci_err_release();
> + continue;
> + }
> +
> + /*
> + * Process the error until there're no pending
> + * errors on the specific PHB.
> + */
> + pci_err_process(hose, err_type, severity, frozen_pe_no);
> + goto restart;
> + }
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * pci_err_init - Initialize PCI error handling component
> + *
> + * It should be done before OPAL interrupts got registered because
> + * that depends on this.
> + */
> +static int __init pci_err_init(void)
> +{
> + int ret = 0;
> +
> + if (!firmware_has_feature(FW_FEATURE_OPALv3)) {
> + pr_err("%s: FW_FEATURE_OPALv3 required!\n",
> + __func__);
> + return -EINVAL;
> + }
> +
> + pci_err_diag = (char *)__get_free_page(GFP_KERNEL|__GFP_ZERO);
> + if (!pci_err_diag) {
> + pr_err("%s: Failed to alloc memory for diag data\n",
> + __func__);
> + return -ENOMEM;
> + }
> +
> + /* Initialize semaphore */
> + sema_init(&pci_err_int_sem, 0);
> + sema_init(&pci_err_seq_sem, 1);
> +
> + /* Start kthread */
> + pci_err_thread = kthread_run(pci_err_handler, NULL, "PCI_ERR");
> + if (IS_ERR(pci_err_thread)) {
> + ret = PTR_ERR(pci_err_thread);
> + pr_err("%s: Failed to start kthread, ret=%d\n",
> + __func__, ret);
> + }
> +
> + free_page((unsigned long)pci_err_diag);
> + return ret;
> +}
> +
> +arch_initcall(pci_err_init);
> diff --git a/arch/powerpc/platforms/pseries/eeh_event.c b/arch/powerpc/platforms/pseries/eeh_event.c
> index 1f86b80..e4c636e 100644
> --- a/arch/powerpc/platforms/pseries/eeh_event.c
> +++ b/arch/powerpc/platforms/pseries/eeh_event.c
> @@ -84,6 +84,14 @@ static int eeh_event_handler(void * dummy)
> eeh_handle_event(pe);
> eeh_pe_state_clear(pe, EEH_PE_RECOVERING);
>
> + /*
> + * If it's the event caused by error reporting IRQ,
> + * we need release the module so that precedent events
> + * could be fired.
> + */
> + if (event->flag & EEH_EVENT_INT)
> + pci_err_release();
> +
> kfree(event);
> mutex_unlock(&eeh_event_mutex);
>
^ permalink raw reply
* Re: [RFC PATCH powerpc] Set cpu sibling mask before online cpu
From: Srivatsa S. Bhat @ 2013-06-11 8:33 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: PowerPC email list, Paul Mackerras, Nikunj A Dadhania, Li Zhong
In-Reply-To: <1370934016.8250.83.camel@pasglop>
On 06/11/2013 12:30 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2013-05-16 at 18:20 +0800, Li Zhong wrote:
>> It seems following race is possible:
>>
>
> .../...
>
>> vdso_getcpu_init();
>> #endif
>> - notify_cpu_starting(cpu);
>> - set_cpu_online(cpu, true);
>> /* Update sibling maps */
>> base = cpu_first_thread_sibling(cpu);
>> for (i = 0; i < threads_per_core; i++) {
>> - if (cpu_is_offline(base + i))
>> + if (cpu_is_offline(base + i) && (cpu != base + i))
>> continue;
>> cpumask_set_cpu(cpu, cpu_sibling_mask(base + i));
>> cpumask_set_cpu(base + i, cpu_sibling_mask(cpu));
>> @@ -667,6 +665,10 @@ __cpuinit void start_secondary(void *unused)
>> }
>> of_node_put(l2_cache);
>>
>> + smp_wmb();
>> + notify_cpu_starting(cpu);
>> + set_cpu_online(cpu, true);
>> +
>
> So we could have an online CPU with an empty sibling mask. Now we can
> have a sibling that isn't online ... Is that ok ?
I think it is OK. We do the same thing on x86 as well - we set up the
sibling links before calling notify_cpu_starting() and setting the cpu
in the cpu_online_mask. In fact, there is even a comment explicitly
noting that order:
arch/x86/kernel/smpboot.c:
220 /*
221 * This must be done before setting cpu_online_mask
222 * or calling notify_cpu_starting.
223 */
224 set_cpu_sibling_map(raw_smp_processor_id());
225 wmb();
226
227 notify_cpu_starting(cpuid);
228
229 /*
230 * Allow the master to continue.
231 */
232 cpumask_set_cpu(cpuid, cpu_callin_mask);
So I agree with Li Zhong's solution.
[Arch-specific CPU hotplug code consolidation efforts such as [1] would
have weeded out such nasty bugs.. I guess we should revive that patchset
sometime soon.]
Regards,
Srivatsa S. Bhat
[1]. https://lwn.net/Articles/500185/
^ permalink raw reply
* Re: [RFC PATCH powerpc] Set cpu sibling mask before online cpu
From: Srivatsa S. Bhat @ 2013-06-11 8:35 UTC (permalink / raw)
To: Li Zhong; +Cc: Paul Mackerras, PowerPC email list
In-Reply-To: <1368699626.2618.183.camel@ThinkPad-T5421>
On 05/16/2013 03:50 PM, Li Zhong wrote:
> It seems following race is possible:
>
> cpu0 cpux
> smp_init->cpu_up->_cpu_up
> __cpu_up
> kick_cpu(1)
> -------------------------------------------------------------------------
> waiting online ...
> ... notify CPU_STARTING
> set cpux active
> set cpux online
> -------------------------------------------------------------------------
> finish waiting online
> ...
> sched_init_smp
> init_sched_domains(cpu_active_mask)
> build_sched_domains
> set cpux sibling info
> -------------------------------------------------------------------------
>
[...]
> This patch tries to move the sibling maps updating before
> notify_cpu_starting() and cpu online, and a write barrier there to make
> sure sibling maps are updated before active and online mask.
>
> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> ---
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Regards,
Srivatsa S. Bhat
> arch/powerpc/kernel/smp.c | 8 +++++---
> 1 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index ee7ac5e..c765937 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -637,12 +637,10 @@ __cpuinit void start_secondary(void *unused)
>
> vdso_getcpu_init();
> #endif
> - notify_cpu_starting(cpu);
> - set_cpu_online(cpu, true);
> /* Update sibling maps */
> base = cpu_first_thread_sibling(cpu);
> for (i = 0; i < threads_per_core; i++) {
> - if (cpu_is_offline(base + i))
> + if (cpu_is_offline(base + i) && (cpu != base + i))
> continue;
> cpumask_set_cpu(cpu, cpu_sibling_mask(base + i));
> cpumask_set_cpu(base + i, cpu_sibling_mask(cpu));
> @@ -667,6 +665,10 @@ __cpuinit void start_secondary(void *unused)
> }
> of_node_put(l2_cache);
>
> + smp_wmb();
> + notify_cpu_starting(cpu);
> + set_cpu_online(cpu, true);
> +
> local_irq_enable();
>
> cpu_startup_entry(CPUHP_ONLINE);
>
>
>
^ permalink raw reply
* Re: [RFC PATCH powerpc] Set cpu sibling mask before online cpu
From: Benjamin Herrenschmidt @ 2013-06-11 9:03 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: PowerPC email list, Paul Mackerras, Nikunj A Dadhania, Li Zhong
In-Reply-To: <51B6E0F4.6040008@linux.vnet.ibm.com>
On Tue, 2013-06-11 at 14:03 +0530, Srivatsa S. Bhat wrote:
>
> So I agree with Li Zhong's solution.
>
> [Arch-specific CPU hotplug code consolidation efforts such as [1] would
> have weeded out such nasty bugs.. I guess we should revive that patchset
> sometime soon.]
Thanks !
Cheers,
Ben.
^ permalink raw reply
* RE: [PATCH 0/2 v3] powerpc: Make ptrace work reliably
From: Bhushan Bharat-R65777 @ 2013-06-11 12:08 UTC (permalink / raw)
To: Bhushan Bharat-R65777, galak@kernel.crashing.org,
benh@kernel.crashing.org, linuxppc-dev@lists.ozlabs.org,
Wood Scott-B07421, Yoder Stuart-B08248, Yang James-RA8135
In-Reply-To: <1369196459-17275-1-git-send-email-Bharat.Bhushan@freescale.com>
Hi Ben,
Ping;=20
Please review this patchset ..=20
Thanks
-Bharat
> -----Original Message-----
> From: Bhushan Bharat-R65777
> Sent: Wednesday, May 22, 2013 9:51 AM
> To: galak@kernel.crashing.org; benh@kernel.crashing.org; linuxppc-
> dev@lists.ozlabs.org; Wood Scott-B07421; Yoder Stuart-B08248; Yang James-=
RA8135
> Cc: Bhushan Bharat-R65777
> Subject: [PATCH 0/2 v3] powerpc: Make ptrace work reliably
>=20
> From: Bharat Bhushan <bharat.bhushan@freescale.com>
>=20
> v2->v3
> - Load PACACURRENT immediately after _MSR(r1), and load DBCR0
> just after "beq resume_kernel
> - Added lat_sysycal results before and after the patch
>=20
> v1->v2
> - Subject line was missing 0/2, 1/2, 2/2
>=20
> Bharat Bhushan (2):
> powerpc: debug control and status registers are 32bit =3D> This patch m=
akes
> debug control and status registers as 32bit as they are.
> This does not fix anything
>=20
> powerpc: restore dbcr0 on user space exit =3D> This patch fixes the ptr=
ace
> reliability issue. The description is the patch
> describes one of the case where it does not work reliably
>=20
> arch/powerpc/include/asm/processor.h | 8 ++++----
> arch/powerpc/kernel/asm-offsets.c | 1 +
> arch/powerpc/kernel/entry_64.S | 28 ++++++++++++++++++++++++----
> 3 files changed, 29 insertions(+), 8 deletions(-)
^ permalink raw reply
* Re: [BUG] PCI related panic on powerpc based board with 3.10-rcX
From: Scott Wood @ 2013-06-11 17:00 UTC (permalink / raw)
To: Rojhalat Ibrahim; +Cc: linuxppc-dev, linux-kernel, Michael Guntsche
In-Reply-To: <10631172.HfDeIDHqX3@pcimr>
On 06/11/2013 02:24:28 AM, Rojhalat Ibrahim wrote:
> On Monday 10 June 2013 17:52:33 Scott Wood wrote:
> > On 06/10/2013 12:07:43 PM, Michael Guntsche wrote:
> > > Good evening,
> > >
> > > This patch does not fix the problem, during boot the kernel still
> > > panics. I had a closer look at the commit and the following patch
> > > fixes it for me....
> > >
> > > diff --git a/arch/powerpc/sysdev/fsl_pci.c
> > > b/arch/powerpc/sysdev/fsl_pci.c
> > > index 028ac1f..21b687f 100644
> > > --- a/arch/powerpc/sysdev/fsl_pci.c
> > > +++ b/arch/powerpc/sysdev/fsl_pci.c
> > > @@ -814,7 +814,7 @@ int __init mpc83xx_add_bridge(struct =20
> device_node
> > > *dev)
> > >
> > > if (ret)
> > >
> > > goto err0;
> > >
> > > } else {
> > >
> > > - fsl_setup_indirect_pci(hose, rsrc_cfg.start,
> > > + setup_indirect_pci(hose, rsrc_cfg.start,
> > >
> > > rsrc_cfg.start + 4, 0);
> > >
> > > }
> >
> > The only difference here is that you're not setting hose->ops to
> > fsl_indirect_pci_ops. Do you know why that is helping, and what
> > hose->ops is set to instead?
> >
> > -Scott
>=20
> The difference is only the read function in hose->ops, which is set to
> indirect_read_config instead of fsl_indirect_read_config.
>=20
> fsl_indirect_read_config calls fsl_pcie_check_link, which is where =20
> the Oops
> occurs.
Why is fsl_pcie_check_link being called for non-PCIe buses?
> Mike, can you find out where exactly in fsl_pcie_check_link the bad =20
> access
> happens? Enabling CONFIG_DEBUG_BUGVERBOSE might help.
Why does it matter? You shouldn't be calling that function at all.
-Scott=
^ permalink raw reply
* Re: [BUG] PCI related panic on powerpc based board with 3.10-rcX
From: Michael Guntsche @ 2013-06-11 17:09 UTC (permalink / raw)
To: Scott Wood; +Cc: linuxppc-dev, Rojhalat Ibrahim, linux-kernel
In-Reply-To: <1370970051.18413.24@snotra>
On Tue, Jun 11, 2013 at 7:00 PM, Scott Wood <scottwood@freescale.com> wrote:
> On 06/11/2013 02:24:28 AM, Rojhalat Ibrahim wrote:
>>
>> On Monday 10 June 2013 17:52:33 Scott Wood wrote:
>> > On 06/10/2013 12:07:43 PM, Michael Guntsche wrote:
>> > > Good evening,
>> > >
>> > > This patch does not fix the problem, during boot the kernel still
>> > > panics. I had a closer look at the commit and the following patch
>> > > fixes it for me....
>> > >
>> > > diff --git a/arch/powerpc/sysdev/fsl_pci.c
>> > > b/arch/powerpc/sysdev/fsl_pci.c
>> > > index 028ac1f..21b687f 100644
>> > > --- a/arch/powerpc/sysdev/fsl_pci.c
>> > > +++ b/arch/powerpc/sysdev/fsl_pci.c
>> > > @@ -814,7 +814,7 @@ int __init mpc83xx_add_bridge(struct device_node
>> > > *dev)
>> > >
>> > > if (ret)
>> > >
>> > > goto err0;
>> > >
>> > > } else {
>> > >
>> > > - fsl_setup_indirect_pci(hose, rsrc_cfg.start,
>> > > + setup_indirect_pci(hose, rsrc_cfg.start,
>> > >
>> > > rsrc_cfg.start + 4, 0);
>> > >
>> > > }
>> >
>> > The only difference here is that you're not setting hose->ops to
>> > fsl_indirect_pci_ops. Do you know why that is helping, and what
>> > hose->ops is set to instead?
>> >
>> > -Scott
>>
>> The difference is only the read function in hose->ops, which is set to
>> indirect_read_config instead of fsl_indirect_read_config.
>>
>> fsl_indirect_read_config calls fsl_pcie_check_link, which is where the
>> Oops
>> occurs.
>
>
> Why is fsl_pcie_check_link being called for non-PCIe buses?
>
>
>> Mike, can you find out where exactly in fsl_pcie_check_link the bad access
>> happens? Enabling CONFIG_DEBUG_BUGVERBOSE might help.
>
>
> Why does it matter? You shouldn't be calling that function at all.
>
> -Scott
For the record BUGVERBOSE is already set with this build so this is
the most detailed trace I get. And regarding Scott's remark, maybe I
was not clear enough in my first report. This is a PCI only board so I
also wondered about the call to fsl_pcie_check_link in the first
place. Since apparently the 83xx related add bridge code already has a
case for boards with PCIe support. So I think the change should really
happen somewhere in this code and not in the PCI only path.
/Mike
^ permalink raw reply
* Re: [BUG] PCI related panic on powerpc based board with 3.10-rcX
From: Scott Wood @ 2013-06-11 17:28 UTC (permalink / raw)
To: Michael Guntsche; +Cc: linuxppc-dev, Rojhalat Ibrahim, linux-kernel
In-Reply-To: <CALG0vJumuRobQtHdWAuWtbC3oS3_CMpDim=dtSNECCO25SgJUA@mail.gmail.com>
On 06/11/2013 12:09:42 PM, Michael Guntsche wrote:
> On Tue, Jun 11, 2013 at 7:00 PM, Scott Wood <scottwood@freescale.com> =20
> wrote:
> > On 06/11/2013 02:24:28 AM, Rojhalat Ibrahim wrote:
> >>
> >> On Monday 10 June 2013 17:52:33 Scott Wood wrote:
> >> > On 06/10/2013 12:07:43 PM, Michael Guntsche wrote:
> >> > > Good evening,
> >> > >
> >> > > This patch does not fix the problem, during boot the kernel =20
> still
> >> > > panics. I had a closer look at the commit and the following =20
> patch
> >> > > fixes it for me....
> >> > >
> >> > > diff --git a/arch/powerpc/sysdev/fsl_pci.c
> >> > > b/arch/powerpc/sysdev/fsl_pci.c
> >> > > index 028ac1f..21b687f 100644
> >> > > --- a/arch/powerpc/sysdev/fsl_pci.c
> >> > > +++ b/arch/powerpc/sysdev/fsl_pci.c
> >> > > @@ -814,7 +814,7 @@ int __init mpc83xx_add_bridge(struct =20
> device_node
> >> > > *dev)
> >> > >
> >> > > if (ret)
> >> > >
> >> > > goto err0;
> >> > >
> >> > > } else {
> >> > >
> >> > > - fsl_setup_indirect_pci(hose, rsrc_cfg.start,
> >> > > + setup_indirect_pci(hose, rsrc_cfg.start,
> >> > >
> >> > > rsrc_cfg.start + 4, 0);
> >> > >
> >> > > }
> >> >
> >> > The only difference here is that you're not setting hose->ops to
> >> > fsl_indirect_pci_ops. Do you know why that is helping, and what
> >> > hose->ops is set to instead?
> >> >
> >> > -Scott
> >>
> >> The difference is only the read function in hose->ops, which is =20
> set to
> >> indirect_read_config instead of fsl_indirect_read_config.
> >>
> >> fsl_indirect_read_config calls fsl_pcie_check_link, which is where =20
> the
> >> Oops
> >> occurs.
> >
> >
> > Why is fsl_pcie_check_link being called for non-PCIe buses?
> >
> >
> >> Mike, can you find out where exactly in fsl_pcie_check_link the =20
> bad access
> >> happens? Enabling CONFIG_DEBUG_BUGVERBOSE might help.
> >
> >
> > Why does it matter? You shouldn't be calling that function at all.
> >
> > -Scott
>=20
> For the record BUGVERBOSE is already set with this build so this is
> the most detailed trace I get. And regarding Scott's remark, maybe I
> was not clear enough in my first report. This is a PCI only board so I
> also wondered about the call to fsl_pcie_check_link in the first
> place.
Yes, I figured it was non-PCIe because the code change that you said =20
helped was on the non-PCIe branch of the if/else. Generally it's good =20
to explicitly mention the chip you're using, though.
fsl_setup_indirect_pci should be renamed to fsl_setup_indirect_pcie. =20
Your patch above should be applied, and fsl_setup_indirect_pcie should =20
be moved into the booke/86xx ifdef to avoid an unused function warning.
-Scott=
^ permalink raw reply
* [PATCH] powerpc/iommu: Remove unused pci_iommu_init() and pci_direct_iommu_init()
From: Bjorn Helgaas @ 2013-06-11 19:57 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras; +Cc: linuxppc-dev
pci_iommu_init() and pci_direct_iommu_init() are not referenced anywhere,
so remove them.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
arch/powerpc/include/asm/iommu.h | 7 -------
1 file changed, 7 deletions(-)
diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
index cbfe678..e670f73 100644
--- a/arch/powerpc/include/asm/iommu.h
+++ b/arch/powerpc/include/asm/iommu.h
@@ -125,13 +125,6 @@ extern void iommu_init_early_pSeries(void);
extern void iommu_init_early_dart(void);
extern void iommu_init_early_pasemi(void);
-#ifdef CONFIG_PCI
-extern void pci_iommu_init(void);
-extern void pci_direct_iommu_init(void);
-#else
-static inline void pci_iommu_init(void) { }
-#endif
-
extern void alloc_dart_table(void);
#if defined(CONFIG_PPC64) && defined(CONFIG_PM)
static inline void iommu_save(void)
^ permalink raw reply related
* Re: [PATCH -V7 09/18] powerpc: Switch 16GB and 16MB explicit hugepages to a different page table format
From: Scott Wood @ 2013-06-11 20:53 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: linux-mm, paulus, linuxppc-dev, dwg
In-Reply-To: <87obbgpmk3.fsf@linux.vnet.ibm.com>
On 06/08/2013 11:57:48 AM, Aneesh Kumar K.V wrote:
> With the config shared I am not finding anything wrong, but I can't =20
> test
> these configs. Also can you confirm what you bisect this to
>=20
> e2b3d202d1dba8f3546ed28224ce485bc50010be
> powerpc: Switch 16GB and 16MB explicit hugepages to a different page =20
> table format
>=20
> or
>=20
> cf9427b85e90bb1ff90e2397ff419691d983c68b "powerpc: New hugepage =20
> directory format"
It's e2b3d202d1dba8f3546ed28224ce485bc50010be.
It turned out to be the change from "pmd_none" to =20
"pmd_none_or_clear_bad". Making that change triggers the "bad pmd" =20
messages even when applied to v3.9 -- so we had bad pmds all along, =20
undetected. Now I get to figure out why. :-(
-Scott=
^ permalink raw reply
* Re: [PATCH] powerpc/iommu: Remove unused pci_iommu_init() and pci_direct_iommu_init()
From: Benjamin Herrenschmidt @ 2013-06-11 21:27 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linuxppc-dev, Paul Mackerras
In-Reply-To: <20130611195705.17794.68833.stgit@bhelgaas-glaptop>
On Tue, 2013-06-11 at 13:57 -0600, Bjorn Helgaas wrote:
> pci_iommu_init() and pci_direct_iommu_init() are not referenced anywhere,
> so remove them.
Ah indeed, some old stuff...
Thanks,
Ben.
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> arch/powerpc/include/asm/iommu.h | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
> index cbfe678..e670f73 100644
> --- a/arch/powerpc/include/asm/iommu.h
> +++ b/arch/powerpc/include/asm/iommu.h
> @@ -125,13 +125,6 @@ extern void iommu_init_early_pSeries(void);
> extern void iommu_init_early_dart(void);
> extern void iommu_init_early_pasemi(void);
>
> -#ifdef CONFIG_PCI
> -extern void pci_iommu_init(void);
> -extern void pci_direct_iommu_init(void);
> -#else
> -static inline void pci_iommu_init(void) { }
> -#endif
> -
> extern void alloc_dart_table(void);
> #if defined(CONFIG_PPC64) && defined(CONFIG_PM)
> static inline void iommu_save(void)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox