[RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Milton Miller]

After the following patch to mark the isa region busy and applying a few
patches[1], I was able to kexec boot into an all-yes-config kernel from 
linux-next, with the following KCONFIG_ALLCONFIG file:

# CONFIG_KALLSYMS_EXTRA_PASS is not set
# CONFIG_CRASH_DUMP is not set
CONFIG_PPC_EARLY_DEBUG_RTAS_CONSOLE=y
# CONFIG_NSC_FIR is not set
# CONFIG_SMC_IRCC_FIR is not set
# CONFIG_ALI_FIR is not set
# CONFIG_TCG_NSC is not set
# CONFIG_TCG_ATMEL is not set
# CONFIG_SENSORS_F71805F is not set
# CONFIG_SENSORS_F71882FG is not set
# CONFIG_SENSORS_F75375S is not set
# CONFIG_SENSORS_IT87 is not set
# CONFIG_SENSORS_PC87360 is not set
# CONFIG_SENSORS_PC87427 is not set
# CONFIG_SENSORS_DME1737 is not set
# CONFIG_SENSORS_SMSC47M1 is not set
# CONFIG_SENSORS_SMSC47B397 is not set
# CONFIG_SENSORS_VT1211 is not set
# CONFIG_SENSORS_W83627HF is not set
# CONFIG_SENSORS_W83627EHF is not set

While the first two might not be required, and the third is just
selecting the right platform, it would be nice to get the drivers
to play as well as the rest of the kernel.

The drivers all are for super-io chips, either irda/FIR drivers or hwmon,
and poke at isa ports without checking request_region.

It would be great if these 17 super-io drivers would check for resource
availability before poking at io ports.

[not that I expect this to merge as it, but maybe insert-resource or something]



[1] 3 section layout patches and the MTD DISKONCHIP patch,
see linuxppc-dev for full list


diff --git a/arch/powerpc/platforms/pseries/pci.c b/arch/powerpc/platforms/pseries/pci.c
index 2c6ded2..db54620 100644
--- a/arch/powerpc/platforms/pseries/pci.c
+++ b/arch/powerpc/platforms/pseries/pci.c
@@ -58,8 +58,10 @@ DECLARE_PCI_FIXUP_HEADER(PCI_ANY_ID, PCI_ANY_ID, pcibios_name_device);
 
 static void __init pSeries_request_regions(void)
 {
-	if (!isa_io_base)
+	if (!isa_io_base) {
+		request_region(0x0,0x400,"isa");
 		return;
+		}
 
 	request_region(0x20,0x20,"pic1");
 	request_region(0xa0,0x20,"pic2");

mtd: remove __PPC__ hardcoded address from nand/diskonchip and devices/docprobe

[Milton Miller]

Such a hardcoded address can cause a checkstop or machine check if
the driver is in the kernel but the address is not acknowledged.

Both drivers allow an address to be specified as either a module
parameter or config option.   Any future powerpc board should either
use one of these methods or find the address in the device tree.

Signed-off-by: Milton Miller <miltonm@bga.com>
---
There is no powerpc defconfig that sets either CONFIG_DOC200{0,1*} nor
CONFIG_MTD_NAND_DISKONCHIP in next-20080710.

diff --git a/drivers/mtd/devices/docprobe.c b/drivers/mtd/devices/docprobe.c
index 6e5d811..6e62922 100644
--- a/drivers/mtd/devices/docprobe.c
+++ b/drivers/mtd/devices/docprobe.c
@@ -76,8 +76,6 @@ static unsigned long __initdata doc_locations[] = {
 	0xe0000, 0xe2000, 0xe4000, 0xe6000,
 	0xe8000, 0xea000, 0xec000, 0xee000,
 #endif /*  CONFIG_MTD_DOCPROBE_HIGH */
-#elif defined(__PPC__)
-	0xe4000000,
 #else
 #warning Unknown architecture for DiskOnChip. No default probe locations defined
 #endif
diff --git a/drivers/mtd/nand/diskonchip.c b/drivers/mtd/nand/diskonchip.c
index cd4393c..765d4f0 100644
--- a/drivers/mtd/nand/diskonchip.c
+++ b/drivers/mtd/nand/diskonchip.c
@@ -52,8 +52,6 @@ static unsigned long __initdata doc_locations[] = {
 	0xe0000, 0xe2000, 0xe4000, 0xe6000,
 	0xe8000, 0xea000, 0xec000, 0xee000,
 #endif /*  CONFIG_MTD_DOCPROBE_HIGH */
-#elif defined(__PPC__)
-	0xe4000000,
 #else
 #warning Unknown architecture for DiskOnChip. No default probe locations defined
 #endif

[1/3] powerpc: add _HEAD_GLOBAL to place functions in .text.head

[Milton Miller]

This is the first step to moving the interrupt vectors to .text.head.

Rather than creating more repetition, factor out creating a function
descriptor, from declaring it to be global in a section.

The order of .globl and the symbol definitions is changed but is
a don't care.

Signed-off-by: Milton Miller <miltonm@bga.com>
---
I left the section in the macros, and added a new macro.   I didn't
want to hunt down other uses of _GLOBAL to audit the section, and _KPROBE
needs to set the section.


diff --git a/include/asm-powerpc/ppc_asm.h b/include/asm-powerpc/ppc_asm.h
index 0966899..cb5a4b0 100644
--- a/include/asm-powerpc/ppc_asm.h
+++ b/include/asm-powerpc/ppc_asm.h
@@ -178,11 +178,8 @@ END_FTR_SECTION_IFCLR(CPU_FTR_PURR);					\
 #define XGLUE(a,b) a##b
 #define GLUE(a,b) XGLUE(a,b)
 
-#define _GLOBAL(name) \
-	.section ".text"; \
+#define _FUNCTION(name) \
 	.align 2 ; \
-	.globl name; \
-	.globl GLUE(.,name); \
 	.section ".opd","aw"; \
 name: \
 	.quad GLUE(.,name); \
@@ -192,57 +189,41 @@ name: \
 	.type GLUE(.,name),@function; \
 GLUE(.,name):
 
+#define _GLOBAL(name) \
+	.section ".text"; \
+	.globl name; \
+	.globl GLUE(.,name); \
+	_FUNCTION(name)
+
 #define _INIT_GLOBAL(name) \
 	.section ".text.init.refok"; \
-	.align 2 ; \
 	.globl name; \
 	.globl GLUE(.,name); \
-	.section ".opd","aw"; \
-name: \
-	.quad GLUE(.,name); \
-	.quad .TOC.@tocbase; \
-	.quad 0; \
-	.previous; \
-	.type GLUE(.,name),@function; \
-GLUE(.,name):
+	_FUNCTION(name)
+
+#define _HEAD_GLOBAL(name) \
+	.section ".text.head"; \
+	.globl name; \
+	.globl GLUE(.,name); \
+	_FUNCTION(name)
 
 #define _KPROBE(name) \
 	.section ".kprobes.text","a"; \
-	.align 2 ; \
 	.globl name; \
 	.globl GLUE(.,name); \
-	.section ".opd","aw"; \
-name: \
-	.quad GLUE(.,name); \
-	.quad .TOC.@tocbase; \
-	.quad 0; \
-	.previous; \
-	.type GLUE(.,name),@function; \
-GLUE(.,name):
+	_FUNCTION(name)
 
 #define _STATIC(name) \
 	.section ".text"; \
-	.align 2 ; \
-	.section ".opd","aw"; \
-name: \
-	.quad GLUE(.,name); \
-	.quad .TOC.@tocbase; \
-	.quad 0; \
-	.previous; \
-	.type GLUE(.,name),@function; \
-GLUE(.,name):
+	_FUNCTION(name)
 
 #define _INIT_STATIC(name) \
 	.section ".text.init.refok"; \
-	.align 2 ; \
-	.section ".opd","aw"; \
-name: \
-	.quad GLUE(.,name); \
-	.quad .TOC.@tocbase; \
-	.quad 0; \
-	.previous; \
-	.type GLUE(.,name),@function; \
-GLUE(.,name):
+	_FUNCTION(name)
+
+#define _HEAD_STATIC(name) \
+	.section ".text.head"; \
+	_FUNCTION(name)
 
 #else /* 32-bit */
 

[2/3] powerpc: head_64.S: put irq vectors in .text.head

[Milton Miller]

We rely on careful placement of the start of the kernel, and use org from
the start of our image to line up the exception handlers.

When building a large kernel, the linker will insert stubs to call from one
toc to the next, breaking our careful placement.  In addition, kexec does
not (currently) look at the entry address specified in the header but assumes
that the begining of the load segment is the entry point.

Move the first 0x8000 bytes to .text.head so it will be linked first.

Signed-off-by: Milton Miller <miltonm@bga.com>
---
diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
index ecced1e..4ad435a 100644
--- a/arch/powerpc/kernel/head_64.S
+++ b/arch/powerpc/kernel/head_64.S
@@ -21,6 +21,7 @@
  *  2 of the License, or (at your option) any later version.
  */
 
+#include <linux/init.h>
 #include <linux/threads.h>
 #include <asm/reg.h>
 #include <asm/page.h>
@@ -70,10 +71,11 @@
  *   2. The kernel is entered at system_reset_iSeries
  */
 
-	.text
+	__HEAD /* should be, but is ... */
+	.section ".text.head"
 	.globl  _stext
 _stext:
-_GLOBAL(__start)
+_HEAD_GLOBAL(__start)
 	/* NOP this out unconditionally */
 BEGIN_FTR_SECTION
 	b	.__start_initialization_multiplatform
@@ -110,7 +112,7 @@ __secondary_hold_acknowledge:
  * is relocated to physical address 0x60 before prom_init is run.
  * All of it must fit below the first exception vector at 0x100.
  */
-_GLOBAL(__secondary_hold)
+_HEAD_GLOBAL(__secondary_hold)
 	mfmsr	r24
 	ori	r24,r24,MSR_RI
 	mtmsrd	r24			/* RI on */
@@ -142,7 +144,7 @@ _GLOBAL(__secondary_hold)
 	.section ".toc","aw"
 exception_marker:
 	.tc	ID_72656773_68657265[TC],0x7265677368657265
-	.text
+	.previous
 
 /*
  * This is the start of the interrupt handlers for pSeries
@@ -614,7 +616,7 @@ unrecov_user_slb:
  * r3 is saved in paca->slb_r3
  * We assume we aren't going to take any exceptions during this procedure.
  */
-_GLOBAL(slb_miss_realmode)
+_HEAD_GLOBAL(slb_miss_realmode)
 	mflr	r10
 
 	stw	r9,PACA_EXSLB+EX_CCR(r13)	/* save CR in exc. frame */
@@ -776,7 +778,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
  * switch (ie, no lazy save of the vector registers).
  * On entry: r13 == 'current' && last_task_used_altivec != 'current'
  */
-_STATIC(load_up_altivec)
+_HEAD_STATIC(load_up_altivec)
 	mfmsr	r5			/* grab the current MSR */
 	oris	r5,r5,MSR_VEC@h
 	mtmsrd	r5			/* enable use of VMX now */
@@ -865,7 +867,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_VSX)
  * been saved already.
  * On entry: r13 == 'current' && last_task_used_vsx != 'current'
  */
-_STATIC(load_up_vsx)
+_HEAD_STATIC(load_up_vsx)
 /* Load FP and VSX registers if they haven't been done yet */
 	andi.	r5,r12,MSR_FP
 	beql+	load_up_fpu		/* skip if already loaded */
@@ -905,7 +907,7 @@ _STATIC(load_up_vsx)
  * Hash table stuff
  */
 	.align	7
-_STATIC(do_hash_page)
+_HEAD_STATIC(do_hash_page)
 	std	r3,_DAR(r1)
 	std	r4,_DSISR(r1)
 
@@ -1035,7 +1037,7 @@ do_ste_alloc:
  * We assume (DAR >> 60) == 0xc.
  */
 	.align	7
-_GLOBAL(do_stab_bolted)
+_HEAD_GLOBAL(do_stab_bolted)
 	stw	r9,PACA_EXSLB+EX_CCR(r13)	/* save CR in exc. frame */
 	std	r11,PACA_EXSLB+EX_SRR0(r13)	/* save SRR0 in exc. frame */
 
@@ -1416,7 +1418,7 @@ copy_to_here:
  * On PowerMac, secondary processors starts from the reset vector, which
  * is temporarily turned into a call to one of the functions below.
  */
-	.section ".text";
+	#.section ".text";
 	.align 2 ;
 
 	.globl	__secondary_start_pmac_0

powerpc: numa.c: always trim to lmb_end_of_DRAM

[Milton Miller]

numa_enforce_memory_limit trys to be smart and only trim the memory it
finds to lmb_end_of_DRAM when a memory limit is set.   However, the
early boot code may also limit memory when iommu=off is specified.  When
this happens, the page allocator is told of pages not in the linear mapping
and gets a fatal DSI.

Signed-off-by: Milton Miller <miltonm@bga.com>
--
Previously this patch tried to check if iommu=off was specified, but it was
requested that the check just be removed.

http://patchwork.ozlabs.org/linuxppc/patch?id=11774

diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index cf4bffb..590406c 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -472,12 +472,10 @@ static unsigned long __init numa_enforce_memory_limit(unsigned long start,
 	/*
 	 * We use lmb_end_of_DRAM() in here instead of memory_limit because
 	 * we've already adjusted it for the limit and it takes care of
-	 * having memory holes below the limit.
+	 * having memory holes below the limit.  Also, in the case of
+	 * iommu_is_off, memory_limit is not set but is implicitly enforced.
 	 */
 
-	if (! memory_limit)
-		return size;
-
 	if (start + size <= lmb_end_of_DRAM())
 		return size;
 

powerpc: pseries, cell: use cpu_thread_in_core in smp_init for of_spin_map

[Milton Miller]

Replace a left over % 2 to be a check for the primary thread and
update the comments.

Note: We assume the secondary threads are not spinning, but they are
actually spinning if we start via kexec.  This means that we call rtas to
spin up the secondary threads. If we get a bad return code from we skip
bringing the cpu online, leaving the thread spinning on the paca.  We don't
handle this case in kexec, and will leave the thread wild during another
kexec.

Signed-off-by: Milton Miller <miltonm@bga.com>
---
I added a debug print and manually verified the correct cpumask was
generated on pSeries.  I ran across the identical code in cell by
the grep of System.map.



diff --git a/arch/powerpc/platforms/cell/smp.c b/arch/powerpc/platforms/cell/smp.c
index efb3964..c0d86e1 100644
--- a/arch/powerpc/platforms/cell/smp.c
+++ b/arch/powerpc/platforms/cell/smp.c
@@ -54,8 +54,8 @@
 #endif
 
 /*
- * The primary thread of each non-boot processor is recorded here before
- * smp init.
+ * The Primary thread of each non-boot processor was started from the OF client
+ * interface by prom_hold_cpus and is spinning on secondary_hold_spinloop.
  */
 static cpumask_t of_spin_map;
 
@@ -208,11 +208,7 @@ void __init smp_init_cell(void)
 	/* Mark threads which are still spinning in hold loops. */
 	if (cpu_has_feature(CPU_FTR_SMT)) {
 		for_each_present_cpu(i) {
-			if (i % 2 == 0)
-				/*
-				 * Even-numbered logical cpus correspond to
-				 * primary threads.
-				 */
+			if (cpu_thread_in_core(i) == 0)
 				cpu_set(i, of_spin_map);
 		}
 	} else {
diff --git a/arch/powerpc/platforms/pseries/smp.c b/arch/powerpc/platforms/pseries/smp.c
index 9d8f8c8..c9337c7 100644
--- a/arch/powerpc/platforms/pseries/smp.c
+++ b/arch/powerpc/platforms/pseries/smp.c
@@ -52,8 +52,8 @@
 
 
 /*
- * The primary thread of each non-boot processor is recorded here before
- * smp init.
+ * The Primary thread of each non-boot processor was started from the OF client
+ * interface by prom_hold_cpus and is spinning on secondary_hold_spinloop.
  */
 static cpumask_t of_spin_map;
 
@@ -191,8 +191,7 @@ static void __devinit smp_pSeries_kick_cpu(int nr)
 static int smp_pSeries_cpu_bootable(unsigned int nr)
 {
 	/* Special case - we inhibit secondary thread startup
-	 * during boot if the user requests it.  Odd-numbered
-	 * cpus are assumed to be secondary threads.
+	 * during boot if the user requests it.
 	 */
 	if (system_state < SYSTEM_RUNNING &&
 	    cpu_has_feature(CPU_FTR_SMT) &&
@@ -229,11 +228,7 @@ static void __init smp_init_pseries(void)
 	/* Mark threads which are still spinning in hold loops. */
 	if (cpu_has_feature(CPU_FTR_SMT)) {
 		for_each_present_cpu(i) { 
-			if (i % 2 == 0)
-				/*
-				 * Even-numbered logical cpus correspond to
-				 * primary threads.
-				 */
+			if (cpu_thread_in_core(i) == 0)
 				cpu_set(i, of_spin_map);
 		}
 	} else {

powerpc: find and destroy possible stale kernel added properties

[Milton Miller]

Because we copy the image using the kernels virtual memory system, we
require kexec user space to avoid overwriting the static kernel image, tce
tables, and the hash table.  Kexec userspace gets this information by
reading propertys the kernel adds, but does not filter these propertys when
starting the next kernel.

When the second kernel trys to add its propertys, the export via
/proc/device-tree is hidden by the pre-existing but stale value from the
flat tree.  Kexec userspace reads the old property, allocates the new kernel
at the old kernel's end, and gets rejected by the check

Search and remove these stale properties before adding the new values.

Signed-off-by: Milton Miller <miltonm@bga.com>
---
While there is an override flag to tell kexec a minimum memory to use, that
is a crude workaround and this should be applied to stable.


diff --git a/arch/powerpc/kernel/machine_kexec_64.c b/arch/powerpc/kernel/machine_kexec_64.c
index a168514..4bd8b4f 100644
--- a/arch/powerpc/kernel/machine_kexec_64.c
+++ b/arch/powerpc/kernel/machine_kexec_64.c
@@ -312,11 +312,24 @@ static struct property kernel_end_prop = {
 static void __init export_htab_values(void)
 {
 	struct device_node *node;
+	struct property *prop;
 
 	node = of_find_node_by_path("/chosen");
 	if (!node)
 		return;
 
+	/* remove any stale propertys so ours can be found */
+	prop = of_find_property(node, kernel_end_prop.name, NULL);
+	if (prop)
+		prom_remove_property(node, prop);
+	prop = of_find_property(node, htab_base_prop.name, NULL);
+	if (prop)
+		prom_remove_property(node, prop);
+	prop = of_find_property(node, htab_size_prop.name, NULL);
+	if (prop)
+		prom_remove_property(node, prop);
+
+	/* information needed by userspace when using default_machine_kexec */
 	kernel_end = __pa(_end);
 	prom_add_property(node, &kernel_end_prop);
 

powerpc: add static and ifdef prom_strtoul and prom_memparse

[Milton Miller]

These functions should have been static, and inspection shows they
are no longer used.   (We used to parse mem= but we now defer that
to early_param).

Signed-off-by: Milton Miller <miltonm@bga.com>
---
Should we just remove them instead?

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 1ea8c8d..bf1f922 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -489,6 +489,7 @@ static int __init prom_setprop(phandle node, const char *nodename,
 	return call_prom("interpret", 1, 1, (u32)(unsigned long) cmd);
 }
 
+#if 0
 /* We can't use the standard versions because of RELOC headaches. */
 #define isxdigit(c)	(('0' <= (c) && (c) <= '9') \
 			 || ('a' <= (c) && (c) <= 'f') \
@@ -498,7 +499,7 @@ static int __init prom_setprop(phandle node, const char *nodename,
 #define islower(c)	('a' <= (c) && (c) <= 'z')
 #define toupper(c)	(islower(c) ? ((c) - 'a' + 'A') : (c))
 
-unsigned long prom_strtoul(const char *cp, const char **endp)
+static unsigned long prom_strtoul(const char *cp, const char **endp)
 {
 	unsigned long result = 0, base = 10, value;
 
@@ -523,7 +524,7 @@ unsigned long prom_strtoul(const char *cp, const char **endp)
 	return result;
 }
 
-unsigned long prom_memparse(const char *ptr, const char **retptr)
+static unsigned long prom_memparse(const char *ptr, const char **retptr)
 {
 	unsigned long ret = prom_strtoul(ptr, retptr);
 	int shift = 0;
@@ -549,6 +550,7 @@ unsigned long prom_memparse(const char *ptr, const char **retptr)
 
 	return ret;
 }
+#endif
 
 /*
  * Early parsing of the command line passed to the kernel, used for

[PATCH] spufs: correct kcalloc usage

[Milton Miller]

kcalloc is supposed to be called with the count as its first argument and
the element size as the second.

Signed-off-by: Milton Miller <miltonm@bga.com>
---
Both arguments are size_t so does not affect correctness.  This callsite is
during module_init and therefore not performance critical.  Another patch
will optimize the case when the count is variable but the size is fixed.


diff --git a/arch/powerpc/platforms/cell/spufs/sputrace.c b/arch/powerpc/platforms/cell/spufs/sputrace.c
index 8c0e957..92d20e9 100644
--- a/arch/powerpc/platforms/cell/spufs/sputrace.c
+++ b/arch/powerpc/platforms/cell/spufs/sputrace.c
@@ -196,8 +196,7 @@ static int __init sputrace_init(void)
 	struct proc_dir_entry *entry;
 	int i, error = -ENOMEM;
 
-	sputrace_log = kcalloc(sizeof(struct sputrace),
-				bufsize, GFP_KERNEL);
+	sputrace_log = kcalloc(bufsize, sizeof(struct sputrace), GFP_KERNEL);
 	if (!sputrace_log)
 		goto out;
 

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Hans de Goede]

Milton Miller wrote:
> After the following patch to mark the isa region busy and applying a few
> patches[1], I was able to kexec boot into an all-yes-config kernel from 
> linux-next, with the following KCONFIG_ALLCONFIG file:
> 
> # CONFIG_KALLSYMS_EXTRA_PASS is not set
> # CONFIG_CRASH_DUMP is not set
> CONFIG_PPC_EARLY_DEBUG_RTAS_CONSOLE=y
> # CONFIG_NSC_FIR is not set
> # CONFIG_SMC_IRCC_FIR is not set
> # CONFIG_ALI_FIR is not set
> # CONFIG_TCG_NSC is not set
> # CONFIG_TCG_ATMEL is not set
> # CONFIG_SENSORS_F71805F is not set
> # CONFIG_SENSORS_F71882FG is not set
> # CONFIG_SENSORS_F75375S is not set
> # CONFIG_SENSORS_IT87 is not set
> # CONFIG_SENSORS_PC87360 is not set
> # CONFIG_SENSORS_PC87427 is not set
> # CONFIG_SENSORS_DME1737 is not set
> # CONFIG_SENSORS_SMSC47M1 is not set
> # CONFIG_SENSORS_SMSC47B397 is not set
> # CONFIG_SENSORS_VT1211 is not set
> # CONFIG_SENSORS_W83627HF is not set
> # CONFIG_SENSORS_W83627EHF is not set
> 
> While the first two might not be required, and the third is just
> selecting the right platform, it would be nice to get the drivers
> to play as well as the rest of the kernel.
> 
> The drivers all are for super-io chips, either irda/FIR drivers or hwmon,
> and poke at isa ports without checking request_region.
> 

Erm,

The superio sensor drivers only poke the superio chip registers without request 
region during the probe phase, iow they try to detect the chip, using a widely 
document and standardized (part of isa pnp AFAIK) procedure on standardized ports.

Let me try to explain a bit about superio chips, they have 2 superio control 
registers (an index and data register) with which things like a manufacturer 
and device id can be read, besides these id registers they also have a set of 
registers with config for different logical devices. Once the id is matched, 
the driver knows which logical device config to read, reads a (different) isa 
base address + range from the logical device config, and then does a 
request_region on the region actually used by the logical device.

The superio control registers are thus a sort of pci configuration space if you 
want, doing a request_region on these is not such a good idea, as multiple 
drivers (for different logical devices within the superio device) may use 
these, so trying to gain exclusive access will lead to troubles.

I hope with this info about the problem space, that you maybe have a suggestion 
on howto fix this?

Regards,

Hans
> 

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Milton Miller]

On Jul 10, 2008, at 4:33 PM, Hans de Goede wrote:

> Milton Miller wrote:
>> After the following patch to mark the isa region busy and applying a 
>> few
>> patches[1], I was able to kexec boot into an all-yes-config kernel 
>> from linux-next, with the following KCONFIG_ALLCONFIG file:
...
>> While the first two might not be required, and the third is just
>> selecting the right platform, it would be nice to get the drivers
>> to play as well as the rest of the kernel.
>> The drivers all are for super-io chips, either irda/FIR drivers or 
>> hwmon,
>> and poke at isa ports without checking request_region.
>
> Erm,
>
> The superio sensor drivers only poke the superio chip registers 
> without request region during the probe phase, iow they try to detect 
> the chip, using a widely document and standardized (part of isa pnp 
> AFAIK) procedure on standardized ports.
>
> Let me try to explain a bit about superio chips, they have 2 superio 
> control registers (an index and data register) with which things like 
> a manufacturer and device id can be read, besides these id registers 
> they also have a set of registers with config for different logical 
> devices. Once the id is matched, the driver knows which logical device 
> config to read, reads a (different) isa base address + range from the 
> logical device config, and then does a request_region on the region 
> actually used by the logical device.
>
> The superio control registers are thus a sort of pci configuration 
> space if you want, doing a request_region on these is not such a good 
> idea, as multiple drivers (for different logical devices within the 
> superio device) may use these, so trying to gain exclusive access will 
> lead to troubles.
>
> I hope with this info about the problem space, that you maybe have a 
> suggestion on howto fix this?
>

My point is that they are probing without even knowing that a such a IO 
range exists.

These are the only drivers left in the tree that still do this.  (At 
least that are not
blocked on a powerpc allyesconfig for 64-bit, !CONFIG_VIRT_TO_BUS).

One could make a superio driver, and create sub-devices for the IR, 
I2C, floppy, parallel, etc
nodes.

I would even accept a check_region (horrors!), it would impove the 
situation.

But most other drivers do this by request_region, probe, then release 
the region afterwards.

Besides, one could argue the superio region should be requested while 
probing, to prevent other cpus from poking at the super io chip at the 
same time.   Think of it as missing locking.

milton

Re: [PATCH] spufs: correct kcalloc usage

[Jeremy Kerr]

Milton,

> kcalloc is supposed to be called with the count as its first argument
> and the element size as the second.

Thanks, applied.

Cheers,


Jeremy

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Jean Delvare]

Hi Hans, hi Milton,

On Thu, 10 Jul 2008 16:51:33 -0500, Milton Miller wrote:
> 
> On Jul 10, 2008, at 4:33 PM, Hans de Goede wrote:
> 
> > Milton Miller wrote:
> >> After the following patch to mark the isa region busy and applying a 
> >> few
> >> patches[1], I was able to kexec boot into an all-yes-config kernel 
> >> from linux-next, with the following KCONFIG_ALLCONFIG file:
> ...
> >> While the first two might not be required, and the third is just
> >> selecting the right platform, it would be nice to get the drivers
> >> to play as well as the rest of the kernel.
> >> The drivers all are for super-io chips, either irda/FIR drivers or 
> >> hwmon,
> >> and poke at isa ports without checking request_region.
> >
> > Erm,
> >
> > The superio sensor drivers only poke the superio chip registers 
> > without request region during the probe phase, iow they try to detect 
> > the chip, using a widely document and standardized (part of isa pnp 
> > AFAIK) procedure on standardized ports.
> >
> > Let me try to explain a bit about superio chips, they have 2 superio 
> > control registers (an index and data register) with which things like 
> > a manufacturer and device id can be read, besides these id registers 
> > they also have a set of registers with config for different logical 
> > devices. Once the id is matched, the driver knows which logical device 
> > config to read, reads a (different) isa base address + range from the 
> > logical device config, and then does a request_region on the region 
> > actually used by the logical device.
> >
> > The superio control registers are thus a sort of pci configuration 
> > space if you want, doing a request_region on these is not such a good 
> > idea, as multiple drivers (for different logical devices within the 
> > superio device) may use these, so trying to gain exclusive access will 
> > lead to troubles.
> >
> > I hope with this info about the problem space, that you maybe have a 
> > suggestion on howto fix this?
> 
> My point is that they are probing without even knowing that a such a IO 
> range exists.
> 
> These are the only drivers left in the tree that still do this.  (At 
> least that are not
> blocked on a powerpc allyesconfig for 64-bit, !CONFIG_VIRT_TO_BUS).

I guess we could make all these drivers depend on X86, I suspect that
these chips are only used in PCs?

> One could make a superio driver, and create sub-devices for the IR, 
> I2C, floppy, parallel, etc
> nodes.

There have been proposals to do this, and this would indeed be a very
good idea, but unfortunately nobody took the time to implement this
properly, push it upstream and volunteer to maintain it. The problem is
that you don't need just a "driver", but a new subsystem, that needs to
be designed and maintained.

> I would even accept a check_region (horrors!), it would impove the 
> situation.
> 
> But most other drivers do this by request_region, probe, then release 
> the region afterwards.

I agree that the hwmon drivers should do this, as long as no superio
subsystem exist. This should solve the problem at hand and might even
help spot bad drivers which use the configuration space for longer than
initialization time.

> Besides, one could argue the superio region should be requested while 
> probing, to prevent other cpus from poking at the super io chip at the 
> same time.   Think of it as missing locking.

Agreed.

Milton, care to submit a patch fixing the affected hwmon drivers?

Thanks,
-- 
Jean Delvare

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans, hi Milton,
> 

<snip>

>> One could make a superio driver, and create sub-devices for the IR, 
>> I2C, floppy, parallel, etc
>> nodes.
> 
> There have been proposals to do this, and this would indeed be a very
> good idea, but unfortunately nobody took the time to implement this
> properly, push it upstream and volunteer to maintain it. The problem is
> that you don't need just a "driver", but a new subsystem, that needs to
> be designed and maintained.
> 

Well, I believe there have been some lightweight superio locking coordinator 
patches been floating around on the lm_sensors list, and I have reviewed them 
and then a new version was done with my issues fixed.

I kinda liked the proposed solution there, it was quite simple, moved all the 
generic superio stuff into generic superio code, and added locking for super io 
access from multiple drivers, what ever happened to those patches?

If were to start using those, we could actually do a request region and then 
never release it, as things should be.

Regards,

Hans

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Jean Delvare]

On Fri, 11 Jul 2008 09:27:26 +0200, Hans de Goede wrote:
> Jean Delvare wrote:
> > Hi Hans, hi Milton,
> > 
> 
> <snip>
> 
> >> One could make a superio driver, and create sub-devices for the IR, 
> >> I2C, floppy, parallel, etc
> >> nodes.
> > 
> > There have been proposals to do this, and this would indeed be a very
> > good idea, but unfortunately nobody took the time to implement this
> > properly, push it upstream and volunteer to maintain it. The problem is
> > that you don't need just a "driver", but a new subsystem, that needs to
> > be designed and maintained.
> 
> Well, I believe there have been some lightweight superio locking coordinator 
> patches been floating around on the lm_sensors list, and I have reviewed them 
> and then a new version was done with my issues fixed.
> 
> I kinda liked the proposed solution there, it was quite simple, moved all the 
> generic superio stuff into generic superio code, and added locking for super io 
> access from multiple drivers, what ever happened to those patches?

As far as I know, nothing, and this is the problem. Somebody needs to
step up and call him/herself the maintainer of the new code, and push
it upstream and convert all the drivers (hwmon, watchdog, parallel
port...) to make use of it. And I am not the one to do this, I am busy
enough as is with i2c and hwmon.

> If were to start using those, we could actually do a request region and then 
> never release it, as things should be.

Yes, if we have a superio access coordinator, it can request the region
and not release it. But as long as we don't have that, I agree with
Milton that the individual drivers should temporarily request the
Super-I/O region before accessing it.

-- 
Jean Delvare

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Hans de Goede]

Jean Delvare wrote:
> On Fri, 11 Jul 2008 09:27:26 +0200, Hans de Goede wrote:
>> Jean Delvare wrote:
>>> Hi Hans, hi Milton,
>>>
>> <snip>
>>
>>>> One could make a superio driver, and create sub-devices for the IR, 
>>>> I2C, floppy, parallel, etc
>>>> nodes.
>>> There have been proposals to do this, and this would indeed be a very
>>> good idea, but unfortunately nobody took the time to implement this
>>> properly, push it upstream and volunteer to maintain it. The problem is
>>> that you don't need just a "driver", but a new subsystem, that needs to
>>> be designed and maintained.
>> Well, I believe there have been some lightweight superio locking coordinator 
>> patches been floating around on the lm_sensors list, and I have reviewed them 
>> and then a new version was done with my issues fixed.
>>
>> I kinda liked the proposed solution there, it was quite simple, moved all the 
>> generic superio stuff into generic superio code, and added locking for super io 
>> access from multiple drivers, what ever happened to those patches?
> 
> As far as I know, nothing, and this is the problem. Somebody needs to
> step up and call him/herself the maintainer of the new code, and push
> it upstream and convert all the drivers (hwmon, watchdog, parallel
> port...) to make use of it. And I am not the one to do this, I am busy
> enough as is with i2c and hwmon.
>

Ok, I've mailed Jim Cromie, the author of the superio access patches 
from end 2007 and told him we're still interested in this and asked him 
if he is willing to drive this forward.

Regards,

Hans

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[David Hubbard]

Hi Hans, Milton, Jean,

On Sun, Jul 13, 2008 at 12:31 AM, Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
> Jean Delvare wrote:
>> On Fri, 11 Jul 2008 09:27:26 +0200, Hans de Goede wrote:
>>> Jean Delvare wrote:
>>>> Hi Hans, hi Milton,
>>>>
>>> <snip>
>>>
>>>>> One could make a superio driver, and create sub-devices for the IR,
>>>>> I2C, floppy, parallel, etc
>>>>> nodes.
>>>> There have been proposals to do this, and this would indeed be a very
>>>> good idea, but unfortunately nobody took the time to implement this
>>>> properly, push it upstream and volunteer to maintain it. The problem is
>>>> that you don't need just a "driver", but a new subsystem, that needs to
>>>> be designed and maintained.
>>> Well, I believe there have been some lightweight superio locking coordinator
>>> patches been floating around on the lm_sensors list, and I have reviewed them
>>> and then a new version was done with my issues fixed.
>>>
>>> I kinda liked the proposed solution there, it was quite simple, moved all the
>>> generic superio stuff into generic superio code, and added locking for super io
>>> access from multiple drivers, what ever happened to those patches?
>>
>> As far as I know, nothing, and this is the problem. Somebody needs to
>> step up and call him/herself the maintainer of the new code, and push
>> it upstream and convert all the drivers (hwmon, watchdog, parallel
>> port...) to make use of it. And I am not the one to do this, I am busy
>> enough as is with i2c and hwmon.
>>
>
> Ok, I've mailed Jim Cromie, the author of the superio access patches
> from end 2007 and told him we're still interested in this and asked him
> if he is willing to drive this forward.

I propose writing a subsystem driver. (Is that properly called "The
SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
put together some code and submit it for review, and maintain it.

Some hwmon chips have odd / unique probe sequences. IMHO this is where
the design needs to be inspected. One of those is the w83627ehf, which
Jean and Hans are familiar enough with to check my work.

Thoughts?
David

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Hans de Goede]

David Hubbard wrote:
> Hi Hans, Milton, Jean,
> 
> On Sun, Jul 13, 2008 at 12:31 AM, Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
>> Jean Delvare wrote:
>>> On Fri, 11 Jul 2008 09:27:26 +0200, Hans de Goede wrote:
>>>> Jean Delvare wrote:
>>>>> Hi Hans, hi Milton,
>>>>>
>>>> <snip>
>>>>
>>>>>> One could make a superio driver, and create sub-devices for the IR,
>>>>>> I2C, floppy, parallel, etc
>>>>>> nodes.
>>>>> There have been proposals to do this, and this would indeed be a very
>>>>> good idea, but unfortunately nobody took the time to implement this
>>>>> properly, push it upstream and volunteer to maintain it. The problem is
>>>>> that you don't need just a "driver", but a new subsystem, that needs to
>>>>> be designed and maintained.
>>>> Well, I believe there have been some lightweight superio locking coordinator
>>>> patches been floating around on the lm_sensors list, and I have reviewed them
>>>> and then a new version was done with my issues fixed.
>>>>
>>>> I kinda liked the proposed solution there, it was quite simple, moved all the
>>>> generic superio stuff into generic superio code, and added locking for super io
>>>> access from multiple drivers, what ever happened to those patches?
>>> As far as I know, nothing, and this is the problem. Somebody needs to
>>> step up and call him/herself the maintainer of the new code, and push
>>> it upstream and convert all the drivers (hwmon, watchdog, parallel
>>> port...) to make use of it. And I am not the one to do this, I am busy
>>> enough as is with i2c and hwmon.
>>>
>> Ok, I've mailed Jim Cromie, the author of the superio access patches
>> from end 2007 and told him we're still interested in this and asked him
>> if he is willing to drive this forward.
> 
> I propose writing a subsystem driver. (Is that properly called "The
> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
> put together some code and submit it for review, and maintain it.
> 
> Some hwmon chips have odd / unique probe sequences. IMHO this is where
> the design needs to be inspected. One of those is the w83627ehf, which
> Jean and Hans are familiar enough with to check my work.
> 
> Thoughts?

I'm afraid that making this a "bus" will be a bit overkill. Jim's patches are 
quite simple and seem todo the job.

Also keep in mind that most users will be platform devices which just want to 
use the superio registers to find out the baseaddress of their logical device, 
a whole bus seems overkill for this, would the hwmon driver then need to be a 
superio_driver (as well as an platform_driver) or can the bus be queried / used
without having to be a bustype-driver?

Regards,

Hans

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[David Hubbard]

Hi Hans,

>> I propose writing a subsystem driver. (Is that properly called "The
>> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
>> put together some code and submit it for review, and maintain it.
>>
>> Some hwmon chips have odd / unique probe sequences. IMHO this is where
>> the design needs to be inspected. One of those is the w83627ehf, which
>> Jean and Hans are familiar enough with to check my work.
>>
>> Thoughts?
>
> I'm afraid that making this a "bus" will be a bit overkill. Jim's patches
> are quite simple and seem todo the job.
>
> Also keep in mind that most users will be platform devices which just want
> to use the superio registers to find out the baseaddress of their logical
> device, a whole bus seems overkill for this, would the hwmon driver then
> need to be a superio_driver (as well as an platform_driver) or can the bus
> be queried / used
> without having to be a bustype-driver?

I think that's a valid point. I am willing to keep it small, but I
would prefer to follow the pattern set in other subsystems. It may be
my lack of experience in designing a subsystem showing here! What are
some alternative ways to implement it?

In other words, Jim's patches are a good start, but maybe I am
misunderstanding them. I see it as the superio-locks module, a driver
that other drivers would depend on / auto-load. Is that something
other subsystems also do?

Regards,
David

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Jean Delvare]

On Sun, 13 Jul 2008 15:26:56 -0600, David Hubbard wrote:
> Hi Hans,
> 
> >> I propose writing a subsystem driver. (Is that properly called "The
> >> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
> >> put together some code and submit it for review, and maintain it.
> >>
> >> Some hwmon chips have odd / unique probe sequences. IMHO this is where
> >> the design needs to be inspected. One of those is the w83627ehf, which
> >> Jean and Hans are familiar enough with to check my work.
> >>
> >> Thoughts?
> >
> > I'm afraid that making this a "bus" will be a bit overkill. Jim's patches
> > are quite simple and seem todo the job.
> >
> > Also keep in mind that most users will be platform devices which just want
> > to use the superio registers to find out the baseaddress of their logical
> > device, a whole bus seems overkill for this, would the hwmon driver then
> > need to be a superio_driver (as well as an platform_driver) or can the bus
> > be queried / used
> > without having to be a bustype-driver?
> 
> I think that's a valid point. I am willing to keep it small, but I
> would prefer to follow the pattern set in other subsystems. It may be
> my lack of experience in designing a subsystem showing here! What are
> some alternative ways to implement it?
> 
> In other words, Jim's patches are a good start, but maybe I am
> misunderstanding them. I see it as the superio-locks module, a driver
> that other drivers would depend on / auto-load. Is that something
> other subsystems also do?

Well, there are two approaches to the problem. The first approach
(which I think Jim took in his patches? I don't really remember) is to
simply solve the problem of concurrent I/O access to the Super-I/O
configuration ports (typically 0x2e/0x2f or 0x4e/0x4f). That would be a
simple driver requesting the ports in question and exporting an API for
other drivers to access them in a safe way.

The second approach is to make it a whole subsystem, as David is
suggesting. The Super-I/O driver would then not only request the I/O
ports, it would also identify which Super-I/O is there, and it would
create devices (in the Linux device driver model sense of the term) for
every logical device we are interested in (amongst which the hwmon
ones.) The hwmon drivers would have to be converted from platform
drivers to superio drivers.

Each approach has its advantages. The first one is rather simple and
also very generic in nature. It could be reused for other purposes. The
second one offers automatic loading of hwmon drivers, which would be
nice to have.

There's probably a middle way which would keep the simplicity of the
first approach while still allowing for driver auto-loading, without
changing the bus type of all drivers. It would probably take some
research though.

Me, I don't really care which path we choose, given that I am not the
one who will take care of it. All I have to say is that this has been
discussed several times over the past 2 years and nothing came out of
it (as far as the mainline kernel is concerned, at least) so whatever
is done now, what really matters is that it makes it into the kernel
quickly. We have some drivers missing features because of this (for
example real-time reading of VID pin values.)

This should also not prevent us from fixing the drivers now so that
they temporarily request the Super-I/O ports they are using, as Milton
suggested. Not only we don't know when we will have something better to
offer, but it might also help us find conflicts between drivers, so
that we know which drivers should make use of the future superio
driver. This could also reveal conflicts with PNP BIOS reservations,
etc. Milton, will you write a patch?

-- 
Jean Delvare

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Milton Miller]

On Jul 14, 2008, at 2:59 AM, Jean Delvare wrote:
> On Sun, 13 Jul 2008 15:26:56 -0600, David Hubbard wrote:
>> Hi Hans,
>>
>>>> I propose writing a subsystem driver. (Is that properly called "The
>>>> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
>>>> put together some code and submit it for review, and maintain it.
>>>>
>>>> Some hwmon chips have odd / unique probe sequences. IMHO this is 
>>>> where
>>>> the design needs to be inspected. One of those is the w83627ehf, 
>>>> which
>>>> Jean and Hans are familiar enough with to check my work.
>>>>
>>>> Thoughts?
>>>
>>> I'm afraid that making this a "bus" will be a bit overkill. Jim's 
>>> patches
>>> are quite simple and seem todo the job.
>>>
>>> Also keep in mind that most users will be platform devices which 
>>> just want
>>> to use the superio registers to find out the baseaddress of their 
>>> logical
>>> device, a whole bus seems overkill for this, would the hwmon driver 
>>> then
>>> need to be a superio_driver (as well as an platform_driver) or can 
>>> the bus
>>> be queried / used
>>> without having to be a bustype-driver?
>>
>> I think that's a valid point. I am willing to keep it small, but I
>> would prefer to follow the pattern set in other subsystems. It may be
>> my lack of experience in designing a subsystem showing here! What are
>> some alternative ways to implement it?
>>
>> In other words, Jim's patches are a good start, but maybe I am
>> misunderstanding them. I see it as the superio-locks module, a driver
>> that other drivers would depend on / auto-load. Is that something
>> other subsystems also do?
>
> Well, there are two approaches to the problem. The first approach
> (which I think Jim took in his patches? I don't really remember) is to
> simply solve the problem of concurrent I/O access to the Super-I/O
> configuration ports (typically 0x2e/0x2f or 0x4e/0x4f). That would be a
> simple driver requesting the ports in question and exporting an API for
> other drivers to access them in a safe way.
>
> The second approach is to make it a whole subsystem, as David is
> suggesting. The Super-I/O driver would then not only request the I/O
> ports, it would also identify which Super-I/O is there, and it would
> create devices (in the Linux device driver model sense of the term) for
> every logical device we are interested in (amongst which the hwmon
> ones.) The hwmon drivers would have to be converted from platform
> drivers to superio drivers.
>
> Each approach has its advantages. The first one is rather simple and
> also very generic in nature. It could be reused for other purposes. The
> second one offers automatic loading of hwmon drivers, which would be
> nice to have.
>
> There's probably a middle way which would keep the simplicity of the
> first approach while still allowing for driver auto-loading, without
> changing the bus type of all drivers. It would probably take some
> research though.

I haven't done the research, but it might be keep superio as
a platform driver, and keep the clients as platform drivers.  Only
have the superio driver probe and discover the subcomponent
addresses and then create the platform devices as children
instead of having each driver create its own platform device.
(This all assumes they are all platform devices in sysfs, I have
not looked).

This is all because in the platform bus the bus driver does not
discover the addresses but relies on drivers or platform setup code.

> Me, I don't really care which path we choose, given that I am not the
> one who will take care of it. All I have to say is that this has been
> discussed several times over the past 2 years and nothing came out of
> it (as far as the mainline kernel is concerned, at least) so whatever
> is done now, what really matters is that it makes it into the kernel
> quickly. We have some drivers missing features because of this (for
> example real-time reading of VID pin values.)
>
> This should also not prevent us from fixing the drivers now so that
> they temporarily request the Super-I/O ports they are using, as Milton
> suggested. Not only we don't know when we will have something better to
> offer, but it might also help us find conflicts between drivers, so
> that we know which drivers should make use of the future superio
> driver. This could also reveal conflicts with PNP BIOS reservations,
> etc. Milton, will you write a patch?

Well, that is the second time you asked me, so I guess I should respond.

While I it is possible for me to write this patch, my schedule and
priority list predict it would not be before the merge window closes.  
In
fact, I'm not sure when it would come out.   It might be argued it could
go in early -rc, but I would fear somebody's chip will not be detected
with the additional check.  For example, the port may reserved as mother
board resources or something.  Also, I have no hardware to test any
proposed patch, so it would be compile check only.

milton

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Hans de Goede]

Milton Miller wrote:
> On Jul 14, 2008, at 2:59 AM, Jean Delvare wrote:
>> On Sun, 13 Jul 2008 15:26:56 -0600, David Hubbard wrote:
>>> Hi Hans,
>>>
>>>>> I propose writing a subsystem driver. (Is that properly called "The
>>>>> SuperIO Bus Driver"?) If no one thinks it's a really bad idea I will
>>>>> put together some code and submit it for review, and maintain it.
>>>>>
>>>>> Some hwmon chips have odd / unique probe sequences. IMHO this is 
>>>>> where
>>>>> the design needs to be inspected. One of those is the w83627ehf, 
>>>>> which
>>>>> Jean and Hans are familiar enough with to check my work.
>>>>>
>>>>> Thoughts?
>>>> I'm afraid that making this a "bus" will be a bit overkill. Jim's 
>>>> patches
>>>> are quite simple and seem todo the job.
>>>>
>>>> Also keep in mind that most users will be platform devices which 
>>>> just want
>>>> to use the superio registers to find out the baseaddress of their 
>>>> logical
>>>> device, a whole bus seems overkill for this, would the hwmon driver 
>>>> then
>>>> need to be a superio_driver (as well as an platform_driver) or can 
>>>> the bus
>>>> be queried / used
>>>> without having to be a bustype-driver?
>>> I think that's a valid point. I am willing to keep it small, but I
>>> would prefer to follow the pattern set in other subsystems. It may be
>>> my lack of experience in designing a subsystem showing here! What are
>>> some alternative ways to implement it?
>>>
>>> In other words, Jim's patches are a good start, but maybe I am
>>> misunderstanding them. I see it as the superio-locks module, a driver
>>> that other drivers would depend on / auto-load. Is that something
>>> other subsystems also do?
>> Well, there are two approaches to the problem. The first approach
>> (which I think Jim took in his patches? I don't really remember) is to
>> simply solve the problem of concurrent I/O access to the Super-I/O
>> configuration ports (typically 0x2e/0x2f or 0x4e/0x4f). That would be a
>> simple driver requesting the ports in question and exporting an API for
>> other drivers to access them in a safe way.
>>
>> The second approach is to make it a whole subsystem, as David is
>> suggesting. The Super-I/O driver would then not only request the I/O
>> ports, it would also identify which Super-I/O is there, and it would
>> create devices (in the Linux device driver model sense of the term) for
>> every logical device we are interested in (amongst which the hwmon
>> ones.) The hwmon drivers would have to be converted from platform
>> drivers to superio drivers.
>>
>> Each approach has its advantages. The first one is rather simple and
>> also very generic in nature. It could be reused for other purposes. The
>> second one offers automatic loading of hwmon drivers, which would be
>> nice to have.
>>
>> There's probably a middle way which would keep the simplicity of the
>> first approach while still allowing for driver auto-loading, without
>> changing the bus type of all drivers. It would probably take some
>> research though.
> 
> I haven't done the research, but it might be keep superio as
> a platform driver, and keep the clients as platform drivers.  Only
> have the superio driver probe and discover the subcomponent
> addresses and then create the platform devices as children
> instead of having each driver create its own platform device.
> (This all assumes they are all platform devices in sysfs, I have
> not looked).
> 
> This is all because in the platform bus the bus driver does not
> discover the addresses but relies on drivers or platform setup code.
> 

This sounds like a good plan, rather then add a new bus type add a superio 
platform driver which does superio probing and registering of platform devices 
for discovered logical devices.

This superio platform driver then needs to also export some functions of those 
few logical devices which need access to the superio registers for more then 
just finding out their own base address.

I guess that it then would be best to load this superio driver by default on 
most systems.

How does this all mix and match with isapnp, it feels to me we're doing 
somewhat the same as isapnp here.

Regards,

Hans

Re: [lm-sensors] [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[David Hubbard]

Hi Hans,

On Mon, Jul 14, 2008 at 11:30 AM, Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
> Milton Miller wrote:
>> I haven't done the research, but it might be keep superio as
>> a platform driver, and keep the clients as platform drivers.  Only
>> have the superio driver probe and discover the subcomponent
>> addresses and then create the platform devices as children
>> instead of having each driver create its own platform device.
>> (This all assumes they are all platform devices in sysfs, I have
>> not looked).
>>
>> This is all because in the platform bus the bus driver does not
>> discover the addresses but relies on drivers or platform setup code.
>>
>
> This sounds like a good plan, rather then add a new bus type add a superio
> platform driver which does superio probing and registering of platform devices
> for discovered logical devices.
>
> This superio platform driver then needs to also export some functions of those
> few logical devices which need access to the superio registers for more then
> just finding out their own base address.
>
> I guess that it then would be best to load this superio driver by default on
> most systems.
>
> How does this all mix and match with isapnp, it feels to me we're doing
> somewhat the same as isapnp here.

Is there any way to use lspci and start at the LPC bridge, then find
the SuperIO chip's IO address? What about ACPI tables? Perhaps probing
logic could look for an LPC bridge before probing certain IO addresses
even if the addresses are not in the LPC bridge config.

A superio platform driver is a good way to go -- it fits with the way
the platform bus does things. Also, Jim's patches are almost there
already.

Thanks,
David

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Jean Delvare]

Hi Milton,

On Mon, 14 Jul 2008 12:09:03 -0500, Milton Miller wrote:
> On Jul 14, 2008, at 2:59 AM, Jean Delvare wrote:
> > Well, there are two approaches to the problem. The first approach
> > (which I think Jim took in his patches? I don't really remember) is to
> > simply solve the problem of concurrent I/O access to the Super-I/O
> > configuration ports (typically 0x2e/0x2f or 0x4e/0x4f). That would be a
> > simple driver requesting the ports in question and exporting an API for
> > other drivers to access them in a safe way.
> >
> > The second approach is to make it a whole subsystem, as David is
> > suggesting. The Super-I/O driver would then not only request the I/O
> > ports, it would also identify which Super-I/O is there, and it would
> > create devices (in the Linux device driver model sense of the term) for
> > every logical device we are interested in (amongst which the hwmon
> > ones.) The hwmon drivers would have to be converted from platform
> > drivers to superio drivers.
> >
> > Each approach has its advantages. The first one is rather simple and
> > also very generic in nature. It could be reused for other purposes. The
> > second one offers automatic loading of hwmon drivers, which would be
> > nice to have.
> >
> > There's probably a middle way which would keep the simplicity of the
> > first approach while still allowing for driver auto-loading, without
> > changing the bus type of all drivers. It would probably take some
> > research though.
> 
> I haven't done the research, but it might be keep superio as
> a platform driver, and keep the clients as platform drivers.  Only
> have the superio driver probe and discover the subcomponent
> addresses and then create the platform devices as children
> instead of having each driver create its own platform device.
> (This all assumes they are all platform devices in sysfs, I have
> not looked).
> 
> This is all because in the platform bus the bus driver does not
> discover the addresses but relies on drivers or platform setup code.

That's more or less what I had in mind, yes.

> > (...) Milton, will you write a patch?
> 
> Well, that is the second time you asked me, so I guess I should respond.
> 
> While I it is possible for me to write this patch, my schedule and
> priority list predict it would not be before the merge window closes.  
> In fact, I'm not sure when it would come out.  It might be argued it
> could go in early -rc, but I would fear somebody's chip will not be detected
> with the additional check.  For example, the port may reserved as mother
> board resources or something.

I really don't see this as something for kernel 2.6.27, it's too late
already. It doesn't fix any actual problem anyway (none that can be
fixed by not loading drivers you don't need, at least.) That would be
for 2.6.28, so we have plenty of time to test the changes and ensure
they do not break anything. As you are the one who reported the issue
as something that was bothering you personally, I expected you to also
spend some time trying to address it.

> (...) Also, I have no hardware to test any
> proposed patch, so it would be compile check only.

If you write the patches and post them to the lm-sensors list, I am
certain that you will find several volunteers for testing. Me, I can
offer testing of the it87, f71805f and w83627ehf drivers.

Thanks,
-- 
Jean Delvare

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Jean Delvare]

On Mon, 14 Jul 2008 11:55:01 -0600, David Hubbard wrote:
> Hi Hans,
> 
> On Mon, Jul 14, 2008 at 11:30 AM, Hans de Goede <j.w.r.degoede@hhs.nl> wrote:
> > Milton Miller wrote:
> >> I haven't done the research, but it might be keep superio as
> >> a platform driver, and keep the clients as platform drivers.  Only
> >> have the superio driver probe and discover the subcomponent
> >> addresses and then create the platform devices as children
> >> instead of having each driver create its own platform device.
> >> (This all assumes they are all platform devices in sysfs, I have
> >> not looked).
> >>
> >> This is all because in the platform bus the bus driver does not
> >> discover the addresses but relies on drivers or platform setup code.
> >
> > This sounds like a good plan, rather then add a new bus type add a superio
> > platform driver which does superio probing and registering of platform devices
> > for discovered logical devices.
> >
> > This superio platform driver then needs to also export some functions of those
> > few logical devices which need access to the superio registers for more then
> > just finding out their own base address.
> >
> > I guess that it then would be best to load this superio driver by default on
> > most systems.
> >
> > How does this all mix and match with isapnp, it feels to me we're doing
> > somewhat the same as isapnp here.
> 
> Is there any way to use lspci and start at the LPC bridge, then find
> the SuperIO chip's IO address? What about ACPI tables? Perhaps probing
> logic could look for an LPC bridge before probing certain IO addresses
> even if the addresses are not in the LPC bridge config.

I always assumed that there was no way to know in advance if a
Super-I/O (LPC) chip was present or not, let alone the exact model of
the chip. The I/O addresses are decoded by the Super-I/O chip itself,
and in general it has no relation to PCI. And I've never seen ports
0x2e/0x2f nor 0x4e/0x4f listed in /proc/ioports.

But of course if there is a way to know, we should use it. Avoiding
random access to I/O ports, even if they are relatively standard in
this case, is always good.

> A superio platform driver is a good way to go -- it fits with the way
> the platform bus does things. Also, Jim's patches are almost there
> already.

Good.

-- 
Jean Delvare

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[David Hubbard]

Hi Jean,

On Tue, Jul 15, 2008 at 2:36 AM, Jean Delvare <khali@linux-fr.org> wrote:
>> Is there any way to use lspci and start at the LPC bridge, then find
>> the SuperIO chip's IO address? What about ACPI tables? Perhaps probing
>> logic could look for an LPC bridge before probing certain IO addresses
>> even if the addresses are not in the LPC bridge config.
>
> I always assumed that there was no way to know in advance if a
> Super-I/O (LPC) chip was present or not, let alone the exact model of
> the chip. The I/O addresses are decoded by the Super-I/O chip itself,
> and in general it has no relation to PCI. And I've never seen ports
> 0x2e/0x2f nor 0x4e/0x4f listed in /proc/ioports.
>
> But of course if there is a way to know, we should use it. Avoiding
> random access to I/O ports, even if they are relatively standard in
> this case, is always good.

I looked at my lspci output and did a little researching, and I think
the only thing we can deduce is that there is an LPC bridge, so
looking for a SuperIO is a good idea. If there is no LPC bridge
listed, I can't say whether probing the ports is a good idea or not.

David

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Jean Delvare]

Hi David,

On Tue, 15 Jul 2008 09:31:29 -0600, David Hubbard wrote:
> On Tue, Jul 15, 2008 at 2:36 AM, Jean Delvare <khali@linux-fr.org> wrote:
> > I always assumed that there was no way to know in advance if a
> > Super-I/O (LPC) chip was present or not, let alone the exact model of
> > the chip. The I/O addresses are decoded by the Super-I/O chip itself,
> > and in general it has no relation to PCI. And I've never seen ports
> > 0x2e/0x2f nor 0x4e/0x4f listed in /proc/ioports.
> >
> > But of course if there is a way to know, we should use it. Avoiding
> > random access to I/O ports, even if they are relatively standard in
> > this case, is always good.
> 
> I looked at my lspci output and did a little researching, and I think
> the only thing we can deduce is that there is an LPC bridge, so
> looking for a SuperIO is a good idea. If there is no LPC bridge
> listed, I can't say whether probing the ports is a good idea or not.

Machines I have here have an "ISA bridge" PCI device. Some of them have
"LPC" in their name, but not all, and anyway the kernel only knows the
device ID, not its user-friendly name:

VIA:
00:11.0 ISA bridge: VIA Technologies, Inc. VT8237 ISA bridge [KT600/K8T800/K8T890 South]

Intel:
00:01.0 ISA bridge: Intel Corporation 82371AB/EB/MB PIIX4 ISA (rev 01)
00:1f.0 ISA bridge: Intel Corporation 82801CAM ISA Bridge (LPC) (rev 01)
00:1f.0 ISA bridge: Intel Corporation 82801EB/ER (ICH5/ICH5R) LPC Interface Bridge (rev 02)

nVidia:
00:01.0 ISA bridge: nVidia Corporation nForce2 ISA Bridge (rev a4)

One of these machines has a LPC bridge but no (detected) Super-I/O chip.

The VIA and nVidia machines do not have "LPC" in their bridge name, but
they do have a Super-I/O chip, while the first Intel machine doesn't.

As a conclusion, there is no clear relationship between the presence of
an ISA or LPC bridge and the presence of a Super-I/O chip. All we can
say is that apparently all PC systems have an ISA bridge. So indeed we
can probably make the probes conditional upon the presence of an "ISA
bridge" PCI device. That's very easy to test. In practice it might be
equivalent to making the driver x86-only.

-- 
Jean Delvare

Re: [RFC] (almost) booting allyesconfig -- please don't poke super-io without request_region

[Rene Herman]

On 16-07-08 09:46, Jean Delvare wrote:

> As a conclusion, there is no clear relationship between the presence
> of an ISA or LPC bridge and the presence of a Super-I/O chip. All we
> can say is that apparently all PC systems have an ISA bridge. So
> indeed we can probably make the probes conditional upon the presence
> of an "ISA bridge" PCI device. That's very easy to test. In practice
> it might be equivalent to making the driver x86-only.

And foregoing non (pre-) PCI. Admittedly, that might not be much of an 
issue and I don't know if "Super-I/O" is maybe even defined such as to 
make it a NON issue but I do for example remember my old 386 chipset 
providing for extended serial rates which I thought was way cool back 
then. If that's Super-I/O (or something that should/could be covered by 
the infrastructure at least) then PCI wouldn't be helpful.

Rene.