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:[email protected]]
Sent: Tuesday, February 11, 2003 1:58 PM
To: Grover, Andrew
Cc: [email protected]; [email protected];
[email protected]
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
> 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:[email protected]]
> 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
> 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;
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
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
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?]
> > 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