Re: [GIT PULL tip/x86/mm] xen/x86 fixes ===> fix sp5100_tco mmio checking.

Re: [GIT PULL tip/x86/mm] xen/x86 fixes ===> fix sp5100_tco mmio checking.

[Yinghai Lu]

On 03/16/2011 11:02 AM, Stefano Stabellini wrote:
> On Wed, 16 Mar 2011, Yinghai Lu wrote:
>> On 03/16/2011 07:43 AM, Stefano Stabellini wrote:
>>> actually attach the logs :)
>>>
>>> On Wed, 16 Mar 2011, Stefano Stabellini wrote:
>>>> On Fri, 11 Mar 2011, Konrad Rzeszutek Wilk wrote:
>>>>> On Fri, Mar 11, 2011 at 01:17:23PM +0000, Stefano Stabellini wrote:
>>>>>> Hello,
>>>>>> recently we had a couple of long discussions with Yinghai about boot
>>>>>> crashes on xen, related to pagetable initialization.
>>>>>> As a result we came up with three patches, two of them fix the first [1]
>>>>>> boot crash and provide a nice cleanup on native:
>>>>>
>>>>> I don't know why this is happening now, but it could be very well
>>>>> related to the build config. Smaller builds don't seem to encounter this, while
>>>>> this is a distro type build. If I use:
>>>>>
>>>>>> Stefano Stabellini (1):
>>>>>>         xen: set max_pfn_mapped to the last pfn mapped
>>>>>
>>>>> it hangs during bootup. The machine hangs during the box (no keyboard interaction)
>>>>> and I can see this in the bootup.
>>>>
>>>> Konrad sent me few other logs offline: log1 is the log of the hang and
>>>> log2 is a successful boot (reverting the problematic patch).
>>>> It looks like the SP5100 TCO WatchDog Timer Driver is using ioremap on
>>>> an address (0xb8fe00) that belongs to the memory range used for the
>>>> pagetable (0x9fc000-0xf43fff).
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000] found SMP MP-table at [ffff8800000ff780] ff780
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]     memblock_x86_reserve_range: [0x000ff780-0x000ff78f]   * MP-table mpf
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]     memblock_x86_reserve_range: [0x000fd240-0x000fd423]   * MP-table mpc
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]     memblock_x86_reserve_range: [0x01cfd000-0x01d1c0e4]              BRK
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000] MEMBLOCK configuration:
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]  memory size = 0x23fe39000
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]  memory.cnt  = 0x3
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]  memory[0x0]      [0x00000000010000-0x0000000009afff], 0x8b000 bytes
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]  memory[0x1]      [0x00000000100000-0x000000bffaffff], 0xbfeb0000 bytes
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]  memory[0x2]      [0x00000100000000-0x0000027fefdfff], 0x17fefe000 bytes
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved.cnt  = 0x5
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x0]    [0x000000000fd240-0x000000000fd423], 0x1e4 bytes
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x1]    [0x000000000ff780-0x000000000ff78f], 0x10 bytes
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x2]    [0x00000001000000-0x00000001d1c0e4], 0xd1c0e5 bytes
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x3]    [0x00000001e33000-0x00000016a36fff], 0x14c04000 bytes
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x4]    [0x000001f0f7e000-0x0000027fefdfff], 0x8ef80000 bytes
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000] Scanning 0 areas for low memory corruption
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]     memblock_x86_reserve_range: [0x00099000-0x0009afff]       TRAMPOLINE
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]     memblock_x86_reserve_range: [0x00095000-0x00098fff]      ACPI WAKEUP
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000] init_memory_mapping: 0000000000000000-00000000bffb0000
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000] DEBUG find_early_table_space: _text=1000000 _end=1e33000 pgtable_start=9fc000 pgtable_end=9fc000
>>
>> Mar 15 16:09:04 phenom kernel: [    0.000000]     memblock_x86_reserve_range: [0x009fc000-0x00f43fff]          PGTABLE
>>
>> e820 said that range is ram and usable. so it is right for memblock to use it.
>>
>> why TCO watchdog try to use ioremap with RAM?  BIOS put wrong mmio in that BAR?
>>
>> could do some sanitary check in that driver.
>>
>
> Yeah, I think the max_pfn_mapped patch might be exposing bugs in the
> drivers.
> Do you remember this patch:
>
> https://lkml.org/lkml/2011/2/4/60
>
> would you be happy with it as a safer alternative?

we should fix tco driver

Mar 15 16:09:04 phenom kernel: [    9.148536] SP5100 TCO timer: SP5100 TCO WatchDog Timer Driver v0.01

Mar 15 16:09:04 phenom kernel: [    9.148628] DEBUG __ioremap_caller WARNING address=b8fe00 size=8 valid=1 reserved=1

so BIOS program wrong MMIO info.

need some checking in that driver like

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 8083728..2fac643 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -42,6 +42,7 @@
  #define PFX TCO_MODULE_NAME ": "
  
  /* internal variables */
+static u32 tcobase_phys;
  static void __iomem *tcobase;
  static unsigned int pm_iobase;
  static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
@@ -305,6 +306,12 @@ static unsigned char __devinit sp5100_tco_setupdevice(void)
  	/* Low three bits of BASE0 are reserved. */
  	val = val << 8 | (inb(SP5100_IO_PM_DATA_REG) & 0xf8);
  
+	if (request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE, "SP5100 TCO")) {
+		printk(KERN_ERR PFX "mmio address 0x%04x already in use\n", val);
+		goto unreg_region;
+	}
+	tcobase_phys = val;
+
  	tcobase = ioremap(val, SP5100_WDT_MEM_MAP_SIZE);
  	if (tcobase == 0) {
  		printk(KERN_ERR PFX "failed to get tcobase address\n");
@@ -414,6 +421,7 @@ static void __devexit sp5100_tco_cleanup(void)
  	/* Deregister */
  	misc_deregister(&sp5100_tco_miscdev);
  	iounmap(tcobase);
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
  	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
  }
  

Re: [GIT PULL tip/x86/mm] xen/x86 fixes ===> fix sp5100_tco mmio checking.

[Mike Waychison]

On Wed, Mar 16, 2011 at 1:45 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On 03/16/2011 11:02 AM, Stefano Stabellini wrote:
>>
>> On Wed, 16 Mar 2011, Yinghai Lu wrote:
>>>
>>> On 03/16/2011 07:43 AM, Stefano Stabellini wrote:
>>>>
>>>> actually attach the logs :)
>>>>
>>>> On Wed, 16 Mar 2011, Stefano Stabellini wrote:
>>>>>
>>>>> On Fri, 11 Mar 2011, Konrad Rzeszutek Wilk wrote:
>>>>>>
>>>>>> On Fri, Mar 11, 2011 at 01:17:23PM +0000, Stefano Stabellini wrote:
>>>>>>>
>>>>>>> Hello,
>>>>>>> recently we had a couple of long discussions with Yinghai about boot
>>>>>>> crashes on xen, related to pagetable initialization.
>>>>>>> As a result we came up with three patches, two of them fix the first
>>>>>>> [1]
>>>>>>> boot crash and provide a nice cleanup on native:
>>>>>>
>>>>>> I don't know why this is happening now, but it could be very well
>>>>>> related to the build config. Smaller builds don't seem to encounter
>>>>>> this, while
>>>>>> this is a distro type build. If I use:
>>>>>>
>>>>>>> Stefano Stabellini (1):
>>>>>>>        xen: set max_pfn_mapped to the last pfn mapped
>>>>>>
>>>>>> it hangs during bootup. The machine hangs during the box (no keyboard
>>>>>> interaction)
>>>>>> and I can see this in the bootup.
>>>>>
>>>>> Konrad sent me few other logs offline: log1 is the log of the hang and
>>>>> log2 is a successful boot (reverting the problematic patch).
>>>>> It looks like the SP5100 TCO WatchDog Timer Driver is using ioremap on
>>>>> an address (0xb8fe00) that belongs to the memory range used for the
>>>>> pagetable (0x9fc000-0xf43fff).
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000] found SMP MP-table at
>>> [ffff8800000ff780] ff780
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]
>>> memblock_x86_reserve_range: [0x000ff780-0x000ff78f]   * MP-table mpf
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]
>>> memblock_x86_reserve_range: [0x000fd240-0x000fd423]   * MP-table mpc
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]
>>> memblock_x86_reserve_range: [0x01cfd000-0x01d1c0e4]              BRK
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000] MEMBLOCK configuration:
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]  memory size = 0x23fe39000
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]  memory.cnt  = 0x3
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]  memory[0x0]
>>>  [0x00000000010000-0x0000000009afff], 0x8b000 bytes
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]  memory[0x1]
>>>  [0x00000000100000-0x000000bffaffff], 0xbfeb0000 bytes
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]  memory[0x2]
>>>  [0x00000100000000-0x0000027fefdfff], 0x17fefe000 bytes
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved.cnt  = 0x5
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x0]
>>>  [0x000000000fd240-0x000000000fd423], 0x1e4 bytes
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x1]
>>>  [0x000000000ff780-0x000000000ff78f], 0x10 bytes
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x2]
>>>  [0x00000001000000-0x00000001d1c0e4], 0xd1c0e5 bytes
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x3]
>>>  [0x00000001e33000-0x00000016a36fff], 0x14c04000 bytes
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]  reserved[0x4]
>>>  [0x000001f0f7e000-0x0000027fefdfff], 0x8ef80000 bytes
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000] Scanning 0 areas for low
>>> memory corruption
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]
>>> memblock_x86_reserve_range: [0x00099000-0x0009afff]       TRAMPOLINE
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]
>>> memblock_x86_reserve_range: [0x00095000-0x00098fff]      ACPI WAKEUP
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000] init_memory_mapping:
>>> 0000000000000000-00000000bffb0000
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000] DEBUG
>>> find_early_table_space: _text=1000000 _end=1e33000 pgtable_start=9fc000
>>> pgtable_end=9fc000
>>>
>>> Mar 15 16:09:04 phenom kernel: [    0.000000]
>>> memblock_x86_reserve_range: [0x009fc000-0x00f43fff]          PGTABLE
>>>
>>> e820 said that range is ram and usable. so it is right for memblock to
>>> use it.
>>>
>>> why TCO watchdog try to use ioremap with RAM?  BIOS put wrong mmio in
>>> that BAR?
>>>
>>> could do some sanitary check in that driver.
>>>
>>
>> Yeah, I think the max_pfn_mapped patch might be exposing bugs in the
>> drivers.
>> Do you remember this patch:
>>
>> https://lkml.org/lkml/2011/2/4/60
>>
>> would you be happy with it as a safer alternative?
>
> we should fix tco driver
>
> Mar 15 16:09:04 phenom kernel: [    9.148536] SP5100 TCO timer: SP5100 TCO
> WatchDog Timer Driver v0.01
>
> Mar 15 16:09:04 phenom kernel: [    9.148628] DEBUG __ioremap_caller WARNING
> address=b8fe00 size=8 valid=1 reserved=1
>
> so BIOS program wrong MMIO info.
>
> need some checking in that driver like
>
> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> index 8083728..2fac643 100644
> --- a/drivers/watchdog/sp5100_tco.c
> +++ b/drivers/watchdog/sp5100_tco.c
> @@ -42,6 +42,7 @@
>  #define PFX TCO_MODULE_NAME ": "
>  /* internal variables */
> +static u32 tcobase_phys;
>  static void __iomem *tcobase;
>  static unsigned int pm_iobase;
>  static DEFINE_SPINLOCK(tco_lock);      /* Guards the hardware */
> @@ -305,6 +306,12 @@ static unsigned char __devinit
> sp5100_tco_setupdevice(void)
>        /* Low three bits of BASE0 are reserved. */
>        val = val << 8 | (inb(SP5100_IO_PM_DATA_REG) & 0xf8);
>  +      if (request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE,
> "SP5100 TCO")) {
> +               printk(KERN_ERR PFX "mmio address 0x%04x already in use\n",
> val);
> +               goto unreg_region;
> +       }
> +       tcobase_phys = val;
> +
>        tcobase = ioremap(val, SP5100_WDT_MEM_MAP_SIZE);
>        if (tcobase == 0) {

Needs a release_mem_region() in this path.  Otherwise this looks fine.

>                printk(KERN_ERR PFX "failed to get tcobase address\n");
> @@ -414,6 +421,7 @@ static void __devexit sp5100_tco_cleanup(void)
>        /* Deregister */
>        misc_deregister(&sp5100_tco_miscdev);
>        iounmap(tcobase);
> +       release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
>        release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
>  }
>

[PATCH] watchdog, SP5100: Check if firmware has set correct value in tcobase.

[Yinghai Lu]

Stefano found SP5100 TCO watchdog driver using wrong address.

[    9.148536] SP5100 TCO timer: SP5100 TCO WatchDog Timer Driver v0.01
[    9.148628] DEBUG __ioremap_caller WARNING address=b8fe00 size=8 valid=1 reserved=1

and e820 said that range is RAM.

We should check if we can use that reading out. BIOS could just program wrong address there.

-v2: Mike pointed out one path need one release.

Reported-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by:Yinghai Lu <yinghai@kernel.org>
Acked-by: Mike Waychison <mikew@google.com>

---
  drivers/watchdog/sp5100_tco.c |   14 ++++++++++++--
  1 file changed, 12 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/watchdog/sp5100_tco.c
===================================================================
--- linux-2.6.orig/drivers/watchdog/sp5100_tco.c
+++ linux-2.6/drivers/watchdog/sp5100_tco.c
@@ -42,6 +42,7 @@
  #define PFX TCO_MODULE_NAME ": "
  
  /* internal variables */
+static u32 tcobase_phys;
  static void __iomem *tcobase;
  static unsigned int pm_iobase;
  static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
@@ -305,10 +306,16 @@ static unsigned char __devinit sp5100_tc
  	/* Low three bits of BASE0 are reserved. */
  	val = val << 8 | (inb(SP5100_IO_PM_DATA_REG) & 0xf8);
  
+	if (request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE, "SP5100 TCO")) {
+		printk(KERN_ERR PFX "mmio address 0x%04x already in use\n", val);
+		goto unreg_region;
+	}
+	tcobase_phys = val;
+
  	tcobase = ioremap(val, SP5100_WDT_MEM_MAP_SIZE);
  	if (tcobase == 0) {
  		printk(KERN_ERR PFX "failed to get tcobase address\n");
-		goto unreg_region;
+		goto unreg_mem_region;
  	}
  
  	/* Enable watchdog decode bit */
@@ -346,7 +353,8 @@ static unsigned char __devinit sp5100_tc
  	/* Done */
  	return 1;
  
-	iounmap(tcobase);
+unreg_mem_region:
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
  unreg_region:
  	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
  exit:
@@ -401,6 +409,7 @@ static int __devinit sp5100_tco_init(str
  
  exit:
  	iounmap(tcobase);
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
  	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
  	return ret;
  }
@@ -414,6 +423,7 @@ static void __devexit sp5100_tco_cleanup
  	/* Deregister */
  	misc_deregister(&sp5100_tco_miscdev);
  	iounmap(tcobase);
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
  	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
  }
  

Re: [PATCH] watchdog, SP5100: Check if firmware has set correct value in tcobase.

[Konrad Rzeszutek Wilk]

On Wed, Mar 16, 2011 at 02:18:17PM -0700, Yinghai Lu wrote:
> 
> 
> Stefano found SP5100 TCO watchdog driver using wrong address.
> 
> [    9.148536] SP5100 TCO timer: SP5100 TCO WatchDog Timer Driver v0.01
> [    9.148628] DEBUG __ioremap_caller WARNING address=b8fe00 size=8 valid=1 reserved=1
> 
> and e820 said that range is RAM.
> 
> We should check if we can use that reading out. BIOS could just program wrong address there.
> 
> -v2: Mike pointed out one path need one release.
> 
> Reported-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by:Yinghai Lu <yinghai@kernel.org>
> Acked-by: Mike Waychison <mikew@google.com>

Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Stefano,

this fixes my bootup issues with your:
xen: set max_pfn_mapped to the last pfn mapped
patch. Will try the full patchset tomorrow.

[PATCH -v3] watchdog, SP5100: Check if firmware has set correct value in tcobase.

[Yinghai Lu]

Stefano found SP5100 TCO watchdog driver using wrong address.

[    9.148536] SP5100 TCO timer: SP5100 TCO WatchDog Timer Driver v0.01
[    9.148628] DEBUG __ioremap_caller WARNING address=b8fe00 size=8 valid=1 reserved=1

and e820 said that range is RAM.

We should check if we can use that reading out. BIOS could just program wrong address there.

-v2: Mike pointed out one path need one release.
-v3: corrected logic checking with request_mem_region_exclusive()
      Found by Konrad.


Reported-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by:Yinghai Lu <yinghai@kernel.org>
Acked-by: Mike Waychison <mikew@google.com>
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
  drivers/watchdog/sp5100_tco.c |   14 ++++++++++++--
  1 file changed, 12 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/watchdog/sp5100_tco.c
===================================================================
--- linux-2.6.orig/drivers/watchdog/sp5100_tco.c
+++ linux-2.6/drivers/watchdog/sp5100_tco.c
@@ -42,6 +42,7 @@
  #define PFX TCO_MODULE_NAME ": "
  
  /* internal variables */
+static u32 tcobase_phys;
  static void __iomem *tcobase;
  static unsigned int pm_iobase;
  static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
@@ -305,10 +306,16 @@ static unsigned char __devinit sp5100_tc
  	/* Low three bits of BASE0 are reserved. */
  	val = val << 8 | (inb(SP5100_IO_PM_DATA_REG) & 0xf8);
  
+	if (!request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE, "SP5100 TCO")) {
+		printk(KERN_ERR PFX "mmio address 0x%04x already in use\n", val);
+		goto unreg_region;
+	}
+	tcobase_phys = val;
+
  	tcobase = ioremap(val, SP5100_WDT_MEM_MAP_SIZE);
  	if (tcobase == 0) {
  		printk(KERN_ERR PFX "failed to get tcobase address\n");
-		goto unreg_region;
+		goto unreg_mem_region;
  	}
  
  	/* Enable watchdog decode bit */
@@ -346,7 +353,8 @@ static unsigned char __devinit sp5100_tc
  	/* Done */
  	return 1;
  
-	iounmap(tcobase);
+unreg_mem_region:
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
  unreg_region:
  	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
  exit:
@@ -401,6 +409,7 @@ static int __devinit sp5100_tco_init(str
  
  exit:
  	iounmap(tcobase);
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
  	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
  	return ret;
  }
@@ -414,6 +423,7 @@ static void __devexit sp5100_tco_cleanup
  	/* Deregister */
  	misc_deregister(&sp5100_tco_miscdev);
  	iounmap(tcobase);
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
  	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
  }
  

Re: [PATCH] watchdog, SP5100: Check if firmware has set correct value in tcobase.

[Stefano Stabellini]

On Thu, 17 Mar 2011, Konrad Rzeszutek Wilk wrote:
> On Wed, Mar 16, 2011 at 02:18:17PM -0700, Yinghai Lu wrote:
> > 
> > 
> > Stefano found SP5100 TCO watchdog driver using wrong address.
> > 
> > [    9.148536] SP5100 TCO timer: SP5100 TCO WatchDog Timer Driver v0.01
> > [    9.148628] DEBUG __ioremap_caller WARNING address=b8fe00 size=8 valid=1 reserved=1
> > 
> > and e820 said that range is RAM.
> > 
> > We should check if we can use that reading out. BIOS could just program wrong address there.
> > 
> > -v2: Mike pointed out one path need one release.
> > 
> > Reported-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Signed-off-by:Yinghai Lu <yinghai@kernel.org>
> > Acked-by: Mike Waychison <mikew@google.com>
> 
> I have no idea why it worked the first time b/c this:
> 
> 
> > +	if (request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE, "SP5100 TCO")) {
> 
> is wrong. It should have been "if (!request...")..
> 
> With that, and with Stefano's patches (stefano/2.6.38-rc6-mm-fix) on top of 2.6.39-rc0 it boots up fine.

Yinghai, thanks for the patch!
I hope that we are not going to find any more of this kind of issues
with other drivers and other BIOSes.

Re: [Xen-devel] [PATCH -v3] watchdog, SP5100: Check if firmware has set correct value in tcobase.

[Konrad Rzeszutek Wilk]

On Wed, Mar 16, 2011 at 08:01:07PM -0700, Yinghai Lu wrote:
> 
> Stefano found SP5100 TCO watchdog driver using wrong address.
> 
> [    9.148536] SP5100 TCO timer: SP5100 TCO WatchDog Timer Driver v0.01
> [    9.148628] DEBUG __ioremap_caller WARNING address=b8fe00 size=8 valid=1 reserved=1
> 
> and e820 said that range is RAM.
> 
> We should check if we can use that reading out. BIOS could just program wrong address there.
> 
> -v2: Mike pointed out one path need one release.
> -v3: corrected logic checking with request_mem_region_exclusive()
>      Found by Konrad.

Yinghai:
Not sure what you are using as a base, but I had to modify this patch
to go on top of 2.6.38. Here is the patch that applies cleanly:

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 8083728..9cbca8b 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -42,6 +42,7 @@
 #define PFX TCO_MODULE_NAME ": "
 
 /* internal variables */
+static u32 tcobase_phys;
 static void __iomem *tcobase;
 static unsigned int pm_iobase;
 static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
@@ -305,10 +306,16 @@ static unsigned char __devinit sp5100_tco_setupdevice(void)
 	/* Low three bits of BASE0 are reserved. */
 	val = val << 8 | (inb(SP5100_IO_PM_DATA_REG) & 0xf8);
 
+	if (!request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE, "SP5100 TCO")) {
+                printk(KERN_ERR PFX "mmio address 0x%04x already in use\n", val);
+                goto unreg_region;
+        }
+        tcobase_phys = val;
+
 	tcobase = ioremap(val, SP5100_WDT_MEM_MAP_SIZE);
 	if (tcobase == 0) {
 		printk(KERN_ERR PFX "failed to get tcobase address\n");
-		goto unreg_region;
+		goto unreg_mem_region;
 	}
 
 	/* Enable watchdog decode bit */
@@ -346,7 +353,8 @@ static unsigned char __devinit sp5100_tco_setupdevice(void)
 	/* Done */
 	return 1;
 
-	iounmap(tcobase);
+unreg_mem_region:
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
 unreg_region:
 	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 exit:
@@ -401,6 +409,7 @@ static int __devinit sp5100_tco_init(struct platform_device *dev)
 
 exit:
 	iounmap(tcobase);
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
 	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 	return ret;
 }
@@ -414,6 +423,7 @@ static void __devexit sp5100_tco_cleanup(void)
 	/* Deregister */
 	misc_deregister(&sp5100_tco_miscdev);
 	iounmap(tcobase);
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
 	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
 }
 
> 
> 
> Reported-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by:Yinghai Lu <yinghai@kernel.org>
> Acked-by: Mike Waychison <mikew@google.com>
> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> ---
>  drivers/watchdog/sp5100_tco.c |   14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> Index: linux-2.6/drivers/watchdog/sp5100_tco.c
> ===================================================================
> --- linux-2.6.orig/drivers/watchdog/sp5100_tco.c
> +++ linux-2.6/drivers/watchdog/sp5100_tco.c
> @@ -42,6 +42,7 @@
>  #define PFX TCO_MODULE_NAME ": "
>  /* internal variables */
> +static u32 tcobase_phys;
>  static void __iomem *tcobase;
>  static unsigned int pm_iobase;
>  static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
> @@ -305,10 +306,16 @@ static unsigned char __devinit sp5100_tc
>  	/* Low three bits of BASE0 are reserved. */
>  	val = val << 8 | (inb(SP5100_IO_PM_DATA_REG) & 0xf8);
> +	if (!request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE, "SP5100 TCO")) {
> +		printk(KERN_ERR PFX "mmio address 0x%04x already in use\n", val);
> +		goto unreg_region;
> +	}
> +	tcobase_phys = val;
> +
>  	tcobase = ioremap(val, SP5100_WDT_MEM_MAP_SIZE);
>  	if (tcobase == 0) {
>  		printk(KERN_ERR PFX "failed to get tcobase address\n");
> -		goto unreg_region;
> +		goto unreg_mem_region;
>  	}
>  	/* Enable watchdog decode bit */
> @@ -346,7 +353,8 @@ static unsigned char __devinit sp5100_tc
>  	/* Done */
>  	return 1;
> -	iounmap(tcobase);
> +unreg_mem_region:
> +	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
>  unreg_region:
>  	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
>  exit:
> @@ -401,6 +409,7 @@ static int __devinit sp5100_tco_init(str
>  exit:
>  	iounmap(tcobase);
> +	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
>  	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
>  	return ret;
>  }
> @@ -414,6 +423,7 @@ static void __devexit sp5100_tco_cleanup
>  	/* Deregister */
>  	misc_deregister(&sp5100_tco_miscdev);
>  	iounmap(tcobase);
> +	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
>  	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
>  }
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [Xen-devel] [PATCH -v3] watchdog, SP5100: Check if firmware has set correct value in tcobase.

[Yinghai Lu]

On Fri, Mar 18, 2011 at 6:10 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Wed, Mar 16, 2011 at 08:01:07PM -0700, Yinghai Lu wrote:
>>
>> Stefano found SP5100 TCO watchdog driver using wrong address.
>>
>> [    9.148536] SP5100 TCO timer: SP5100 TCO WatchDog Timer Driver v0.01
>> [    9.148628] DEBUG __ioremap_caller WARNING address=b8fe00 size=8 valid=1 reserved=1
>>
>> and e820 said that range is RAM.
>>
>> We should check if we can use that reading out. BIOS could just program wrong address there.
>>
>> -v2: Mike pointed out one path need one release.
>> -v3: corrected logic checking with request_mem_region_exclusive()
>>      Found by Konrad.
>
> Yinghai:
> Not sure what you are using as a base, but I had to modify this patch
> to go on top of 2.6.38. Here is the patch that applies cleanly:


after

commit 4562f53940432369df88e195ef8f9b642bdf7cd6
Author: Wim Van Sebroeck <wim@iguana.be>
Date:   Mon Feb 21 12:16:44 2011 +0000

    watchdog: convert to DEFINE_PCI_DEVICE_TABLE

    Convert static struct pci_device_id *[] to static
DEFINE_PCI_DEVICE_TABLE tables.

    Signed-off-by: Wim Van Sebroeck <wim@iguana.be>

diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
index 8083728..1bc4938 100644
--- a/drivers/watchdog/sp5100_tco.c
+++ b/drivers/watchdog/sp5100_tco.c
@@ -259,7 +259,7 @@ static struct miscdevice sp5100_tco_miscdev = {
  * register a pci_driver, because someone else might
  * want to register another driver on the same PCI id.
  */
-static struct pci_device_id sp5100_tco_pci_tbl[] = {
+static DEFINE_PCI_DEVICE_TABLE(sp5100_tco_pci_tbl) = {
        { PCI_VENDOR_ID_ATI, PCI_DEVICE_ID_ATI_SBX00_SMBUS, PCI_ANY_ID,
          PCI_ANY_ID, },
        { 0, },                 /* End of list */

commit 15e28bf130081a574192fb934b832ac7d07739f7
Author: Priyanka Gupta <priyankag@google.com>
Date:   Mon Oct 25 17:58:04 2010 -0700

    watchdog: Add support for sp5100 chipset TCO

    This driver adds /dev/watchdog support for the AMD sp5100 aka
SB7x0 chipsets.

    It follows the same conventions found in other /dev/watchdog drivers.

    Signed-off-by: Priyanka Gupta <priyankag@google.com>
    Signed-off-by: Mike Waychison <mikew@google.com>
    Signed-off-by: Wim Van Sebroeck <wim@iguana.be>

[PATCH -v3 -resend] watchdog, SP5100: Check if firmware has set correct value in tcobase.

[Yinghai Lu]

Stefano found SP5100 TCO watchdog driver using wrong address.

[    9.148536] SP5100 TCO timer: SP5100 TCO WatchDog Timer Driver v0.01
[    9.148628] DEBUG __ioremap_caller WARNING address=b8fe00 size=8 valid=1 reserved=1

and e820 said that range is RAM.

We should check if we can use that reading out. BIOS could just program wrong address there.

-v2: Mike pointed out one path need one release.
-v3: corrected logic checking with request_mem_region_exclusive()
      Found by Konrad.


Reported-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by:Yinghai Lu <yinghai@kernel.org>
Acked-by: Mike Waychison <mikew@google.com>
Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
  drivers/watchdog/sp5100_tco.c |   14 ++++++++++++--
  1 file changed, 12 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/watchdog/sp5100_tco.c
===================================================================
--- linux-2.6.orig/drivers/watchdog/sp5100_tco.c
+++ linux-2.6/drivers/watchdog/sp5100_tco.c
@@ -42,6 +42,7 @@
  #define PFX TCO_MODULE_NAME ": "
  
  /* internal variables */
+static u32 tcobase_phys;
  static void __iomem *tcobase;
  static unsigned int pm_iobase;
  static DEFINE_SPINLOCK(tco_lock);	/* Guards the hardware */
@@ -305,10 +306,16 @@ static unsigned char __devinit sp5100_tc
  	/* Low three bits of BASE0 are reserved. */
  	val = val << 8 | (inb(SP5100_IO_PM_DATA_REG) & 0xf8);
  
+	if (!request_mem_region_exclusive(val, SP5100_WDT_MEM_MAP_SIZE, "SP5100 TCO")) {
+		printk(KERN_ERR PFX "mmio address 0x%04x already in use\n", val);
+		goto unreg_region;
+	}
+	tcobase_phys = val;
+
  	tcobase = ioremap(val, SP5100_WDT_MEM_MAP_SIZE);
  	if (tcobase == 0) {
  		printk(KERN_ERR PFX "failed to get tcobase address\n");
-		goto unreg_region;
+		goto unreg_mem_region;
  	}
  
  	/* Enable watchdog decode bit */
@@ -346,7 +353,8 @@ static unsigned char __devinit sp5100_tc
  	/* Done */
  	return 1;
  
-	iounmap(tcobase);
+unreg_mem_region:
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
  unreg_region:
  	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
  exit:
@@ -401,6 +409,7 @@ static int __devinit sp5100_tco_init(str
  
  exit:
  	iounmap(tcobase);
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
  	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
  	return ret;
  }
@@ -414,6 +423,7 @@ static void __devexit sp5100_tco_cleanup
  	/* Deregister */
  	misc_deregister(&sp5100_tco_miscdev);
  	iounmap(tcobase);
+	release_mem_region(tcobase_phys, SP5100_WDT_MEM_MAP_SIZE);
  	release_region(pm_iobase, SP5100_PM_IOPORTS_SIZE);
  }
  

Re: [PATCH -v3 -resend] watchdog, SP5100: Check if firmware has set correct value in tcobase.

[Wim Van Sebroeck]

Hi Yinghai,

> Stefano found SP5100 TCO watchdog driver using wrong address.
>
> [    9.148536] SP5100 TCO timer: SP5100 TCO WatchDog Timer Driver v0.01
> [    9.148628] DEBUG __ioremap_caller WARNING address=b8fe00 size=8 valid=1 reserved=1
>
> and e820 said that range is RAM.
>
> We should check if we can use that reading out. BIOS could just program wrong address there.
>
> -v2: Mike pointed out one path need one release.
> -v3: corrected logic checking with request_mem_region_exclusive()
>      Found by Konrad.
>
>
> Reported-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by:Yinghai Lu <yinghai@kernel.org>
> Acked-by: Mike Waychison <mikew@google.com>
> Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

This patch is in linux-2.6-watchdog-next for a week now. Will go upstream to linus.

Kind regards,
Wim.

Re: [PATCH -v3 -resend] watchdog, SP5100: Check if firmware has set correct value in tcobase.

[Yinghai Lu]

On 03/24/2011 01:31 AM, Wim Van Sebroeck wrote:
>
> This patch is in linux-2.6-watchdog-next for a week now. Will go upstream to linus.

Thanks.