RE: [ACPI] [PATCH] 1/3 ACPI resource handling

RE: [ACPI] [PATCH] 1/3 ACPI resource handling

[Moore, Robert]

1) This seems like a good idea to simplify the parsing of the resource lists

2) I'm not convinced that this buys a whole lot -- it just hides the code
behind a macro (something that's not generally liked in the Linux world.)
Would this procedure be called from more than one place?

3) Fallout from (1), ok.

Bob


-----Original Message-----
From: Bjorn Helgaas [mailto:bjorn_helgaas@hp.com] 
Sent: Tuesday, February 11, 2003 1:58 PM
To: Grover, Andrew
Cc: t-kochi@bq.jp.nec.com; linux-kernel@vger.kernel.org;
acpi-devel@lists.sourceforge.net
Subject: [ACPI] [PATCH] 1/3 ACPI resource handling

These patches against acpi-20030123-2.5.59.diff.gz make it more
convenient to handle ACPI resources and reduce code duplication.

The changes fall into three pieces:

    1) Addition of acpi_walk_resources(), which calls a callback for
       each resource in _CRS or _PRS.  This is handy because you don't
       have to allocate/free buffers, loop over resource lists, or
       make assumptions about the order in which resources appear.

    2) Addition of acpi_resource_to_address64(), which copies
       address16, address32, or address64 resources to an address64
       structure, so you don't have to maintain three sets of code
       that differ only in the size of the address they deal with.

    3) Changes to the EC, PCI link, and ACPI PCI hot-plug drivers to
       use acpi_walk_resources() instead of acpi_get_current_resources().

So far this is just "cleanup" in the sense that there's no new
functionality.  As soon as any issues with these patches are
worked out, I have additional patches that build on them to
add ia64 support for multiple I/O port spaces.

Bjorn

Re: [ACPI] [PATCH] 1/3 ACPI resource handling

[Bjorn Helgaas]

> 2) I'm not convinced that this buys a whole lot -- it just hides the code
> behind a macro (something that's not generally liked in the Linux world.)
> Would this procedure be called from more than one place?

I assume you're wondering about the value of this piece:

> -----Original Message-----
> From: Bjorn Helgaas [mailto:bjorn_helgaas@hp.com] 
> Sent: Tuesday, February 11, 2003 1:58 PM
> ...
>     2) Addition of acpi_resource_to_address64(), which copies
>        address16, address32, or address64 resources to an address64
>        structure, so you don't have to maintain three sets of code
>        that differ only in the size of the address they deal with.

If so, decode_acpi_resource() in drivers/hotplug/acpiphp_glue.c is a
good example of why acpi_resource_to_address64() is useful.
decode_acpi_resource() contains a switch to handle 16-, 32-,
and 64-bit addresses.

So does acpi_get_crs_addr() in arch/ia64/kernel/acpi.c.  So will
my code to support multiple IO port space on IA64 (unless we
have something like acpi_resource_to_address64()).

None of these places really cares whether the address is 16, 32,
or 64 bits; they just have to have code to use the correct structure
type.  I'm proposing that we hide that ugliness in one place, and
most places can just deal with address64 and be done with it.

Bjorn

Re: [ACPI] [PATCH] 1/3 ACPI resource handling

[Bjorn Helgaas]

> 2) I'm not convinced that this buys a whole lot -- it just hides the code
> behind a macro (something that's not generally liked in the Linux world.)
> Would this procedure be called from more than one place?

Or, since you mention a macro, maybe your question is not about
the usefulness of acpi_resource_to_address64() itself, but about
how I implemented it, namely, with the copy_field and copy_address
macros:

+#define copy_field(out, in, field)     out->field = in->field
+#define copy_address(out, in)                                  \
+       copy_field(out, in, resource_type);                     \
+       copy_field(out, in, producer_consumer);                 \
...

If you'd rather see it implemented without the macros, I have no
objection to that.  I just used the macros to reduce the chance of
making typographical errors along the way.  Attached is the
equivalent patch (untested) without the macros.  I also added a
note to the struct acpi_resource_addressXX definitions about the
need to make corresponding changes to acpi_resource_to_address64()
if the structures change.

Bjorn


diff -u -ur linux-2.5.59/drivers/acpi/resources/rsxface.c acpi-2/drivers/acpi/resources/rsxface.c
--- linux-2.5.59/drivers/acpi/resources/rsxface.c	2003-01-16 19:22:23.000000000 -0700
+++ acpi-2/drivers/acpi/resources/rsxface.c	2003-02-21 14:45:00.000000000 -0700
@@ -233,3 +233,83 @@
 	status = acpi_rs_set_srs_method_data (device_handle, in_buffer);
 	return_ACPI_STATUS (status);
 }
+
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_resource_to_address64
+ *
+ * PARAMETERS:  resource                - Pointer to a resource
+ *              out                     - Pointer to the users's return 
+ *                                        buffer (a struct
+ *                                        acpi_resource_address64)
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: If the resource is an address16, address32, or address64,
+ *              copy it to the address64 return buffer.  This saves the
+ *              caller from having to duplicate code for different-sized
+ *              addresses.
+ *
+ ******************************************************************************/
+
+acpi_status
+acpi_resource_to_address64 (
+	struct acpi_resource            *resource,
+	struct acpi_resource_address64  *out)
+{
+	struct acpi_resource_address16  *address16;
+	struct acpi_resource_address32  *address32;
+	struct acpi_resource_address64  *address64;
+
+	switch (resource->id) {
+		case ACPI_RSTYPE_ADDRESS16:
+			address16 = (struct acpi_resource_address16 *) &resource->data;
+			out->resource_type              = address16->resource_type;
+			out->producer_consumer          = address16->producer_consumer;
+			out->decode                     = address16->decode;
+			out->min_address_fixed          = address16->min_address_fixed;
+			out->max_address_fixed          = address16->max_address_fixed;
+			out->attribute                  = address16->attribute;
+			out->granularity                = address16->granularity;
+			out->min_address_range          = address16->min_address_range;
+			out->max_address_range          = address16->max_address_range;
+			out->address_translation_offset = address16->address_translation_offset;
+			out->address_length             = address16->address_length;
+			out->resource_source            = address16->resource_source;
+			break;
+		case ACPI_RSTYPE_ADDRESS32:
+			address32 = (struct acpi_resource_address32 *) &resource->data;
+			out->resource_type              = address32->resource_type;
+			out->producer_consumer          = address32->producer_consumer;
+			out->decode                     = address32->decode;
+			out->min_address_fixed          = address32->min_address_fixed;
+			out->max_address_fixed          = address32->max_address_fixed;
+			out->attribute                  = address32->attribute;
+			out->granularity                = address32->granularity;
+			out->min_address_range          = address32->min_address_range;
+			out->max_address_range          = address32->max_address_range;
+			out->address_translation_offset = address32->address_translation_offset;
+			out->address_length             = address32->address_length;
+			out->resource_source            = address32->resource_source;
+			break;
+		case ACPI_RSTYPE_ADDRESS64:
+			address64 = (struct acpi_resource_address64 *) &resource->data;
+			out->resource_type              = address64->resource_type;
+			out->producer_consumer          = address64->producer_consumer;
+			out->decode                     = address64->decode;
+			out->min_address_fixed          = address64->min_address_fixed;
+			out->max_address_fixed          = address64->max_address_fixed;
+			out->attribute                  = address64->attribute;
+			out->granularity                = address64->granularity;
+			out->min_address_range          = address64->min_address_range;
+			out->max_address_range          = address64->max_address_range;
+			out->address_translation_offset = address64->address_translation_offset;
+			out->address_length             = address64->address_length;
+			out->resource_source            = address64->resource_source;
+			break;
+		default:
+			return (AE_BAD_PARAMETER);
+	}
+	return (AE_OK);
+}
diff -u -ur linux-2.5.59/include/acpi/acpixf.h acpi-2/include/acpi/acpixf.h
--- linux-2.5.59/include/acpi/acpixf.h	2003-02-21 14:39:23.000000000 -0700
+++ acpi-2/include/acpi/acpixf.h	2003-02-21 14:40:16.000000000 -0700
@@ -340,6 +340,11 @@
 	acpi_handle                     bus_device_handle,
 	struct acpi_buffer              *ret_buffer);
 
+acpi_status
+acpi_resource_to_address64 (
+	struct acpi_resource            *resource,
+	struct acpi_resource_address64  *out);
+
 
 /*
  * Hardware (ACPI device) interfaces
diff -u -ur linux-2.5.59/include/acpi/actypes.h acpi-2/include/acpi/actypes.h
--- linux-2.5.59/include/acpi/actypes.h	2003-02-21 14:39:23.000000000 -0700
+++ acpi-2/include/acpi/actypes.h	2003-02-21 14:48:35.000000000 -0700
@@ -1049,6 +1049,11 @@
 	char                                *string_ptr;
 };
 
+/*
+ * Changes to the following address structures should
+ * also be reflected in acpi_resource_to_address64().
+ */
+
 struct acpi_resource_address16
 {
 	u32                                 resource_type;

Re: [ACPI] [PATCH] 1/3 ACPI resource handling

[Matthew Wilcox]

On Fri, Feb 21, 2003 at 03:09:15PM -0700, Bjorn Helgaas wrote:
> Or, since you mention a macro, maybe your question is not about
> the usefulness of acpi_resource_to_address64() itself, but about
> how I implemented it, namely, with the copy_field and copy_address
> macros:

Can I suggest that you do a simple memcpy() for the case where you're
translating an address64 into an address64?

-- 
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk

Re: [ACPI] [PATCH] 1/3 ACPI resource handling

[Bjorn Helgaas]

On Friday 21 February 2003 3:15 pm, Matthew Wilcox wrote:
> On Fri, Feb 21, 2003 at 03:09:15PM -0700, Bjorn Helgaas wrote:
> > Or, since you mention a macro, maybe your question is not about
> > the usefulness of acpi_resource_to_address64() itself, but about
> > how I implemented it, namely, with the copy_field and copy_address
> > macros:
> 
> Can I suggest that you do a simple memcpy() for the case where you're
> translating an address64 into an address64?

I suppose we could.  Or maybe we should just get to the root of the
thing and make a generic acpi_resource_address structure and never
even expose the 16, 32, and 64 bit variants.  As far as I can tell,
they just make life difficult for consumers.

Then acpi_rs_address16_resource(), acpi_rs_address32_resource(),
and acpi_rs_address64_resource() could be collapsed into one function
with pretty trivial checks for the different sizes.  And there's lots
of similar collapsing that could be done.  I bet we'd even find one
or two defects in the process of removing all the duplicated code.

Of course, I guess that would change the CA interface, so that
constrains things a bit.

Bjorn

Re: [ACPI] [PATCH] 1/3 ACPI resource handling

[Pavel Machek]

Hi!

> 1) This seems like a good idea to simplify the parsing of the resource lists
> 
> 2) I'm not convinced that this buys a whole lot -- it just hides the code
> behind a macro (something that's not generally liked in the Linux world.)
> Would this procedure be called from more than one place?

Well, reducing code duplication *is* liked in Linux world. Use inline
function instead of macro if possible, through.
									Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [ACPI] [PATCH] 1/3 ACPI resource handling

[Folkert van Heusden]

> > 1) This seems like a good idea to simplify the parsing of the resource lists
> > 2) I'm not convinced that this buys a whole lot -- it just hides the code
> > behind a macro (something that's not generally liked in the Linux world.)
> > Would this procedure be called from more than one place?
> Well, reducing code duplication *is* liked in Linux world. Use inline
> function instead of macro if possible, through.

Isn't it better to use functions instead of macro's? Reduces the code
size--> less dirty cache-lines.

I saw, by the way, several functions duplicated in the networking-code.
For example a lot of them have a net_random-alike function. Imho they
should use the net_random in utils.c. Sadly my patches were ignored by the
maintainers.


Folkert

RE: [ACPI] [PATCH] 1/3 ACPI resource handling

[Moore, Robert]

This is worth looking into, anyway.
memcpy is appropriate, also.
Bob


-----Original Message-----
From: Bjorn Helgaas [mailto:bjorn_helgaas@hp.com] 
Sent: Friday, February 21, 2003 2:37 PM
To: Matthew Wilcox
Cc: Moore, Robert; Grover, Andrew; Walz, Michael; t-kochi@bq.jp.nec.com;
linux-kernel@vger.kernel.org; acpi-devel@lists.sourceforge.net
Subject: Re: [ACPI] [PATCH] 1/3 ACPI resource handling

On Friday 21 February 2003 3:15 pm, Matthew Wilcox wrote:
> On Fri, Feb 21, 2003 at 03:09:15PM -0700, Bjorn Helgaas wrote:
> > Or, since you mention a macro, maybe your question is not about
> > the usefulness of acpi_resource_to_address64() itself, but about
> > how I implemented it, namely, with the copy_field and copy_address
> > macros:
> 
> Can I suggest that you do a simple memcpy() for the case where you're
> translating an address64 into an address64?

I suppose we could.  Or maybe we should just get to the root of the
thing and make a generic acpi_resource_address structure and never
even expose the 16, 32, and 64 bit variants.  As far as I can tell,
they just make life difficult for consumers.

Then acpi_rs_address16_resource(), acpi_rs_address32_resource(),
and acpi_rs_address64_resource() could be collapsed into one function
with pretty trivial checks for the different sizes.  And there's lots
of similar collapsing that could be done.  I bet we'd even find one
or two defects in the process of removing all the duplicated code.

Of course, I guess that would change the CA interface, so that
constrains things a bit.

Bjorn

RE: [ACPI] [PATCH] 1/3 ACPI resource handling

[Moore, Robert]

Yes, as long as the code is used more than once (which it appears to be),
then of course it should be procedurized.
Bob


-----Original Message-----
From: Pavel Machek [mailto:pavel@ucw.cz] 
Sent: Sunday, February 23, 2003 2:55 PM
To: Moore, Robert
Cc: 'Bjorn Helgaas'; Grover, Andrew; Walz, Michael; t-kochi@bq.jp.nec.com;
linux-kernel@vger.kernel.org; acpi-devel@lists.sourceforge.net
Subject: Re: [ACPI] [PATCH] 1/3 ACPI resource handling

Hi!

> 1) This seems like a good idea to simplify the parsing of the resource
lists
> 
> 2) I'm not convinced that this buys a whole lot -- it just hides the code
> behind a macro (something that's not generally liked in the Linux world.)
> Would this procedure be called from more than one place?

Well, reducing code duplication *is* liked in Linux world. Use inline
function instead of macro if possible, through.
	
Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [ACPI] [PATCH] 1/3 ACPI resource handling

[Bjorn Helgaas]

Good grief.  If people would actually read and think about the
change instead of having a knee-jerk reaction to seeing
"#define", they would see that inline functions will not work
in this particular application.  The macros were used to
factor code that was identical except that it operated on
different structure types.  Inline functions obviously require
explicit argument declarations, while macros don't, so macros
are the only choice for this type of factoring.

In any case, this horse has been long dead.  I've posted versions of
acpi_resource_to_address64() both with and without the macros, and
haven't heard any substantial objections, so I'm assuming the ACPI
folks will either ignore them or pick the one they like best and
we can move on.

Bjorn

On Monday 24 February 2003 3:48 pm, Moore, Robert wrote:
> Yes, as long as the code is used more than once (which it appears to be),
> then of course it should be procedurized.
> Bob
> 
> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz] 
> Sent: Sunday, February 23, 2003 2:55 PM
> To: Moore, Robert
> Cc: 'Bjorn Helgaas'; Grover, Andrew; Walz, Michael; t-kochi@bq.jp.nec.com;
> linux-kernel@vger.kernel.org; acpi-devel@lists.sourceforge.net
> Subject: Re: [ACPI] [PATCH] 1/3 ACPI resource handling
> 
> Hi!
> 
> > 1) This seems like a good idea to simplify the parsing of the resource
> lists
> > 
> > 2) I'm not convinced that this buys a whole lot -- it just hides the code
> > behind a macro (something that's not generally liked in the Linux world.)
> > Would this procedure be called from more than one place?
> 
> Well, reducing code duplication *is* liked in Linux world. Use inline
> function instead of macro if possible, through.
> 	
> Pavel