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

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: RE: [PATCH 1/2] make acpi_path_name() global

[Rajesh Shah]

On Thu, Dec 15, 2005 at 11:56:47PM -0500, Brown, Len wrote:
> 
> 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

Hotplug drivers use it to print where we found good, bad or missing
ACPI methods like _HPP, OSHP, _OSC that we need to work properly.
This is very useful as a debug aid, since ACPI namespace problems
have always been the number one reason why the hotplug drivers
fail to work.

thanks,
Rajesh



-------------------------------------------------------
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

[Moore, Robert]

AcpiGetName is in fact an external interface to ACPICA, and is declared
in acpixf.h.

The three headers that are intended to be "public" are as follows:

Acpiosxf.h - Interfaces to be implemented by the OS interface layer
(OSL).
Acpixf.h - Interfaces to ACPICA, to be used by drivers, etc.
Actypes.h - Types and structs required to use the public interfaces.

The naming convention for ACPICA is that all "non-public" interfaces
contain the prefix "Acpi??" where ?? is the short abbreviation for the
component (Ut = Utilities, Dm = Disassembler, etc.) Anything without the
?? abbreviation is a public interface. Rather than use another
abbreviation, we decided to just drop it for the public interfaces to
keep things short and descriptive.

To clarify things for the world, I would have no problem with renaming
the existing "acpi.h" to something else that indicates it is internal
only, and then renaming actypes.h to acpi.h (or acpica.h) and pulling in
both acpiosxf.h and acpixf.h within it.

This way, we could export acpi.h (or acpica.h) to the world as the
external header for ACPICA.

Bob


> -----Original Message-----
> From: Rajesh Shah [mailto:rajesh.shah-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org]
> Sent: Friday, December 16, 2005 10:39 AM
> To: Brown, Len
> Cc: Randy Dunlap; acpi-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; pcihpd-
> discuss-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Accardi, Kristen C; gregkh-l3A5Bk7waGM@public.gmane.org;
Moore,
> Robert
> Subject: Re: [ACPI] RE: [PATCH 1/2] make acpi_path_name() global
> 
> On Thu, Dec 15, 2005 at 11:56:47PM -0500, Brown, Len wrote:
> >
> > 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
> 
> Hotplug drivers use it to print where we found good, bad or missing
> ACPI methods like _HPP, OSHP, _OSC that we need to work properly.
> This is very useful as a debug aid, since ACPI namespace problems
> have always been the number one reason why the hotplug drivers
> fail to work.
> 
> thanks,
> Rajesh



-------------------------------------------------------
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: [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