[PATCH] Re: 2.6.21-rc7-mm2 -- hvsi console driver registration failure

[PATCH] Re: 2.6.21-rc7-mm2 -- hvsi console driver registration failure

[Andy Whitcroft]

Trying to get 2.6.21-rc7-mm2 to boot on large PPC64 seems to be a
bit of a challenge.  We have been seeing panics on boot from the
hvsi driver:

	Couldn't register hvsi console driver

Tracking this back, this seems to come from hvsi driver trying to
register itself via tty_register_driver() with a zero units.

The failure is triggered by a change in semantics for kmalloc()
between SLAB and SLUB; kmalloc(0) now returns NULL rather than an
allocation at the smallest size.  Looking at the code in question
even when the allocation succeeds we will not actually use the
memory when device->num is zero.

It is not clear to me if this is a bug in the hvsi driver in that
it should specify some units.  It seems we will try and reserve zero
devices in this case, which seems pointless.

I have tested with the patch below which seems safe to me and stops
the errors and even seems to make the console work.  But perhaps
someone with more driver fu, could verify if driver->num of zero
has any meaning and kick this to the hvsi people if not.

-apw

=== 8< ===
tty_register_driver: only allocate tty instances when defined

If device->num is zero we attempt to kmalloc() zero bytes.
When SLUB is enabled this returns a null pointer and take that as
an allocation failure and fail the device register.  Check for no
devices and avoid the allocation.

Signed-off-by: Andy Whitcroft <apw@shadowen.org>
---
diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
index 959a616..71c4579 100644
--- a/drivers/char/tty_io.c
+++ b/drivers/char/tty_io.c
@@ -3724,7 +3724,7 @@ int tty_register_driver(struct tty_driver *driver)
 	if (driver->flags & TTY_DRIVER_INSTALLED)
 		return 0;
 
-	if (!(driver->flags & TTY_DRIVER_DEVPTS_MEM)) {
+	if (!(driver->flags & TTY_DRIVER_DEVPTS_MEM) && driver->num) {
 		p = kmalloc(driver->num * 3 * sizeof(void *), GFP_KERNEL);
 		if (!p)
 			return -ENOMEM;

Re: [PATCH] Re: 2.6.21-rc7-mm2 -- hvsi console driver registration failure

[Linas Vepstas]

On Fri, May 04, 2007 at 12:38:58PM +0100, Andy Whitcroft wrote:
> 
> Trying to get 2.6.21-rc7-mm2 to boot on large PPC64 seems to be a
> bit of a challenge.  We have been seeing panics on boot from the
> hvsi driver:
> 
> 	Couldn't register hvsi console driver
> 
> Tracking this back, this seems to come from hvsi driver trying to
> register itself via tty_register_driver() with a zero units.
> 
> The failure is triggered by a change in semantics for kmalloc()
> between SLAB and SLUB; kmalloc(0) now returns NULL rather than an
> allocation at the smallest size.  Looking at the code in question
> even when the allocation succeeds we will not actually use the
> memory when device->num is zero.
> 
> It is not clear to me if this is a bug in the hvsi driver in that
> it should specify some units.  It seems we will try and reserve zero
> devices in this case, which seems pointless.

Yes, it seems pointless to me ... 

> I have tested with the patch below which seems safe to me and stops
> the errors and even seems to make the console work.  But perhaps
> someone with more driver fu, could verify if driver->num of zero
> has any meaning and kick this to the hvsi people if not.

Hollis nominated me to be "hvsi people", although I'm near-totally
ignorant of the thing.

If hvsi_count is zero, then the device tree did not have any
"serial" nodes that speak "hvterm-protocol". The hvsi should not 
have even tried to register anything. The attached patch seems more 
to the point.

--linas


The hvsi driver is used whenever the device-tree contains
nodes for serial ports, and those serial ports speak the hvterm
protocol. However, if no such nodes are found, then the hvsi
driver should not even register. 

This patch avoids a kernel panic with "Couldn't register hvsi 
console driver". 

In addition, this patch makes tty_register_driver refuse
to do anything, if there are no actual tty ports to be 
registered.

Utterly & completely untested.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>

----
 drivers/char/hvsi.c   |    4 ++++
 drivers/char/tty_io.c |    3 +++
 2 files changed, 7 insertions(+)

Index: linux-2.6.21-rc7-mm2/drivers/char/hvsi.c
===================================================================
--- linux-2.6.21-rc7-mm2.orig/drivers/char/hvsi.c	2007-04-26 15:37:33.000000000 -0500
+++ linux-2.6.21-rc7-mm2/drivers/char/hvsi.c	2007-05-04 13:55:56.000000000 -0500
@@ -1148,6 +1148,10 @@ static int __init hvsi_init(void)
 {
 	int i;
 
+	/* No serial hvterm-protocol device-tree nodes found. */
+	if (hvsi_count == 0)
+		return 0;
+
 	hvsi_driver = alloc_tty_driver(hvsi_count);
 	if (!hvsi_driver)
 		return -ENOMEM;
Index: linux-2.6.21-rc7-mm2/drivers/char/tty_io.c
===================================================================
--- linux-2.6.21-rc7-mm2.orig/drivers/char/tty_io.c	2007-04-26 15:37:33.000000000 -0500
+++ linux-2.6.21-rc7-mm2/drivers/char/tty_io.c	2007-05-04 13:54:14.000000000 -0500
@@ -3724,6 +3724,9 @@ int tty_register_driver(struct tty_drive
 	if (driver->flags & TTY_DRIVER_INSTALLED)
 		return 0;
 
+	if (driver->num == 0)
+		return -ENODEV;
+
 	if (!(driver->flags & TTY_DRIVER_DEVPTS_MEM)) {
 		p = kmalloc(driver->num * 3 * sizeof(void *), GFP_KERNEL);
 		if (!p)

Re: [PATCH] Re: 2.6.21-rc7-mm2 -- hvsi console driver registration failure

[Andrew Morton]

On Fri, 04 May 2007 12:38:58 +0100
Andy Whitcroft <apw@shadowen.org> wrote:

> 
> Trying to get 2.6.21-rc7-mm2 to boot on large PPC64 seems to be a
> bit of a challenge.  We have been seeing panics on boot from the
> hvsi driver:
> 
> 	Couldn't register hvsi console driver
> 
> Tracking this back, this seems to come from hvsi driver trying to
> register itself via tty_register_driver() with a zero units.
> 
> The failure is triggered by a change in semantics for kmalloc()
> between SLAB and SLUB; kmalloc(0) now returns NULL rather than an
> allocation at the smallest size.  Looking at the code in question
> even when the allocation succeeds we will not actually use the
> memory when device->num is zero.

OK, thanks for working that out.

Christoph, we should be emitting loud warnings so that this problem is easy
to debug.

Better, we should be emitting loud warnigns which then disable themselves
and then succeeding the allocation so that people can proceed with their
kernel testing.

When all the loud-warning sites have been fixed, we can take that code out
again.

The present situation is maximally tester-hostile.

> It is not clear to me if this is a bug in the hvsi driver in that
> it should specify some units.  It seems we will try and reserve zero
> devices in this case, which seems pointless.
> 
> I have tested with the patch below which seems safe to me and stops
> the errors and even seems to make the console work.  But perhaps
> someone with more driver fu, could verify if driver->num of zero
> has any meaning and kick this to the hvsi people if not.
> 
> -apw
> 
> === 8< ===
> tty_register_driver: only allocate tty instances when defined
> 
> If device->num is zero we attempt to kmalloc() zero bytes.
> When SLUB is enabled this returns a null pointer and take that as
> an allocation failure and fail the device register.  Check for no
> devices and avoid the allocation.
> 
> Signed-off-by: Andy Whitcroft <apw@shadowen.org>
> ---
> diff --git a/drivers/char/tty_io.c b/drivers/char/tty_io.c
> index 959a616..71c4579 100644
> --- a/drivers/char/tty_io.c
> +++ b/drivers/char/tty_io.c
> @@ -3724,7 +3724,7 @@ int tty_register_driver(struct tty_driver *driver)
>  	if (driver->flags & TTY_DRIVER_INSTALLED)
>  		return 0;
>  
> -	if (!(driver->flags & TTY_DRIVER_DEVPTS_MEM)) {
> +	if (!(driver->flags & TTY_DRIVER_DEVPTS_MEM) && driver->num) {
>  		p = kmalloc(driver->num * 3 * sizeof(void *), GFP_KERNEL);
>  		if (!p)
>  			return -ENOMEM;

Re: [PATCH] Re: 2.6.21-rc7-mm2 -- hvsi console driver registration failure

[Christoph Lameter]

On Fri, 4 May 2007, Andrew Morton wrote:

> Better, we should be emitting loud warnigns which then disable themselves
> and then succeeding the allocation so that people can proceed with their
> kernel testing.
> 
> When all the loud-warning sites have been fixed, we can take that code out
> again.
> 
> The present situation is maximally tester-hostile.
i

SLUB: Allocate smallest object size if the user asks for 0 bytes.

Makes SLUB behave like SLAB in this area to avoid issues....

Throw a stack dump to alert people.

At some point the behavior should be switched back. NULL is no
memory as far as I can tell and if the use asked for 0 bytes then
he need to get no memory.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

---
 include/linux/slub_def.h |    8 ++++++--
 mm/slub.c                |    2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

Index: slub/mm/slub.c
===================================================================
--- slub.orig/mm/slub.c	2007-05-04 14:17:22.000000000 -0700
+++ slub/mm/slub.c	2007-05-04 14:19:36.000000000 -0700
@@ -2009,7 +2009,7 @@ static struct kmem_cache *get_slab(size_
 {
 	int index = kmalloc_index(size);
 
-	if (!size)
+	if (!index)
 		return NULL;
 
 	/* Allocation too large? */
Index: slub/include/linux/slub_def.h
===================================================================
--- slub.orig/include/linux/slub_def.h	2007-05-04 14:13:40.000000000 -0700
+++ slub/include/linux/slub_def.h	2007-05-04 14:18:25.000000000 -0700
@@ -81,8 +81,12 @@ extern struct kmem_cache kmalloc_caches[
  */
 static inline int kmalloc_index(int size)
 {
-	if (size == 0)
-		return 0;
+	/*
+	 * We should return 0 if size == 0 but we use the smallest object
+	 * here for SLAB legacy reasons.
+	 */
+	WARN_ON(size == 0);
+
 	if (size > 64 && size <= 96)
 		return 1;
 	if (size > 128 && size <= 192)