acpi: unbreak ACPI_HANDLE(), encapsulate fwnode_operations

PATCH In review [email protected] 4 participants 6 messages Sep 16, 2017 – Sep 20, 2017

Subsystems (2)

ACPI ACPI component architecture (acpica)

lore

Share & tools

lore

[email protected]

author

2017-09-16 00:35 UTC #0 lore raw

cover acpi: unbreak ACPI_HANDLE(), encapsulate fwnode_operations

From: John Hubbard <[email protected]>

Hi everyone,

I really don't know for sure which fix is going to be preferred--the following patch, or just an obvious one-line fix that changes DECLARE_ACPI_FWNODE_OPS() so that it invokes EXPORT_SYMBOL, instead of EXPORT_SYMBOL_GPL. I explained the reasoning in PATCH 1/1, anyway, so please see what you think.

John Hubbard (1):
acpi: unbreak ACPI_HANDLE(), encapsulate fwnode_operations

 drivers/acpi/property.c | 13 +++++++++++++
 include/acpi/acpi_bus.h | 18 ++++--------------
 2 files changed, 17 insertions(+), 14 deletions(-)

Signature · 1 line

2.14.1

[email protected]

author

2017-09-16 00:35 UTC #1 lore raw

PATCH 1/1 acpi: unbreak ACPI_HANDLE(), encapsulate fwnode_operations

From: John Hubbard <[email protected]>

Due to commit db3e50f3234b ("device property: Get rid of struct fwnode_handle type field"), ACPI_HANDLE() inadvertently became a GPL-only call. The call path that led to that was:

ACPI_HANDLE()

    ACPI_COMPANION()
        to_acpi_device_node()
            is_acpi_device_node()
                acpi_device_fwnode_ops
                    DECLARE_ACPI_FWNODE_OPS(acpi_device_fwnode_ops);

...and the new DECLARE_ACPI_FWNODE_OPS() includes EXPORT_SYMBOL_GPL, whereas previously it was a static struct.

In order to avoid changing any of that, let's instead provide ever so slightly better encapsulation of those struct fwnode_operations instances. Those do not really need to be directly used in inline function calls in header files. Simply moving two small functions (is_acpi_device_node and is_acpi_data_node) out of acpi_bus.h, and into a .c file, does that.

That leaves the internals of struct fwnode_operations as GPL-only (which I think was the intent all along), but un-breaks any driver code out there that relies on the ACPI subsystem's being (historically) an EXPORT_SYMBOL-usable system. By that, I mean, ACPI_HANDLE() and other basic ACPI calls were non-GPL-protected.

Also, while I'm there, remove a tiny bit of redundancy that was missed in the earlier commit, by having is_acpi_node() use the other two routines, instead of checking fwnode directly.

Signed-off-by John Hubbard <[email protected]>

---
 drivers/acpi/property.c | 13 +++++++++++++
 include/acpi/acpi_bus.h | 18 ++++--------------
 2 files changed, 17 insertions(+), 14 deletions(-)

drivers/acpi/property.c +13 -0

index c1c216163de3..1e3c2517a1ac 100644
@@ -1293,3 +1293,16 @@ static int acpi_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 DECLARE_ACPI_FWNODE_OPS(acpi_device_fwnode_ops);
 DECLARE_ACPI_FWNODE_OPS(acpi_data_fwnode_ops);
 const struct fwnode_operations acpi_static_fwnode_ops;
+
+bool is_acpi_device_node(const struct fwnode_handle *fwnode)
+{
+	return !IS_ERR_OR_NULL(fwnode) &&
+		fwnode->ops == &acpi_device_fwnode_ops;
+}
+EXPORT_SYMBOL(is_acpi_device_node);
+
+bool is_acpi_data_node(const struct fwnode_handle *fwnode)
+{
+	return !IS_ERR_OR_NULL(fwnode) && fwnode->ops == &acpi_data_fwnode_ops;
+}
+EXPORT_SYMBOL(is_acpi_data_node);

include/acpi/acpi_bus.h +4 ~~-14~~

index dedf9d789166..fa1505292f6c 100644
@@ -399,17 +399,12 @@ extern const struct fwnode_operations acpi_device_fwnode_ops;
 extern const struct fwnode_operations acpi_data_fwnode_ops;
 extern const struct fwnode_operations acpi_static_fwnode_ops;
 
+bool is_acpi_device_node(const struct fwnode_handle *fwnode);
+bool is_acpi_data_node(const struct fwnode_handle *fwnode);
+
 static inline bool is_acpi_node(const struct fwnode_handle *fwnode)
 {
-	return !IS_ERR_OR_NULL(fwnode) &&
-		(fwnode->ops == &acpi_device_fwnode_ops
-		 || fwnode->ops == &acpi_data_fwnode_ops);
-}
-
-static inline bool is_acpi_device_node(const struct fwnode_handle *fwnode)
-{
-	return !IS_ERR_OR_NULL(fwnode) &&
-		fwnode->ops == &acpi_device_fwnode_ops;
+	return (is_acpi_device_node(fwnode) || is_acpi_data_node(fwnode));
 }
 
 #define to_acpi_device_node(__fwnode)					\

@@ -422,11 +417,6 @@ static inline bool is_acpi_device_node(const struct fwnode_handle *fwnode)
 			NULL;						\
 	})
 
-static inline bool is_acpi_data_node(const struct fwnode_handle *fwnode)
-{
-	return !IS_ERR_OR_NULL(fwnode) && fwnode->ops == &acpi_data_fwnode_ops;
-}
-
 #define to_acpi_data_node(__fwnode)					\
 	({								\
 		typeof(__fwnode) __to_acpi_data_node_fwnode = __fwnode;	\

Signature · 1 line

2.14.1

Sakari Ailus

Acked-by on v1 1/1

2017-09-18 10:50 UTC #2 lore raw

Hi John,

Quoted 23 lines · On Fri, Sep 15, 2017 at 05:35:27PM -0700, [email protected] wrote:

From: John Hubbard <[email protected]>
Due to commit db3e50f3234b ("device property: Get rid of struct fwnode_handle type field"), ACPI_HANDLE() inadvertently became a GPL-only call. The call path that led to that was:
ACPI_HANDLE()
    ACPI_COMPANION()
        to_acpi_device_node()
            is_acpi_device_node()
                acpi_device_fwnode_ops
                    DECLARE_ACPI_FWNODE_OPS(acpi_device_fwnode_ops);
...and the new DECLARE_ACPI_FWNODE_OPS() includes EXPORT_SYMBOL_GPL, whereas previously it was a static struct.
In order to avoid changing any of that, let's instead provide ever so slightly better encapsulation of those struct fwnode_operations instances. Those do not really need to be directly used in inline function calls in header files. Simply moving two small functions (is_acpi_device_node and is_acpi_data_node) out of acpi_bus.h, and into a .c file, does that.

Quoted 5 lines

That leaves the internals of struct fwnode_operations as GPL-only (which I think was the intent all along), but un-breaks any driver code out there that relies on the ACPI subsystem's being (historically) an EXPORT_SYMBOL-usable system. By that, I mean, ACPI_HANDLE() and other basic ACPI calls were non-GPL-protected.

Works for me.

Acked-by Sakari Ailus <[email protected]>

OF appears to have the same effective change; Rob: let me know if you'd like me to send a patch for that, or whether this is preferred. On OF both appear to be used (EXPORT_SYMBOL / EXPORT_SYMBOL_GPL).

Signature · 4 lines

Regards,

Sakari Ailus
[email protected]

Mika Westerberg

Acked-by on v1 1/1

2017-09-19 07:52 UTC #3 lore raw

In reply to [email protected]

Quoted 29 lines · On Fri, Sep 15, 2017 at 05:35:27PM -0700, [email protected] wrote:

From: John Hubbard <[email protected]>
Due to commit db3e50f3234b ("device property: Get rid of struct fwnode_handle type field"), ACPI_HANDLE() inadvertently became a GPL-only call. The call path that led to that was:
ACPI_HANDLE()
    ACPI_COMPANION()
        to_acpi_device_node()
            is_acpi_device_node()
                acpi_device_fwnode_ops
                    DECLARE_ACPI_FWNODE_OPS(acpi_device_fwnode_ops);
...and the new DECLARE_ACPI_FWNODE_OPS() includes EXPORT_SYMBOL_GPL, whereas previously it was a static struct.
In order to avoid changing any of that, let's instead provide ever so slightly better encapsulation of those struct fwnode_operations instances. Those do not really need to be directly used in inline function calls in header files. Simply moving two small functions (is_acpi_device_node and is_acpi_data_node) out of acpi_bus.h, and into a .c file, does that.
That leaves the internals of struct fwnode_operations as GPL-only (which I think was the intent all along), but un-breaks any driver code out there that relies on the ACPI subsystem's being (historically) an EXPORT_SYMBOL-usable system. By that, I mean, ACPI_HANDLE() and other basic ACPI calls were non-GPL-protected.

Quoted 5 lines

Also, while I'm there, remove a tiny bit of redundancy that was missed in the earlier commit, by having is_acpi_node() use the other two routines, instead of checking fwnode directly.
Signed-off-by John Hubbard <[email protected]>

Acked-by Mika Westerberg <[email protected]>

Rafael J. Wysocki

2017-09-19 20:48 UTC #4 lore raw

Quoted 30 lines · CEST Mika Westerberg

Quoted 29 lines · On Fri, Sep 15, 2017 at 05:35:27PM -0700, [email protected] wrote:
From: John Hubbard <[email protected]>
Due to commit db3e50f3234b ("device property: Get rid of struct fwnode_handle type field"), ACPI_HANDLE() inadvertently became a GPL-only call. The call path that led to that was:
ACPI_HANDLE()
    ACPI_COMPANION()
        to_acpi_device_node()
            is_acpi_device_node()
                acpi_device_fwnode_ops
                    DECLARE_ACPI_FWNODE_OPS(acpi_device_fwnode_ops);
...and the new DECLARE_ACPI_FWNODE_OPS() includes EXPORT_SYMBOL_GPL, whereas previously it was a static struct.
In order to avoid changing any of that, let's instead provide ever so slightly better encapsulation of those struct fwnode_operations instances. Those do not really need to be directly used in inline function calls in header files. Simply moving two small functions (is_acpi_device_node and is_acpi_data_node) out of acpi_bus.h, and into a .c file, does that.
That leaves the internals of struct fwnode_operations as GPL-only (which I think was the intent all along), but un-breaks any driver code out there that relies on the ACPI subsystem's being (historically) an EXPORT_SYMBOL-usable system. By that, I mean, ACPI_HANDLE() and other basic ACPI calls were non-GPL-protected.

Quoted 8 lines

Quoted 5 lines
Also, while I'm there, remove a tiny bit of redundancy that was missed in the earlier commit, by having is_acpi_node() use the other two routines, instead of checking fwnode directly.
Signed-off-by John Hubbard <[email protected]>
Acked-by Mika Westerberg <[email protected]>

OK, applied.

Side note: I'm slightly unhappy with the number of checks in the ACPI_COMPANION() path.

Do we really ever pass anything other than struct acpi_device to ACPI_COMPANION_SET() as the second arg?

Thanks,
Rafael

Mika Westerberg

2017-09-20 08:34 UTC #5 lore raw

Quoted 2 lines · Rafael J. Wysocki

Do we really ever pass anything other than struct acpi_device to ACPI_COMPANION_SET() as the second arg?

No, and we should not accept anything else than acpi_device either.