salinfo incorrectly returning -ENOMEM

salinfo incorrectly returning -ENOMEM

[dann frazier]

salinfo_decode will silently exit occasionally due to a failed open
of /proc/sal/XXX/data.  This is because the kernel is returning -ENOMEM
after attempting to vmalloc a buffer of size  
ia64_sal_get_state_info_size(data->type).

However, on my system ia64_sal_get_state_info_size is returning 0, in
which case I'd think a NULL vmalloc() result is correct.  

This patch assumes, of course, that 0 is a reasonable return value from
ia64_sal_get_state_info_size().

Signed-off-by: dann frazier <dannf@hp.com>

--- linux-2.6.16-rc1/arch/ia64/kernel/salinfo.c.orig	2006-01-27 14:44:20.000000000 -0700
+++ linux-2.6.16-rc1/arch/ia64/kernel/salinfo.c	2006-01-27 14:45:39.000000000 -0700
@@ -358,6 +358,7 @@ salinfo_log_open(struct inode *inode, st
 {
 	struct proc_dir_entry *entry = PDE(inode);
 	struct salinfo_data *data = entry->data;
+	u64 size;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -370,8 +371,9 @@ salinfo_log_open(struct inode *inode, st
 	data->open = 1;
 	spin_unlock(&data_lock);
 
-	if (data->state = STATE_NO_DATA &&
-	    !(data->log_buffer = vmalloc(ia64_sal_get_state_info_size(data->type)))) {
+	size = ia64_sal_get_state_info_size(data->type);
+	if (data->state = STATE_NO_DATA && size &&
+	    !(data->log_buffer = vmalloc(size))) {
 		data->open = 0;
 		return -ENOMEM;
 	}

Re: salinfo incorrectly returning -ENOMEM

[Prarit Bhargava]

[-- Attachment #1: Type: text/plain, Size: 971 bytes --]

dann frazier wrote:
> salinfo_decode will silently exit occasionally due to a failed open
> of /proc/sal/XXX/data.  This is because the kernel is returning -ENOMEM
> after attempting to vmalloc a buffer of size  
> ia64_sal_get_state_info_size(data->type).
> 
> However, on my system ia64_sal_get_state_info_size is returning 0, in
> which case I'd think a NULL vmalloc() result is correct.  
> 
> This patch assumes, of course, that 0 is a reasonable return value from
> ia64_sal_get_state_info_size().
> 

I'm working on fixing this up ...

The issue here is that ia64_sal_get_state_info_size is not doing any 
test on ia64_sal_retval.status other than to return 0.  0 is a 
reasonable return on an error, but :) as you noted the caller better be 
watching for it.

Dann, this patch will at least tell you what that return value is and 
might a) indicate a bigger bug, or b) indicate what's wrong with your 
platform ;).

(Patch is against Tony's latest tree)

P.

> 


[-- Attachment #2: dann.patch --]
[-- Type: text/plain, Size: 489 bytes --]

diff --git a/include/asm-ia64/sal.h b/include/asm-ia64/sal.h
--- a/include/asm-ia64/sal.h
+++ b/include/asm-ia64/sal.h
@@ -700,8 +700,11 @@ ia64_sal_get_state_info (u64 sal_info_ty
 	struct ia64_sal_retval isrv;
 	SAL_CALL_REENTRANT(isrv, SAL_GET_STATE_INFO, sal_info_type, 0,
 	              sal_info, 0, 0, 0, 0);
-	if (isrv.status)
+	if (isrv.status) {
+		printk("ia64_sal_get_state_info failed: %s\n",
+		       ia64_sal_strerror(isrv.status));
 		return 0;
+	}
 
 	return isrv.v0;
 }

Re: salinfo incorrectly returning -ENOMEM

[dann frazier]

On Fri, 2006-01-27 at 17:45 -0500, Prarit Bhargava wrote:
> dann frazier wrote:
> > salinfo_decode will silently exit occasionally due to a failed open
> > of /proc/sal/XXX/data.  This is because the kernel is returning -ENOMEM
> > after attempting to vmalloc a buffer of size  
> > ia64_sal_get_state_info_size(data->type).
> > 
> > However, on my system ia64_sal_get_state_info_size is returning 0, in
> > which case I'd think a NULL vmalloc() result is correct.  
> > 
> > This patch assumes, of course, that 0 is a reasonable return value from
> > ia64_sal_get_state_info_size().
> > 
> 
> I'm working on fixing this up ...
> 
> The issue here is that ia64_sal_get_state_info_size is not doing any 
> test on ia64_sal_retval.status other than to return 0.  0 is a 
> reasonable return on an error, but :) as you noted the caller better be 
> watching for it.
> 
> Dann, this patch will at least tell you what that return value is and 
> might a) indicate a bigger bug, or b) indicate what's wrong with your 
> platform ;).

Thanks Prarit.  Here's the output I get:

ia64_sal_get_state_info failed: Call completed with error
ia64_sal_get_state_info failed: Call completed with error

Useful, eh? :)

Re: salinfo incorrectly returning -ENOMEM

[Keith Owens]

dann frazier (on Fri, 27 Jan 2006 15:21:06 -0700) wrote:
>salinfo_decode will silently exit occasionally due to a failed open
>of /proc/sal/XXX/data.  This is because the kernel is returning -ENOMEM
>after attempting to vmalloc a buffer of size  
>ia64_sal_get_state_info_size(data->type).
>
>However, on my system ia64_sal_get_state_info_size is returning 0, in
>which case I'd think a NULL vmalloc() result is correct.  
>
>This patch assumes, of course, that 0 is a reasonable return value from
>ia64_sal_get_state_info_size().

Nak this patch.

Zero is not a valid return value from ia64_sal_get_state_info_size().
That SAL call must return the maximum size for the record type so that
the OS can preallocate the required buffers while the OS is in normal
context.  The buffers cannot be allocated when the SAL interrupt occurs
because vmalloc cannot be used in interrupt context.  If SAL is not
returning a valid value from ia64_sal_get_state_info_size() then your
version of SAL is wrong and the only thing that the OS can do is to
give up on that record type.  So an error return to salinfo_decode is
the correct response here.

You said that salinfo_decode will silently exit.  Have you tried
salinfo 1.0, that logs why the salinfo_decode failed and retries it a
few times.

Re: salinfo incorrectly returning -ENOMEM

[dann frazier]

On Sat, 2006-01-28 at 11:07 +1100, Keith Owens wrote:
> dann frazier (on Fri, 27 Jan 2006 15:21:06 -0700) wrote:
> >salinfo_decode will silently exit occasionally due to a failed open
> >of /proc/sal/XXX/data.  This is because the kernel is returning -ENOMEM
> >after attempting to vmalloc a buffer of size  
> >ia64_sal_get_state_info_size(data->type).
> >
> >However, on my system ia64_sal_get_state_info_size is returning 0, in
> >which case I'd think a NULL vmalloc() result is correct.  
> >
> >This patch assumes, of course, that 0 is a reasonable return value from
> >ia64_sal_get_state_info_size().
> 
> Nak this patch.
> 
> Zero is not a valid return value from ia64_sal_get_state_info_size().
> That SAL call must return the maximum size for the record type so that
> the OS can preallocate the required buffers while the OS is in normal
> context.  The buffers cannot be allocated when the SAL interrupt occurs
> because vmalloc cannot be used in interrupt context.  If SAL is not
> returning a valid value from ia64_sal_get_state_info_size() then your
> version of SAL is wrong and the only thing that the OS can do is to
> give up on that record type.  

Just to follow-up - we've determined that is an issue with the version
of firmware some of our machines are running.  Thanks for everyone's
responses.