[PATCH] MIPS: fix pfn_valid() for FLAGMEM

[PATCH] MIPS: fix pfn_valid() for FLAGMEM

[Wu Zhangjin]

When CONFIG_FLAGMEM enabled, STD/Hiberation will fail on YeeLoong
laptop, This patch fix it:

if pfn is between min_low_pfn and max_mapnr, the old pfn_valid() will
return TRUE, but in reality, if the memory is not continuous, it should
be false. for example:

$ cat /proc/iomem | grep "System RAM"
00000000-0fffffff : System RAM
90000000-bfffffff : System RAM

as we can see, it is not continuous, so, some of the memory is not valid
but regarded as valid by pfn_valid(), and at last make STD/Hibernate
fail when shrinking a too large number of invalid memory.

Here, we fix it via checking pfn is in the "System RAM" or not, if yes,
return TRUE.

Signed-off-by: Wu Zhangjin <wuzhangjin@gmail.com>
---
 arch/mips/include/asm/page.h |   11 ++++-------
 arch/mips/mm/page.c          |   17 +++++++++++++++++
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/arch/mips/include/asm/page.h b/arch/mips/include/asm/page.h
index f266295..16903a6 100644
--- a/arch/mips/include/asm/page.h
+++ b/arch/mips/include/asm/page.h
@@ -168,13 +168,10 @@ typedef struct { unsigned long pgprot; } pgprot_t;
 
 #ifdef CONFIG_FLATMEM
 
-#define pfn_valid(pfn)							\
-({									\
-	unsigned long __pfn = (pfn);					\
-	/* avoid <linux/bootmem.h> include hell */			\
-	extern unsigned long min_low_pfn;				\
-									\
-	__pfn >= min_low_pfn && __pfn < max_mapnr;			\
+#define pfn_valid(pfn)				\
+({						\
+	extern int is_pfn_valid(unsigned long); \
+	is_pfn_valid(pfn);			\
 })
 
 #elif defined(CONFIG_SPARSEMEM)
diff --git a/arch/mips/mm/page.c b/arch/mips/mm/page.c
index f5c7375..48a4d2a 100644
--- a/arch/mips/mm/page.c
+++ b/arch/mips/mm/page.c
@@ -689,3 +689,20 @@ void copy_page(void *to, void *from)
 }
 
 #endif /* CONFIG_SIBYTE_DMA_PAGEOPS */
+
+#ifdef CONFIG_FLATMEM
+int is_pfn_valid(unsigned long pfn)
+{
+	int i;
+
+	for (i = 0; i < boot_mem_map.nr_map; i++) {
+		if (boot_mem_map.map[i].type == BOOT_MEM_RAM) {
+			if ((pfn >= PFN_DOWN(boot_mem_map.map[i].addr)) &&
+				((pfn) <= (PFN_UP(boot_mem_map.map[i].addr +
+					boot_mem_map.map[i].size))))
+				return 1;
+		}
+	}
+	return 0;
+}
+#endif
-- 
1.6.2.1

Re: [PATCH] MIPS: fix pfn_valid() for FLAGMEM

[Pavel Machek]

On Thu 2009-10-08 16:57:32, Wu Zhangjin wrote:
> When CONFIG_FLAGMEM enabled, STD/Hiberation will fail on YeeLoong
> laptop, This patch fix it:
> 
> if pfn is between min_low_pfn and max_mapnr, the old pfn_valid() will
> return TRUE, but in reality, if the memory is not continuous, it should
> be false. for example:
> 
> $ cat /proc/iomem | grep "System RAM"
> 00000000-0fffffff : System RAM
> 90000000-bfffffff : System RAM
> 
> as we can see, it is not continuous, so, some of the memory is not valid
> but regarded as valid by pfn_valid(), and at last make STD/Hibernate
> fail when shrinking a too large number of invalid memory.
> 
> Here, we fix it via checking pfn is in the "System RAM" or not, if yes,
> return TRUE.

"return FALSE"?

> Signed-off-by: Wu Zhangjin <wuzhangjin@gmail.com>

Looks mostly ok, small comments below. 

> @@ -168,13 +168,10 @@ typedef struct { unsigned long pgprot; } pgprot_t;
>  
>  #ifdef CONFIG_FLATMEM
>  
> -#define pfn_valid(pfn)							\
> -({									\
> -	unsigned long __pfn = (pfn);					\
> -	/* avoid <linux/bootmem.h> include hell */			\
> -	extern unsigned long min_low_pfn;				\
> -									\
> -	__pfn >= min_low_pfn && __pfn < max_mapnr;			\
> +#define pfn_valid(pfn)				\
> +({						\
> +	extern int is_pfn_valid(unsigned long); \
> +	is_pfn_valid(pfn);			\
>  })

"extern int pfn_valid here"

...and get away without the ugly macro?

>  #elif defined(CONFIG_SPARSEMEM)
> diff --git a/arch/mips/mm/page.c b/arch/mips/mm/page.c
> index f5c7375..48a4d2a 100644
> --- a/arch/mips/mm/page.c
> +++ b/arch/mips/mm/page.c
> @@ -689,3 +689,20 @@ void copy_page(void *to, void *from)
>  }
>  
>  #endif /* CONFIG_SIBYTE_DMA_PAGEOPS */
> +
> +#ifdef CONFIG_FLATMEM
> +int is_pfn_valid(unsigned long pfn)

...with simple function rename?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] MIPS: fix pfn_valid() for FLAGMEM

[Wu Zhangjin]

Hi,

On Thu, 2009-10-08 at 11:29 +0200, Pavel Machek wrote:
> On Thu 2009-10-08 16:57:32, Wu Zhangjin wrote:
> > When CONFIG_FLAGMEM enabled, STD/Hiberation will fail on YeeLoong
> > laptop, This patch fix it:
> > 
> > if pfn is between min_low_pfn and max_mapnr, the old pfn_valid() will
> > return TRUE, but in reality, if the memory is not continuous, it should
> > be false. for example:
> > 
> > $ cat /proc/iomem | grep "System RAM"
> > 00000000-0fffffff : System RAM
> > 90000000-bfffffff : System RAM
> > 
> > as we can see, it is not continuous, so, some of the memory is not valid
> > but regarded as valid by pfn_valid(), and at last make STD/Hibernate
> > fail when shrinking a too large number of invalid memory.
> > 
> > Here, we fix it via checking pfn is in the "System RAM" or not, if yes,
> > return TRUE.
> 
> "return FALSE"?
> 
> > Signed-off-by: Wu Zhangjin <wuzhangjin@gmail.com>
> 
> Looks mostly ok, small comments below. 
> 
> > @@ -168,13 +168,10 @@ typedef struct { unsigned long pgprot; } pgprot_t;
> >  
> >  #ifdef CONFIG_FLATMEM
> >  
> > -#define pfn_valid(pfn)							\
> > -({									\
> > -	unsigned long __pfn = (pfn);					\
> > -	/* avoid <linux/bootmem.h> include hell */			\
> > -	extern unsigned long min_low_pfn;				\
> > -									\
> > -	__pfn >= min_low_pfn && __pfn < max_mapnr;			\
> > +#define pfn_valid(pfn)				\
> > +({						\
> > +	extern int is_pfn_valid(unsigned long); \
> > +	is_pfn_valid(pfn);			\
> >  })
> 
> "extern int pfn_valid here"
> 
> ...and get away without the ugly macro?
> 

Perhaps need to move the whole "#ifdef CONFIG_FLATMEM" to #ifndef
ASSEMBLY, otherwise,

"arch/mips/include/asm/page.h:170: Error: unrecognized opcode `extern
int pfn_valid(unsigned long)'". 

Regards,
	Wu Zhangjin

Re: [PATCH] MIPS: fix pfn_valid() for FLAGMEM

[Pavel Machek]

On Thu 2009-10-08 18:33:39, Wu Zhangjin wrote:
> Hi,
> 
> On Thu, 2009-10-08 at 11:29 +0200, Pavel Machek wrote:
> > On Thu 2009-10-08 16:57:32, Wu Zhangjin wrote:
> > > When CONFIG_FLAGMEM enabled, STD/Hiberation will fail on YeeLoong
> > > laptop, This patch fix it:
> > > 
> > > if pfn is between min_low_pfn and max_mapnr, the old pfn_valid() will
> > > return TRUE, but in reality, if the memory is not continuous, it should
> > > be false. for example:
> > > 
> > > $ cat /proc/iomem | grep "System RAM"
> > > 00000000-0fffffff : System RAM
> > > 90000000-bfffffff : System RAM
> > > 
> > > as we can see, it is not continuous, so, some of the memory is not valid
> > > but regarded as valid by pfn_valid(), and at last make STD/Hibernate
> > > fail when shrinking a too large number of invalid memory.
> > > 
> > > Here, we fix it via checking pfn is in the "System RAM" or not, if yes,
> > > return TRUE.
> > 
> > "return FALSE"?
> > 
> > > Signed-off-by: Wu Zhangjin <wuzhangjin@gmail.com>
> > 
> > Looks mostly ok, small comments below. 
> > 
> > > @@ -168,13 +168,10 @@ typedef struct { unsigned long pgprot; } pgprot_t;
> > >  
> > >  #ifdef CONFIG_FLATMEM
> > >  
> > > -#define pfn_valid(pfn)							\
> > > -({									\
> > > -	unsigned long __pfn = (pfn);					\
> > > -	/* avoid <linux/bootmem.h> include hell */			\
> > > -	extern unsigned long min_low_pfn;				\
> > > -									\
> > > -	__pfn >= min_low_pfn && __pfn < max_mapnr;			\
> > > +#define pfn_valid(pfn)				\
> > > +({						\
> > > +	extern int is_pfn_valid(unsigned long); \
> > > +	is_pfn_valid(pfn);			\
> > >  })
> > 
> > "extern int pfn_valid here"
> > 
> > ...and get away without the ugly macro?
> > 
> 
> Perhaps need to move the whole "#ifdef CONFIG_FLATMEM" to #ifndef
> ASSEMBLY, otherwise,
> 
> "arch/mips/include/asm/page.h:170: Error: unrecognized opcode `extern
> int pfn_valid(unsigned long)'". 

I guess so. pfn_valid() will not work from assembly, anywa, so...
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] MIPS: fix pfn_valid() for FLAGMEM

[Wu Zhangjin]

Hello,

> > > 
> > > Looks mostly ok, small comments below. 
> > > 
> > > > @@ -168,13 +168,10 @@ typedef struct { unsigned long pgprot; } pgprot_t;
> > > >  
> > > >  #ifdef CONFIG_FLATMEM
> > > >  
> > > > -#define pfn_valid(pfn)							\
> > > > -({									\
> > > > -	unsigned long __pfn = (pfn);					\
> > > > -	/* avoid <linux/bootmem.h> include hell */			\
> > > > -	extern unsigned long min_low_pfn;				\
> > > > -									\
> > > > -	__pfn >= min_low_pfn && __pfn < max_mapnr;			\
> > > > +#define pfn_valid(pfn)				\
> > > > +({						\
> > > > +	extern int is_pfn_valid(unsigned long); \
> > > > +	is_pfn_valid(pfn);			\
> > > >  })
> > > 
> > > "extern int pfn_valid here"
> > > 
> > > ...and get away without the ugly macro?
> > > 
> > 
> > Perhaps need to move the whole "#ifdef CONFIG_FLATMEM" to #ifndef
> > ASSEMBLY, otherwise,
> > 
> > "arch/mips/include/asm/page.h:170: Error: unrecognized opcode `extern
> > int pfn_valid(unsigned long)'". 
> 
> I guess so. pfn_valid() will not work from assembly, anywa, so...

Seems pfn_valid() is only called in non-ASSEMBLY source code, so, we are
safe to move it to "#ifndef ASSEMBLY", just tested it, works.

Regards,
	Wu Zhangjin

Re: [PATCH] MIPS: fix pfn_valid() for FLAGMEM

[Sergei Shtylyov]

Hello.

Wu Zhangjin wrote:

> When CONFIG_FLAGMEM enabled, STD/Hiberation will fail on YeeLoong
>   

   You surely meant FLATMEM here and the subject.

> laptop, This patch fix it:
>   

   Fixes.

> Signed-off-by: Wu Zhangjin <wuzhangjin@gmail.com>
>   
[...]
> diff --git a/arch/mips/include/asm/page.h b/arch/mips/include/asm/page.h
> index f266295..16903a6 100644
> --- a/arch/mips/include/asm/page.h
> +++ b/arch/mips/include/asm/page.h
> @@ -168,13 +168,10 @@ typedef struct { unsigned long pgprot; } pgprot_t;
>  
>  #ifdef CONFIG_FLATMEM
>  
> -#define pfn_valid(pfn)							\
> -({									\
> -	unsigned long __pfn = (pfn);					\
> -	/* avoid <linux/bootmem.h> include hell */			\
> -	extern unsigned long min_low_pfn;				\
> -									\
> -	__pfn >= min_low_pfn && __pfn < max_mapnr;			\
> +#define pfn_valid(pfn)				\
> +({						\
> +	extern int is_pfn_valid(unsigned long); \
> +	is_pfn_valid(pfn);			\
>   

   Why not just define pfn_valid() as *extern* function in this case?

> diff --git a/arch/mips/mm/page.c b/arch/mips/mm/page.c
> index f5c7375..48a4d2a 100644
> --- a/arch/mips/mm/page.c
> +++ b/arch/mips/mm/page.c
> @@ -689,3 +689,20 @@ void copy_page(void *to, void *from)
>  }
>  
>  #endif /* CONFIG_SIBYTE_DMA_PAGEOPS */
> +
> +#ifdef CONFIG_FLATMEM
> +int is_pfn_valid(unsigned long pfn)
> +{
> +	int i;
> +
> +	for (i = 0; i < boot_mem_map.nr_map; i++) {
> +		if (boot_mem_map.map[i].type == BOOT_MEM_RAM) {
> +			if ((pfn >= PFN_DOWN(boot_mem_map.map[i].addr)) &&
> +				((pfn) <= (PFN_UP(boot_mem_map.map[i].addr +
>   

   Not <?

> +					boot_mem_map.map[i].size))))
> +				return 1;
> +		}
> +	}
> +	return 0;
> +}
> +#endif
>   

WBR, Sergei

Re: [PATCH] MIPS: fix pfn_valid() for FLAGMEM

[Wu Zhangjin]

Hello,

> > When CONFIG_FLAGMEM enabled, STD/Hiberation will fail on YeeLoong
> >   
> 
>    You surely meant FLATMEM here and the subject.
> 
> > laptop, This patch fix it:
> >   
> 
>    Fixes.
> 

Thanks, will resend a new one later.

> > Signed-off-by: Wu Zhangjin <wuzhangjin@gmail.com>
> >   
> [...]
> > diff --git a/arch/mips/include/asm/page.h b/arch/mips/include/asm/page.h
> > index f266295..16903a6 100644
> > --- a/arch/mips/include/asm/page.h
> > +++ b/arch/mips/include/asm/page.h
> > @@ -168,13 +168,10 @@ typedef struct { unsigned long pgprot; } pgprot_t;
> >  
> >  #ifdef CONFIG_FLATMEM
> >  
> > -#define pfn_valid(pfn)							\
> > -({									\
> > -	unsigned long __pfn = (pfn);					\
> > -	/* avoid <linux/bootmem.h> include hell */			\
> > -	extern unsigned long min_low_pfn;				\
> > -									\
> > -	__pfn >= min_low_pfn && __pfn < max_mapnr;			\
> > +#define pfn_valid(pfn)				\
> > +({						\
> > +	extern int is_pfn_valid(unsigned long); \
> > +	is_pfn_valid(pfn);			\
> >   
> 
>    Why not just define pfn_valid() as *extern* function in this case?
> 
> > diff --git a/arch/mips/mm/page.c b/arch/mips/mm/page.c
> > index f5c7375..48a4d2a 100644
> > --- a/arch/mips/mm/page.c
> > +++ b/arch/mips/mm/page.c
> > @@ -689,3 +689,20 @@ void copy_page(void *to, void *from)
> >  }
> >  
> >  #endif /* CONFIG_SIBYTE_DMA_PAGEOPS */
> > +
> > +#ifdef CONFIG_FLATMEM
> > +int is_pfn_valid(unsigned long pfn)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < boot_mem_map.nr_map; i++) {
> > +		if (boot_mem_map.map[i].type == BOOT_MEM_RAM) {
> > +			if ((pfn >= PFN_DOWN(boot_mem_map.map[i].addr)) &&
> > +				((pfn) <= (PFN_UP(boot_mem_map.map[i].addr +
> >   
> 
>    Not <?
> 

Just checked it again, here should be <.

Regards,
	Wu Zhangjin

Re: [PATCH] MIPS: fix pfn_valid() for FLAGMEM

[Ralf Baechle]

On Thu, Oct 08, 2009 at 04:57:32PM +0800, Wu Zhangjin wrote:

> When CONFIG_FLAGMEM enabled, STD/Hiberation will fail on YeeLoong
> laptop, This patch fix it:
> 
> if pfn is between min_low_pfn and max_mapnr, the old pfn_valid() will
> return TRUE, but in reality, if the memory is not continuous, it should
> be false. for example:

Hm...  All that pfn_valid() indicates is that a page frame number is valid
to index a pfn.  That is that a pfn is valid to index the mem_map array.

> $ cat /proc/iomem | grep "System RAM"
> 00000000-0fffffff : System RAM
> 90000000-bfffffff : System RAM
> 
> as we can see, it is not continuous, so, some of the memory is not valid
> but regarded as valid by pfn_valid(), and at last make STD/Hibernate
> fail when shrinking a too large number of invalid memory.
> 
> Here, we fix it via checking pfn is in the "System RAM" or not, if yes,
> return TRUE.

Are the non-memory parts marked as reserved?

  Ralf

Re: [PATCH] MIPS: fix pfn_valid() for FLAGMEM

[Wu Zhangjin]

On Thu, 2009-10-08 at 16:42 +0200, Ralf Baechle wrote:
> On Thu, Oct 08, 2009 at 04:57:32PM +0800, Wu Zhangjin wrote:
> 
> > When CONFIG_FLAGMEM enabled, STD/Hiberation will fail on YeeLoong
> > laptop, This patch fix it:
> > 
> > if pfn is between min_low_pfn and max_mapnr, the old pfn_valid() will
> > return TRUE, but in reality, if the memory is not continuous, it should
> > be false. for example:
> 
> Hm...  All that pfn_valid() indicates is that a page frame number is valid
> to index a pfn.  That is that a pfn is valid to index the mem_map array.
> 
> > $ cat /proc/iomem | grep "System RAM"
> > 00000000-0fffffff : System RAM
> > 90000000-bfffffff : System RAM
> > 
> > as we can see, it is not continuous, so, some of the memory is not valid
> > but regarded as valid by pfn_valid(), and at last make STD/Hibernate
> > fail when shrinking a too large number of invalid memory.
> > 
> > Here, we fix it via checking pfn is in the "System RAM" or not, if yes,
> > return TRUE.
> 
> Are the non-memory parts marked as reserved?
> 
No, so, is that a need to mark them?

Regrads,
	Wu Zhangjin

Re: [PATCH] MIPS: fix pfn_valid() for FLAGMEM

[Ralf Baechle]

On Thu, Oct 08, 2009 at 11:46:00PM +0800, Wu Zhangjin wrote:

> > Are the non-memory parts marked as reserved?
> > 
> No, so, is that a need to mark them?

Initially all pages are marked as reserved.

Which seems to be good enough for x86:

$ cat /proc/iomem
00000000-0009efff : System RAM
0009f000-0009ffff : reserved
000c0000-000cffff : pnp 00:0d
000e0000-000fffff : pnp 00:0d
00100000-7fe5b7ff : System RAM
[...]

The 0x9f000 - 0x9ffff range is the good old ISA I/O memory range (classic
MDA/CGA/VGA etc.), that is non-memory yet:

#ifdef CONFIG_FLATMEM
#define pfn_valid(pfn)          ((pfn) < max_mapnr)
#endif /* CONFIG_FLATMEM */

is sufficient on x86 so I think something else must be wrong.

  Ralf

Re: [PATCH] MIPS: fix pfn_valid() for FLAGMEM

[Sergei Shtylyov]

Hello.

Ralf Baechle wrote:

>>>Are the non-memory parts marked as reserved?

>>No, so, is that a need to mark them?

> Initially all pages are marked as reserved.

> Which seems to be good enough for x86:

> $ cat /proc/iomem
> 00000000-0009efff : System RAM
> 0009f000-0009ffff : reserved
> 000c0000-000cffff : pnp 00:0d
> 000e0000-000fffff : pnp 00:0d
> 00100000-7fe5b7ff : System RAM
> [...]

> The 0x9f000 - 0x9ffff range is the good old ISA I/O memory range (classic
> MDA/CGA/VGA etc.), that is non-memory yet:

    Not really, it's a usual RAM. CGA/VGA video memory occupies 
000a0000-000bffff and MDA occupies 000b0000-000b7ffff (if not less).  This 
range is probably reserved by BIOS for something like EBDA...

WBR, Sergei

Re: [PATCH] MIPS: fix pfn_valid() for FLAGMEM

[Sergei Shtylyov]

Hello, I wrote:

>>>> Are the non-memory parts marked as reserved?

>>> No, so, is that a need to mark them?

>> Initially all pages are marked as reserved.

>> Which seems to be good enough for x86:

>> $ cat /proc/iomem
>> 00000000-0009efff : System RAM
>> 0009f000-0009ffff : reserved
>> 000c0000-000cffff : pnp 00:0d
>> 000e0000-000fffff : pnp 00:0d
>> 00100000-7fe5b7ff : System RAM
>> [...]

>> The 0x9f000 - 0x9ffff range is the good old ISA I/O memory range (classic
>> MDA/CGA/VGA etc.), that is non-memory yet:
> 
> 
>    Not really, it's a usual RAM. CGA/VGA video memory occupies 

    Oh, I meant EGA/VGA. CGA memory is at 000b8000-000bffff, of course.

> 000a0000-000bffff and MDA occupies 000b0000-000b7ffff (if not less).
> This range is probably reserved by BIOS for something like EBDA...

    Which stands for Extended BIOS Data Area.

WBR, Sergei