* Re: [GIT PULL tip/x86/mm] xen/x86 fixes ===> fix sp5100_tco mmio checking. [not found] ` <alpine.DEB.2.00.1103161757040.3382@kaball-desktop> @ 2011-03-16 20:45 ` Yinghai Lu 2011-03-16 21:01 ` Mike Waychison 0 siblings, 1 reply; 11+ messages in thread From: Yinghai Lu @ 2011-03-16 20:45 UTC (permalink / raw) To: Stefano Stabellini Cc: Konrad Rzeszutek Wilk, H. Peter Anvin, linux-kernel@vger.kernel.org, Jeremy Fitzhardinge, xen-devel@lists.xensource.com, Wim Van Sebroeck, linux-watchdog, Mike Waychison, Priyanka Gupta 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); } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [GIT PULL tip/x86/mm] xen/x86 fixes ===> fix sp5100_tco mmio checking. 2011-03-16 20:45 ` [GIT PULL tip/x86/mm] xen/x86 fixes ===> fix sp5100_tco mmio checking Yinghai Lu @ 2011-03-16 21:01 ` Mike Waychison 2011-03-16 21:18 ` [PATCH] watchdog, SP5100: Check if firmware has set correct value in tcobase Yinghai Lu 0 siblings, 1 reply; 11+ messages in thread From: Mike Waychison @ 2011-03-16 21:01 UTC (permalink / raw) To: Yinghai Lu Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, H. Peter Anvin, linux-kernel@vger.kernel.org, Jeremy Fitzhardinge, xen-devel@lists.xensource.com, Wim Van Sebroeck, linux-watchdog, Priyanka Gupta 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); > } > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] watchdog, SP5100: Check if firmware has set correct value in tcobase. 2011-03-16 21:01 ` Mike Waychison @ 2011-03-16 21:18 ` Yinghai Lu 2011-03-17 0:00 ` Konrad Rzeszutek Wilk [not found] ` <20110317022331.GA3528@dumpdata.com> 0 siblings, 2 replies; 11+ messages in thread From: Yinghai Lu @ 2011-03-16 21:18 UTC (permalink / raw) To: Mike Waychison, Wim Van Sebroeck Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, H. Peter Anvin, linux-kernel@vger.kernel.org, Jeremy Fitzhardinge, xen-devel@lists.xensource.com, linux-watchdog, Priyanka Gupta 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); } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] watchdog, SP5100: Check if firmware has set correct value in tcobase. 2011-03-16 21:18 ` [PATCH] watchdog, SP5100: Check if firmware has set correct value in tcobase Yinghai Lu @ 2011-03-17 0:00 ` Konrad Rzeszutek Wilk [not found] ` <20110317022331.GA3528@dumpdata.com> 1 sibling, 0 replies; 11+ messages in thread From: Konrad Rzeszutek Wilk @ 2011-03-17 0:00 UTC (permalink / raw) To: Yinghai Lu Cc: Mike Waychison, Wim Van Sebroeck, Stefano Stabellini, H. Peter Anvin, linux-kernel@vger.kernel.org, Jeremy Fitzhardinge, xen-devel@lists.xensource.com, linux-watchdog, Priyanka Gupta 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <20110317022331.GA3528@dumpdata.com>]
* [PATCH -v3] watchdog, SP5100: Check if firmware has set correct value in tcobase. [not found] ` <20110317022331.GA3528@dumpdata.com> @ 2011-03-17 3:01 ` Yinghai Lu 2011-03-18 13:10 ` [Xen-devel] " Konrad Rzeszutek Wilk 2011-03-17 12:35 ` [PATCH] " Stefano Stabellini 2011-03-24 5:33 ` [PATCH -v3 -resend] " Yinghai Lu 2 siblings, 1 reply; 11+ messages in thread From: Yinghai Lu @ 2011-03-17 3:01 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Mike Waychison, Wim Van Sebroeck, Stefano Stabellini, H. Peter Anvin, linux-kernel@vger.kernel.org, Jeremy Fitzhardinge, xen-devel@lists.xensource.com, linux-watchdog, Priyanka Gupta 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); } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Xen-devel] [PATCH -v3] watchdog, SP5100: Check if firmware has set correct value in tcobase. 2011-03-17 3:01 ` [PATCH -v3] " Yinghai Lu @ 2011-03-18 13:10 ` Konrad Rzeszutek Wilk 2011-03-18 16:39 ` Yinghai Lu 0 siblings, 1 reply; 11+ messages in thread From: Konrad Rzeszutek Wilk @ 2011-03-18 13:10 UTC (permalink / raw) To: Yinghai Lu Cc: Priyanka Gupta, Jeremy Fitzhardinge, xen-devel@lists.xensource.com, linux-watchdog, Stefano Stabellini, linux-kernel@vger.kernel.org, Mike Waychison, Wim Van Sebroeck, H. Peter Anvin 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Xen-devel] [PATCH -v3] watchdog, SP5100: Check if firmware has set correct value in tcobase. 2011-03-18 13:10 ` [Xen-devel] " Konrad Rzeszutek Wilk @ 2011-03-18 16:39 ` Yinghai Lu 0 siblings, 0 replies; 11+ messages in thread From: Yinghai Lu @ 2011-03-18 16:39 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Priyanka Gupta, Jeremy Fitzhardinge, xen-devel@lists.xensource.com, linux-watchdog, Stefano Stabellini, linux-kernel@vger.kernel.org, Mike Waychison, Wim Van Sebroeck, H. Peter Anvin 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> ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] watchdog, SP5100: Check if firmware has set correct value in tcobase. [not found] ` <20110317022331.GA3528@dumpdata.com> 2011-03-17 3:01 ` [PATCH -v3] " Yinghai Lu @ 2011-03-17 12:35 ` Stefano Stabellini 2011-03-24 5:33 ` [PATCH -v3 -resend] " Yinghai Lu 2 siblings, 0 replies; 11+ messages in thread From: Stefano Stabellini @ 2011-03-17 12:35 UTC (permalink / raw) To: Konrad Rzeszutek Wilk Cc: Yinghai Lu, Mike Waychison, Wim Van Sebroeck, Stefano Stabellini, H. Peter Anvin, linux-kernel@vger.kernel.org, Jeremy Fitzhardinge, xen-devel@lists.xensource.com, linux-watchdog@vger.kernel.org, Priyanka Gupta 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH -v3 -resend] watchdog, SP5100: Check if firmware has set correct value in tcobase. [not found] ` <20110317022331.GA3528@dumpdata.com> 2011-03-17 3:01 ` [PATCH -v3] " Yinghai Lu 2011-03-17 12:35 ` [PATCH] " Stefano Stabellini @ 2011-03-24 5:33 ` Yinghai Lu 2011-03-24 8:31 ` Wim Van Sebroeck 2 siblings, 1 reply; 11+ messages in thread From: Yinghai Lu @ 2011-03-24 5:33 UTC (permalink / raw) To: Wim Van Sebroeck, Andrew Morton Cc: Konrad Rzeszutek Wilk, Mike Waychison, Stefano Stabellini, H. Peter Anvin, linux-kernel@vger.kernel.org, Jeremy Fitzhardinge, xen-devel@lists.xensource.com, linux-watchdog, Priyanka Gupta 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); } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -v3 -resend] watchdog, SP5100: Check if firmware has set correct value in tcobase. 2011-03-24 5:33 ` [PATCH -v3 -resend] " Yinghai Lu @ 2011-03-24 8:31 ` Wim Van Sebroeck 2011-03-24 15:40 ` Yinghai Lu 0 siblings, 1 reply; 11+ messages in thread From: Wim Van Sebroeck @ 2011-03-24 8:31 UTC (permalink / raw) To: Yinghai Lu Cc: Andrew Morton, Konrad Rzeszutek Wilk, Mike Waychison, Stefano Stabellini, H. Peter Anvin, linux-kernel@vger.kernel.org, Jeremy Fitzhardinge, xen-devel@lists.xensource.com, linux-watchdog, Priyanka Gupta 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH -v3 -resend] watchdog, SP5100: Check if firmware has set correct value in tcobase. 2011-03-24 8:31 ` Wim Van Sebroeck @ 2011-03-24 15:40 ` Yinghai Lu 0 siblings, 0 replies; 11+ messages in thread From: Yinghai Lu @ 2011-03-24 15:40 UTC (permalink / raw) To: Wim Van Sebroeck Cc: Andrew Morton, Konrad Rzeszutek Wilk, Mike Waychison, Stefano Stabellini, H. Peter Anvin, linux-kernel@vger.kernel.org, Jeremy Fitzhardinge, xen-devel@lists.xensource.com, linux-watchdog, Priyanka Gupta 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-03-24 15:41 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <alpine.DEB.2.00.1103111201470.2968@kaball-desktop>
[not found] ` <20110311222129.GA3168@dumpdata.com>
[not found] ` <alpine.DEB.2.00.1103161150010.3382@kaball-desktop>
[not found] ` <alpine.DEB.2.00.1103161442210.3382@kaball-desktop>
[not found] ` <4D80F992.10603@kernel.org>
[not found] ` <alpine.DEB.2.00.1103161757040.3382@kaball-desktop>
2011-03-16 20:45 ` [GIT PULL tip/x86/mm] xen/x86 fixes ===> fix sp5100_tco mmio checking Yinghai Lu
2011-03-16 21:01 ` Mike Waychison
2011-03-16 21:18 ` [PATCH] watchdog, SP5100: Check if firmware has set correct value in tcobase Yinghai Lu
2011-03-17 0:00 ` Konrad Rzeszutek Wilk
[not found] ` <20110317022331.GA3528@dumpdata.com>
2011-03-17 3:01 ` [PATCH -v3] " Yinghai Lu
2011-03-18 13:10 ` [Xen-devel] " Konrad Rzeszutek Wilk
2011-03-18 16:39 ` Yinghai Lu
2011-03-17 12:35 ` [PATCH] " Stefano Stabellini
2011-03-24 5:33 ` [PATCH -v3 -resend] " Yinghai Lu
2011-03-24 8:31 ` Wim Van Sebroeck
2011-03-24 15:40 ` Yinghai Lu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).