| Message ID | a0c795358306954294e7fe31ec974f3c51b56f4c.1499983092.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote: > We're about to amend device scan with multiple checks whether we're > running on a Mac. Speed that up by performing the DMI match only once > and caching the result. > > Switch over existing Apple DMI checks, thereby fixing two deficiencies: > > * They only match "Apple Inc." but not "Apple Computer, Inc.", which is > used by BIOSes released between January 2006 (when the first x86 Macs > started shipping) and January 2007 (when the company name changed upon > introduction of the iPhone). > > * They are now #defined to false on non-x86 arches and can thus be > optimized away by the compiler. > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > Changes v2 -> v3: > - Newly inserted patch in v3 to avoid repeated DMI checks for Apple > hardware: The result of the first DMI check in osi.c is cached. > Two other existing DMI checks are converted to use the result. > Because one of them is in a module (sbs.ko), the bool is_apple_system > needs to be exported. On non-x86, the DMI checks and Apple-specific > code are omitted altogether. (Andy, Rafael) > > drivers/acpi/osi.c | 4 ++++ > drivers/acpi/pci_root.c | 3 +-- > drivers/acpi/sbs.c | 24 +----------------------- > include/linux/acpi.h | 6 ++++++ > 4 files changed, 12 insertions(+), 25 deletions(-) > > diff --git a/drivers/acpi/osi.c b/drivers/acpi/osi.c > index cd953ae10238..6c253d4006b4 100644 > --- a/drivers/acpi/osi.c > +++ b/drivers/acpi/osi.c > @@ -258,12 +258,16 @@ bool acpi_osi_is_win8(void) > EXPORT_SYMBOL(acpi_osi_is_win8); > > #ifdef CONFIG_X86 > +bool is_apple_system; > +EXPORT_SYMBOL(is_apple_system); Maybe prepend the name of this variable with acpi_ to indicate that this is ACPI-specific. Other than that, I'm basically fine with the changes in this series, but I need to have a closer look at them. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jul 15, 2017 at 12:03:31AM +0200, Rafael J. Wysocki wrote: > On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote: > > --- a/drivers/acpi/osi.c > > +++ b/drivers/acpi/osi.c > > @@ -258,12 +258,16 @@ bool acpi_osi_is_win8(void) > > EXPORT_SYMBOL(acpi_osi_is_win8); > > > > #ifdef CONFIG_X86 > > +bool is_apple_system; > > +EXPORT_SYMBOL(is_apple_system); > > Maybe prepend the name of this variable with acpi_ to indicate that this is > ACPI-specific. It's not really ACPI-specific, osi.c just happens to be the best place to set the variable because the acpi_osi_dmi_table[] is checked very early during boot. So early in fact, that I could even replace the existing Apple DMI check in arch/x86/kernel/early-quirks.c with "is_apple_system". These non-ACPI files currently contain an Apple DMI check: arch/x86/kernel/early-quirks.c drivers/pci/quirks.c (2x) drivers/firmware/efi/apple-properties.c drivers/thunderbolt/tb.c drivers/thunderbolt/icm.c The latter three do not #include <linux/acpi.h> yet. Somehow it feels odd to include that header to check for presence of an Apple system because that's not really ACPI-related. I guess I could introduce a new <linux/apple.h> but I hate the insane proliferation of additional files in include/linux/. I could merge the contents of apple_bl.h and apple-gmux.h into that new header to reduce the number of files a bit. Struggling to find a solution that's nice and clean. Any ideas? Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 20, 2017 at 4:03 PM, Lukas Wunner <lukas@wunner.de> wrote: > On Sat, Jul 15, 2017 at 12:03:31AM +0200, Rafael J. Wysocki wrote: >> On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote: >> > --- a/drivers/acpi/osi.c >> > +++ b/drivers/acpi/osi.c >> > @@ -258,12 +258,16 @@ bool acpi_osi_is_win8(void) >> > EXPORT_SYMBOL(acpi_osi_is_win8); >> > >> > #ifdef CONFIG_X86 >> > +bool is_apple_system; >> > +EXPORT_SYMBOL(is_apple_system); >> >> Maybe prepend the name of this variable with acpi_ to indicate that this is >> ACPI-specific. > > It's not really ACPI-specific, osi.c just happens to be the best place > to set the variable because the acpi_osi_dmi_table[] is checked very > early during boot. So early in fact, that I could even replace the > existing Apple DMI check in arch/x86/kernel/early-quirks.c with > "is_apple_system". OK > These non-ACPI files currently contain an Apple DMI check: > arch/x86/kernel/early-quirks.c > drivers/pci/quirks.c (2x) > drivers/firmware/efi/apple-properties.c > drivers/thunderbolt/tb.c > drivers/thunderbolt/icm.c > > The latter three do not #include <linux/acpi.h> yet. Somehow it feels > odd to include that header to check for presence of an Apple system > because that's not really ACPI-related. > > I guess I could introduce a new <linux/apple.h> but I hate the insane > proliferation of additional files in include/linux/. There is include/linux/platform_data/x86/ so maybe put it in there? > I could merge the contents of apple_bl.h and apple-gmux.h into that new header to > reduce the number of files a bit. > > Struggling to find a solution that's nice and clean. Any ideas? I guess you still want it to work if someone configures the kernel without CONFIG_ACPI, although that's slightly debatable, so the variable should be defined somewhere in the arch code I suppose. I also guess you could add something like arch/x86/platform/apple/ and put the checks and the variable in there (in which case I'd call it x86_apple_machine or similar). Then invoke the check from acpi_osi_dmi_table[] code and the early-quirks.c one is only needed for !CONFIG_ACPI. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2017-07-20 at 16:03 +0200, Lukas Wunner wrote: > On Sat, Jul 15, 2017 at 12:03:31AM +0200, Rafael J. Wysocki wrote: > > On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote: > I guess I could introduce a new <linux/apple.h> but I hate the insane > proliferation of additional files in include/linux/. I could merge > the contents of apple_bl.h and apple-gmux.h into that new header to > reduce the number of files a bit. Just a side comment: include/platform_data/x86/apple.h in this case
On Thu, 2017-07-20 at 16:27 +0200, Rafael J. Wysocki wrote: > On Thu, Jul 20, 2017 at 4:03 PM, Lukas Wunner <lukas@wunner.de> wrote: > > On Sat, Jul 15, 2017 at 12:03:31AM +0200, Rafael J. Wysocki wrote: > > > On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote: > > > > > > I guess I could introduce a new <linux/apple.h> but I hate the > > insane > > proliferation of additional files in include/linux/. > > There is include/linux/platform_data/x86/ so maybe put it in there? Just suggested the same :-) > > I could merge the contents of apple_bl.h and apple-gmux.h into that > > new header to > > reduce the number of files a bit. > > > > Struggling to find a solution that's nice and clean. Any ideas? > > I guess you still want it to work if someone configures the kernel > without CONFIG_ACPI, although that's slightly debatable, so the > variable should be defined somewhere in the arch code I suppose. > > I also guess you could add something like arch/x86/platform/apple/ and > put the checks and the variable in there (in which case I'd call it > x86_apple_machine or similar). I'm not sure if we can use drivers/platform/x86 for this, either agreed way is fine to me. Darren? > > Then invoke the check from acpi_osi_dmi_table[] code and the > early-quirks.c one is only needed for !CONFIG_ACPI.
On Thursday, July 20, 2017 05:33:43 PM Andy Shevchenko wrote: > On Thu, 2017-07-20 at 16:27 +0200, Rafael J. Wysocki wrote: > > On Thu, Jul 20, 2017 at 4:03 PM, Lukas Wunner <lukas@wunner.de> wrote: > > > On Sat, Jul 15, 2017 at 12:03:31AM +0200, Rafael J. Wysocki wrote: > > > > On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote: > > > > > > > > > > I guess I could introduce a new <linux/apple.h> but I hate the > > > insane > > > proliferation of additional files in include/linux/. > > > > There is include/linux/platform_data/x86/ so maybe put it in there? > > Just suggested the same :-) > > > > I could merge the contents of apple_bl.h and apple-gmux.h into that > > > new header to > > > reduce the number of files a bit. > > > > > > Struggling to find a solution that's nice and clean. Any ideas? > > > > I guess you still want it to work if someone configures the kernel > > without CONFIG_ACPI, although that's slightly debatable, so the > > variable should be defined somewhere in the arch code I suppose. > > > > I also guess you could add something like arch/x86/platform/apple/ and > > put the checks and the variable in there (in which case I'd call it > > x86_apple_machine or similar). > > I'm not sure if we can use drivers/platform/x86 for this, either agreed > way is fine to me. No, because of the ACPI involvement. That needs to go under arch/ IMO. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 20, 2017 at 04:49:48PM +0200, Rafael Wysocki wrote: > On Thursday, July 20, 2017 05:33:43 PM Andy Shevchenko wrote: > > On Thu, 2017-07-20 at 16:27 +0200, Rafael J. Wysocki wrote: > > > On Thu, Jul 20, 2017 at 4:03 PM, Lukas Wunner <lukas@wunner.de> wrote: > > > > On Sat, Jul 15, 2017 at 12:03:31AM +0200, Rafael J. Wysocki wrote: > > > > > On Friday, July 14, 2017 12:36:19 AM Lukas Wunner wrote: > > > > > > > > > > > > > > I guess I could introduce a new <linux/apple.h> but I hate the > > > > insane > > > > proliferation of additional files in include/linux/. > > > > > > There is include/linux/platform_data/x86/ so maybe put it in there? > > > > Just suggested the same :-) Yes please, this is precisely why we created this directory. > > > > > > I could merge the contents of apple_bl.h and apple-gmux.h into that > > > > new header to > > > > reduce the number of files a bit. > > > > > > > > Struggling to find a solution that's nice and clean. Any ideas? > > > > > > I guess you still want it to work if someone configures the kernel > > > without CONFIG_ACPI, although that's slightly debatable, so the > > > variable should be defined somewhere in the arch code I suppose. > > > > > > I also guess you could add something like arch/x86/platform/apple/ and > > > put the checks and the variable in there (in which case I'd call it > > > x86_apple_machine or similar). > > > > I'm not sure if we can use drivers/platform/x86 for this, either agreed > > way is fine to me. > > No, because of the ACPI involvement. That needs to go under arch/ IMO. We have an example, silead_dmi.c, which extracts properties based on the DMI match, and adds them to the i2c client device. While I suppose we could do something like this, the drivers affected are really not "platform drivers", but rather common drivers with platform specific properties. I concur with Rafael.
diff --git a/drivers/acpi/osi.c b/drivers/acpi/osi.c index cd953ae10238..6c253d4006b4 100644 --- a/drivers/acpi/osi.c +++ b/drivers/acpi/osi.c @@ -258,12 +258,16 @@ bool acpi_osi_is_win8(void) EXPORT_SYMBOL(acpi_osi_is_win8); #ifdef CONFIG_X86 +bool is_apple_system; +EXPORT_SYMBOL(is_apple_system); + static void __init acpi_osi_dmi_darwin(bool enable, const struct dmi_system_id *d) { pr_notice("DMI detected to setup _OSI(\"Darwin\"): %s\n", d->ident); osi_config.darwin_dmi = 1; __acpi_osi_setup_darwin(enable); + is_apple_system = true; } static void __init acpi_osi_dmi_linux(bool enable, diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 9eec3095e6c3..1b6341717820 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -431,8 +431,7 @@ static void negotiate_os_control(struct acpi_pci_root *root, int *no_aspm) * been called successfully. We know the feature set supported by the * platform, so avoid calling _OSC at all */ - - if (dmi_match(DMI_SYS_VENDOR, "Apple Inc.")) { + if (is_apple_system) { root->osc_control_set = ~OSC_PCI_EXPRESS_PME_CONTROL; decode_osc_control(root, "OS assumes control of", root->osc_control_set); diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c index ad0b13ad4bbb..9b945ae0037e 100644 --- a/drivers/acpi/sbs.c +++ b/drivers/acpi/sbs.c @@ -31,7 +31,6 @@ #include <linux/jiffies.h> #include <linux/delay.h> #include <linux/power_supply.h> -#include <linux/dmi.h> #include "sbshc.h" #include "battery.h" @@ -58,8 +57,6 @@ static unsigned int cache_time = 1000; module_param(cache_time, uint, 0644); MODULE_PARM_DESC(cache_time, "cache time in milliseconds"); -static bool sbs_manager_broken; - #define MAX_SBS_BAT 4 #define ACPI_SBS_BLOCK_MAX 32 @@ -632,31 +629,12 @@ static void acpi_sbs_callback(void *context) } } -static int disable_sbs_manager(const struct dmi_system_id *d) -{ - sbs_manager_broken = true; - return 0; -} - -static struct dmi_system_id acpi_sbs_dmi_table[] = { - { - .callback = disable_sbs_manager, - .ident = "Apple", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc.") - }, - }, - { }, -}; - static int acpi_sbs_add(struct acpi_device *device) { struct acpi_sbs *sbs; int result = 0; int id; - dmi_check_system(acpi_sbs_dmi_table); - sbs = kzalloc(sizeof(struct acpi_sbs), GFP_KERNEL); if (!sbs) { result = -ENOMEM; @@ -677,7 +655,7 @@ static int acpi_sbs_add(struct acpi_device *device) result = 0; - if (!sbs_manager_broken) { + if (!is_apple_system) { result = acpi_manager_get_info(sbs); if (!result) { sbs->manager_present = 1; diff --git a/include/linux/acpi.h b/include/linux/acpi.h index cafdfb84ca28..d068cee890ee 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -389,6 +389,12 @@ extern int acpi_blacklisted(void); extern void acpi_osi_setup(char *str); extern bool acpi_osi_is_win8(void); +#ifdef CONFIG_X86 +extern bool is_apple_system; +#else +#define is_apple_system false +#endif + #ifdef CONFIG_ACPI_NUMA int acpi_map_pxm_to_online_node(int pxm); int acpi_get_node(acpi_handle handle);
We're about to amend device scan with multiple checks whether we're running on a Mac. Speed that up by performing the DMI match only once and caching the result. Switch over existing Apple DMI checks, thereby fixing two deficiencies: * They only match "Apple Inc." but not "Apple Computer, Inc.", which is used by BIOSes released between January 2006 (when the first x86 Macs started shipping) and January 2007 (when the company name changed upon introduction of the iPhone). * They are now #defined to false on non-x86 arches and can thus be optimized away by the compiler. Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Signed-off-by: Lukas Wunner <lukas@wunner.de> --- Changes v2 -> v3: - Newly inserted patch in v3 to avoid repeated DMI checks for Apple hardware: The result of the first DMI check in osi.c is cached. Two other existing DMI checks are converted to use the result. Because one of them is in a module (sbs.ko), the bool is_apple_system needs to be exported. On non-x86, the DMI checks and Apple-specific code are omitted altogether. (Andy, Rafael) drivers/acpi/osi.c | 4 ++++ drivers/acpi/pci_root.c | 3 +-- drivers/acpi/sbs.c | 24 +----------------------- include/linux/acpi.h | 6 ++++++ 4 files changed, 12 insertions(+), 25 deletions(-)