[PATCH 1/2] make acpi_path_name() global

[PATCH 1/2] make acpi_path_name() global

[Randy Dunlap]

From: Randy Dunlap <randy_d_dunlap-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Make acpi_path_name() usable by everyone.
I need this for adding SATA suspend/resume ACPI support.

Note:
This function is now a memory allocator and callers should kfree()
the memory when done with it.  It doesn't seem safe to me to leave
it as returning a pointer to a static buffer (as it was in multiple
cloned copies) as we add callers to it.


Signed-off-by: Randy Dunlap <randy_d_dunlap-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/acpi/osl.c      |   29 +++++++++++++++++++++++++++++
 include/acpi/acpiosxf.h |    2 ++
 2 files changed, 31 insertions(+)

--- linux-2615-rc5g5.orig/drivers/acpi/osl.c
+++ linux-2615-rc5g5/drivers/acpi/osl.c
@@ -1078,6 +1078,35 @@ void acpi_os_release_lock(acpi_handle ha
 	spin_unlock_irqrestore((spinlock_t *) handle, flags);
 }
 
+/**
+ * acpi_path_name - get ACPI path_name for the given handle
+ * @handle: ACPI object handle to look up name of
+ *
+ * Allocates memory if successul.  Caller must free the memory.
+ *
+ * Returns: pointer to path_name if successful, NULL if not.
+ */
+u8 *acpi_path_name(acpi_handle handle)
+{
+	acpi_status		status;
+	static u8		*path_name;
+	struct acpi_buffer	ret_buf = {ACPI_PATHNAME_MAX, path_name};
+
+	path_name = kzalloc(ACPI_PATHNAME_MAX, GFP_KERNEL);
+	if (!path_name)
+		return NULL;
+
+	status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &ret_buf);
+
+	if (ACPI_FAILURE(status)) {
+		acpi_os_free(path_name);
+		return NULL;
+	}
+
+	return path_name;
+}
+EXPORT_SYMBOL(acpi_path_name);
+
 #ifndef ACPI_USE_LOCAL_CACHE
 
 /*******************************************************************************
--- linux-2615-rc5g5.orig/include/acpi/acpiosxf.h
+++ linux-2615-rc5g5/include/acpi/acpiosxf.h
@@ -112,6 +112,8 @@ unsigned long acpi_os_acquire_lock(acpi_
 
 void acpi_os_release_lock(acpi_handle handle, unsigned long flags);
 
+u8 *acpi_path_name(acpi_handle handle);
+
 /*
  * Memory allocation and mapping
  */


---


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

RE: [PATCH 1/2] make acpi_path_name() global

[Brown, Len]

I think that some of our code partitioning is screwed up
and this continues and extends that tradition:-)

acpiosxf.h is supposed to declare ACPICA core things that
the OS needs to see.  osl.c is supposed to glue and
wrap the appropriate core and Linux functions together.

The big problem we have here is that every acpi sub-system
header is sucked into a consolidated header and so every
internal function is available to the entire kernel.

an example of this is acpi_get_name(), which as declared
isn't supposed to be available outside the core.
All the uses of it to print ACPI namespace tokens in /proc
are totally bogus and should be deleted.  The other use
of it is to print out debug strings -- stuff the OS
doesn't really need to "know" and maybe something the
core should do for it.  That leaves the hotplug use,
which I don't understand.  What does SATA need
with the actual path names?

thanks,
-Len

>-----Original Message-----
>From: Randy Dunlap [mailto:randy_d_dunlap-VuQAYsv1563Yd54FQh9/CA@public.gmane.org] 
>Sent: Thursday, December 15, 2005 8:20 PM
>To: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; 
>pcihpd-discuss-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>Cc: Accardi, Kristen C; Brown, Len; gregkh-l3A5Bk7waGM@public.gmane.org
>Subject: [PATCH 1/2] make acpi_path_name() global
>
>From: Randy Dunlap <randy_d_dunlap-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>
>Make acpi_path_name() usable by everyone.
>I need this for adding SATA suspend/resume ACPI support.
>
>Note:
>This function is now a memory allocator and callers should kfree()
>the memory when done with it.  It doesn't seem safe to me to leave
>it as returning a pointer to a static buffer (as it was in multiple
>cloned copies) as we add callers to it.
>
>
>Signed-off-by: Randy Dunlap <randy_d_dunlap-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>---
> drivers/acpi/osl.c      |   29 +++++++++++++++++++++++++++++
> include/acpi/acpiosxf.h |    2 ++
> 2 files changed, 31 insertions(+)
>
>--- linux-2615-rc5g5.orig/drivers/acpi/osl.c
>+++ linux-2615-rc5g5/drivers/acpi/osl.c
>@@ -1078,6 +1078,35 @@ void acpi_os_release_lock(acpi_handle ha
> 	spin_unlock_irqrestore((spinlock_t *) handle, flags);
> }
> 
>+/**
>+ * acpi_path_name - get ACPI path_name for the given handle
>+ * @handle: ACPI object handle to look up name of
>+ *
>+ * Allocates memory if successul.  Caller must free the memory.
>+ *
>+ * Returns: pointer to path_name if successful, NULL if not.
>+ */
>+u8 *acpi_path_name(acpi_handle handle)
>+{
>+	acpi_status		status;
>+	static u8		*path_name;
>+	struct acpi_buffer	ret_buf = {ACPI_PATHNAME_MAX, 
>path_name};
>+
>+	path_name = kzalloc(ACPI_PATHNAME_MAX, GFP_KERNEL);
>+	if (!path_name)
>+		return NULL;
>+
>+	status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &ret_buf);
>+
>+	if (ACPI_FAILURE(status)) {
>+		acpi_os_free(path_name);
>+		return NULL;
>+	}
>+
>+	return path_name;
>+}
>+EXPORT_SYMBOL(acpi_path_name);
>+
> #ifndef ACPI_USE_LOCAL_CACHE
> 
> 
>/**************************************************************
>*****************
>--- linux-2615-rc5g5.orig/include/acpi/acpiosxf.h
>+++ linux-2615-rc5g5/include/acpi/acpiosxf.h
>@@ -112,6 +112,8 @@ unsigned long acpi_os_acquire_lock(acpi_
> 
> void acpi_os_release_lock(acpi_handle handle, unsigned long flags);
> 
>+u8 *acpi_path_name(acpi_handle handle);
>+
> /*
>  * Memory allocation and mapping
>  */
>
>
>---
>


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_idv37&alloc_id\x16865&op=click

Re: RE: [PATCH 1/2] make acpi_path_name() global

[Randy.Dunlap]

On Thu, 15 Dec 2005 23:56:47 -0500 Brown, Len wrote:

> I think that some of our code partitioning is screwed up
> and this continues and extends that tradition:-)

I won't deny or argue with you there.

> acpiosxf.h is supposed to declare ACPICA core things that
> the OS needs to see.  osl.c is supposed to glue and
> wrap the appropriate core and Linux functions together.
> 
> The big problem we have here is that every acpi sub-system
> header is sucked into a consolidated header and so every
> internal function is available to the entire kernel.
> 
> an example of this is acpi_get_name(), which as declared
> isn't supposed to be available outside the core.
> All the uses of it to print ACPI namespace tokens in /proc
> are totally bogus and should be deleted.  The other use
> of it is to print out debug strings -- stuff the OS
> doesn't really need to "know" and maybe something the
> core should do for it.  That leaves the hotplug use,
> which I don't understand.  What does SATA need
> with the actual path names?

SATA uses the name string for (a) debugging, as you have
mentioned, and (b) for comparing complete device strings
to match/find a SATA _ADR under a device (in a device's
scope).  I can do without (a) as well as the rest of the
kernel can, of course.  I don't know how to find and match
SATA devices (part b) without it, so this is an opportunity
for you to educate me.  :)


> thanks,
> -Len
> 
> >-----Original Message-----
> >From: Randy Dunlap [mailto:randy_d_dunlap-VuQAYsv1563Yd54FQh9/CA@public.gmane.org] 
> >Sent: Thursday, December 15, 2005 8:20 PM
> >To: acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; 
> >pcihpd-discuss-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> >Cc: Accardi, Kristen C; Brown, Len; gregkh-l3A5Bk7waGM@public.gmane.org
> >Subject: [PATCH 1/2] make acpi_path_name() global
> >
> >From: Randy Dunlap <randy_d_dunlap-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> >
> >Make acpi_path_name() usable by everyone.
> >I need this for adding SATA suspend/resume ACPI support.
> >
> >Note:
> >This function is now a memory allocator and callers should kfree()
> >the memory when done with it.  It doesn't seem safe to me to leave
> >it as returning a pointer to a static buffer (as it was in multiple
> >cloned copies) as we add callers to it.
> >
> >
> >Signed-off-by: Randy Dunlap <randy_d_dunlap-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> >---
> > drivers/acpi/osl.c      |   29 +++++++++++++++++++++++++++++
> > include/acpi/acpiosxf.h |    2 ++
> > 2 files changed, 31 insertions(+)
> >
> >--- linux-2615-rc5g5.orig/drivers/acpi/osl.c
> >+++ linux-2615-rc5g5/drivers/acpi/osl.c
> >@@ -1078,6 +1078,35 @@ void acpi_os_release_lock(acpi_handle ha
> > 	spin_unlock_irqrestore((spinlock_t *) handle, flags);
> > }
> > 
> >+/**
> >+ * acpi_path_name - get ACPI path_name for the given handle
> >+ * @handle: ACPI object handle to look up name of
> >+ *
> >+ * Allocates memory if successul.  Caller must free the memory.
> >+ *
> >+ * Returns: pointer to path_name if successful, NULL if not.
> >+ */
> >+u8 *acpi_path_name(acpi_handle handle)
> >+{
> >+	acpi_status		status;
> >+	static u8		*path_name;
> >+	struct acpi_buffer	ret_buf = {ACPI_PATHNAME_MAX, 
> >path_name};
> >+
> >+	path_name = kzalloc(ACPI_PATHNAME_MAX, GFP_KERNEL);
> >+	if (!path_name)
> >+		return NULL;
> >+
> >+	status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &ret_buf);
> >+
> >+	if (ACPI_FAILURE(status)) {
> >+		acpi_os_free(path_name);
> >+		return NULL;
> >+	}
> >+
> >+	return path_name;
> >+}
> >+EXPORT_SYMBOL(acpi_path_name);
> >+
> > #ifndef ACPI_USE_LOCAL_CACHE
> > 
> > 
> >/**************************************************************
> >*****************
> >--- linux-2615-rc5g5.orig/include/acpi/acpiosxf.h
> >+++ linux-2615-rc5g5/include/acpi/acpiosxf.h
> >@@ -112,6 +112,8 @@ unsigned long acpi_os_acquire_lock(acpi_
> > 
> > void acpi_os_release_lock(acpi_handle handle, unsigned long flags);
> > 
> >+u8 *acpi_path_name(acpi_handle handle);
> >+
> > /*
> >  * Memory allocation and mapping
> >  */


---
~Randy


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: RE: [PATCH 1/2] make acpi_path_name() global

[Kristen Accardi]

On Thu, 2005-12-15 at 21:27 -0800, Randy.Dunlap wrote:
> On Thu, 15 Dec 2005 23:56:47 -0500 Brown, Len wrote:

<snip>
> > an example of this is acpi_get_name(), which as declared
> > isn't supposed to be available outside the core.
> > All the uses of it to print ACPI namespace tokens in /proc
> > are totally bogus and should be deleted.  The other use
> > of it is to print out debug strings -- stuff the OS
> > doesn't really need to "know" and maybe something the
> > core should do for it.  That leaves the hotplug use,
> > which I don't understand.  What does SATA need
> > with the actual path names?
> 
> SATA uses the name string for (a) debugging, as you have
> mentioned, and (b) for comparing complete device strings
> to match/find a SATA _ADR under a device (in a device's
> scope).  I can do without (a) as well as the rest of the
> kernel can, of course.  I don't know how to find and match
> SATA devices (part b) without it, so this is an opportunity
> for you to educate me.  :)
> 
> 
We use this in pciehp and shpchp as well for the same reason.  One thing
I don't love about these patches is the need now to worry about freeing
memory after you make these calls.  To me that seems like asking for
memory leaks in error paths at some point down the road when someone
less careful than Randy adds some code in.  If there were some way to
return a pointer to a static string, I'd prefer that solution, but I
can't figure out from looking at the acpi code if that is possible.  If
there's some better way to perform this compare, I'd like to hear it
too.

Kristen



-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

Re: [PATCH 1/2] make acpi_path_name() global

[Rolf Eike Beer]

> On Thu, 2005-12-15 at 21:27 -0800, Randy.Dunlap wrote:
>> On Thu, 15 Dec 2005 23:56:47 -0500 Brown, Len wrote:
>
> <snip>
>> > an example of this is acpi_get_name(), which as declared
>> > isn't supposed to be available outside the core.
>> > All the uses of it to print ACPI namespace tokens in /proc
>> > are totally bogus and should be deleted.  The other use
>> > of it is to print out debug strings -- stuff the OS
>> > doesn't really need to "know" and maybe something the
>> > core should do for it.  That leaves the hotplug use,
>> > which I don't understand.  What does SATA need
>> > with the actual path names?
>>
>> SATA uses the name string for (a) debugging, as you have
>> mentioned, and (b) for comparing complete device strings
>> to match/find a SATA _ADR under a device (in a device's
>> scope).  I can do without (a) as well as the rest of the
>> kernel can, of course.  I don't know how to find and match
>> SATA devices (part b) without it, so this is an opportunity
>> for you to educate me.  :)
>>
>>
> We use this in pciehp and shpchp as well for the same reason.  One thing
> I don't love about these patches is the need now to worry about freeing
> memory after you make these calls.  To me that seems like asking for
> memory leaks in error paths at some point down the road when someone
> less careful than Randy adds some code in.  If there were some way to
> return a pointer to a static string, I'd prefer that solution, but I
> can't figure out from looking at the acpi code if that is possible.  If
> there's some better way to perform this compare, I'd like to hear it
> too.

Don't do the allocation in the function but pass a buffer and it's length
to it. The caller can do dynamic allocation if needed or use a buffer on
stack if not. This also makes error handling easier as ENOMEM can't happen
in acpi_path_name() anymore.

Eike


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click

RE: [PATCH 1/2] make acpi_path_name() global

[Moore, Robert]

That is correct, all ACPICA interfaces that require a buffer parameter
allow the buffer object to be pre-initialized with a buffer allocated by
the caller or allow ACPICA to allocate the buffer.

Bob


> -----Original Message-----
> From: Rolf Eike Beer [mailto:eike-B90SbfZsE4eELgA04lAiVw@public.gmane.org]
> Sent: Friday, December 16, 2005 1:04 PM
> To: Accardi, Kristen C
> Cc: Randy.Dunlap; Brown, Len; randy_d_dunlap-VuQAYsv1563Yd54FQh9/CA@public.gmane.org; acpi-
> devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; pcihpd-discuss-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org;
> gregkh-l3A5Bk7waGM@public.gmane.org; Moore, Robert
> Subject: Re: [PATCH 1/2] make acpi_path_name() global
> 
> > On Thu, 2005-12-15 at 21:27 -0800, Randy.Dunlap wrote:
> >> On Thu, 15 Dec 2005 23:56:47 -0500 Brown, Len wrote:
> >
> > <snip>
> >> > an example of this is acpi_get_name(), which as declared
> >> > isn't supposed to be available outside the core.
> >> > All the uses of it to print ACPI namespace tokens in /proc
> >> > are totally bogus and should be deleted.  The other use
> >> > of it is to print out debug strings -- stuff the OS
> >> > doesn't really need to "know" and maybe something the
> >> > core should do for it.  That leaves the hotplug use,
> >> > which I don't understand.  What does SATA need
> >> > with the actual path names?
> >>
> >> SATA uses the name string for (a) debugging, as you have
> >> mentioned, and (b) for comparing complete device strings
> >> to match/find a SATA _ADR under a device (in a device's
> >> scope).  I can do without (a) as well as the rest of the
> >> kernel can, of course.  I don't know how to find and match
> >> SATA devices (part b) without it, so this is an opportunity
> >> for you to educate me.  :)
> >>
> >>
> > We use this in pciehp and shpchp as well for the same reason.  One
thing
> > I don't love about these patches is the need now to worry about
freeing
> > memory after you make these calls.  To me that seems like asking for
> > memory leaks in error paths at some point down the road when someone
> > less careful than Randy adds some code in.  If there were some way
to
> > return a pointer to a static string, I'd prefer that solution, but I
> > can't figure out from looking at the acpi code if that is possible.
If
> > there's some better way to perform this compare, I'd like to hear it
> > too.
> 
> Don't do the allocation in the function but pass a buffer and it's
length
> to it. The caller can do dynamic allocation if needed or use a buffer
on
> stack if not. This also makes error handling easier as ENOMEM can't
happen
> in acpi_path_name() anymore.
> 
> Eike


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://ads.osdn.com/?ad_idv37&alloc_id\x16865&op=click