| Message ID | 20240528013626.14066-9-luke@ljones.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform-x86-asus-wmi: multiple fixes, improvements, features | expand |
On 5/27/2024 8:36 PM, Luke D. Jones wrote: > Exposes the APU memory setting available on a few ASUS models such as > the ROG Ally. > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > --- > .../ABI/testing/sysfs-platform-asus-wmi | 8 ++ > drivers/platform/x86/asus-wmi.c | 109 ++++++++++++++++++ > include/linux/platform_data/x86/asus-wmi.h | 3 + > 3 files changed, 120 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi > index ac881e72e374..d221a3bc1a81 100644 > --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi > +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi > @@ -245,3 +245,11 @@ Description: > Show the maximum performance and efficiency core countin format > 0x[E][P] where [E] is the efficiency core count, and [P] is > the perfromance core count. > + > +What: /sys/devices/platform/<platform>/apu_mem > +Date: Jun 2024 > +KernelVersion: 6.11 > +Contact: "Luke Jones" <luke@ljones.dev> > +Description: > + Set the maximum available system memory for the APU. > + * Min=0, Max=8 What is the unit? It seems like multiples of something? > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > index f62a36dfcd4b..4b5fbae8c563 100644 > --- a/drivers/platform/x86/asus-wmi.c > +++ b/drivers/platform/x86/asus-wmi.c > @@ -855,6 +855,112 @@ static DEVICE_ATTR_RW(cores_enabled); > WMI_SIMPLE_SHOW(cores_max, "0x%x\n", ASUS_WMI_DEVID_CORES_MAX); > static DEVICE_ATTR_RO(cores_max); > > +/* Device memory available to APU */ > + > +static ssize_t apu_mem_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct asus_wmi *asus = dev_get_drvdata(dev); > + int err; > + u32 mem; > + > + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_APU_MEM, &mem); > + if (err < 0) > + return err; > + > + switch (mem) { > + case 256: > + mem = 0; > + break; > + case 258: > + mem = 1; > + break; > + case 259: > + mem = 2; > + break; > + case 260: > + mem = 3; > + break; > + case 261: > + mem = 4; > + break; > + case 262: > + mem = 8; > + break; > + case 263: > + mem = 5; > + break; > + case 264: > + mem = 6; > + break; > + case 265: > + mem = 7; > + break; > + default: > + mem = 4; > + break; > + } > + > + return sysfs_emit(buf, "%d\n", mem); > +} > + > +static ssize_t apu_mem_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct asus_wmi *asus = dev_get_drvdata(dev); > + int result, err; > + u32 mem; > + > + result = kstrtou32(buf, 10, &mem); > + if (result) > + return result; > + > + switch (mem) { > + case 0: > + mem = 0; > + break; > + case 1: > + mem = 258; > + break; > + case 2: > + mem = 259; > + break; > + case 3: > + mem = 260; > + break; > + case 4: > + mem = 261; > + break; > + case 5: > + mem = 263; > + break; > + case 6: > + mem = 264; > + break; > + case 7: > + mem = 265; > + break; > + case 8: > + mem = 262; Is case 8 a mistake, or intentionally out of order? > + break; > + default: > + return -EIO; > + } > + > + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_APU_MEM, mem, &result); > + if (err) { > + pr_warn("Failed to set apu_mem: %d\n", err); > + return err; > + } > + > + pr_info("APU memory changed, reboot required\n"); If you're logging something into the logs for this, I'd say make it more useful. "APU memory changed to %d MB" > + sysfs_notify(&asus->platform_device->dev.kobj, NULL, "apu_mem"); So this is a case that the BIOS attributes API I mentioned before would be REALLY useful. There is a pending_reboot sysfs file that userspace can query to know if a given setting requires a reboot or not. Fwupd also uses this attribute to know to delay BIOS updates until the system has been rebooted. > + > + return count; > +} > +static DEVICE_ATTR_RW(apu_mem); > + > /* Tablet mode ****************************************************************/ > > static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus) > @@ -4100,6 +4206,7 @@ static struct attribute *platform_attributes[] = { > &dev_attr_panel_fhd.attr, > &dev_attr_cores_enabled.attr, > &dev_attr_cores_max.attr, > + &dev_attr_apu_mem.attr, > &dev_attr_mini_led_mode.attr, > &dev_attr_available_mini_led_mode.attr, > NULL > @@ -4176,6 +4283,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj, > else if (attr == &dev_attr_cores_enabled.attr > || attr == &dev_attr_cores_max.attr) > ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CORES_SET); > + else if (attr == &dev_attr_apu_mem.attr) > + ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_APU_MEM); > else if (attr == &dev_attr_mini_led_mode.attr) > ok = asus->mini_led_dev_id != 0; > else if (attr == &dev_attr_available_mini_led_mode.attr) > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h > index 5a56e7e97785..efe608861e55 100644 > --- a/include/linux/platform_data/x86/asus-wmi.h > +++ b/include/linux/platform_data/x86/asus-wmi.h > @@ -121,6 +121,9 @@ > /* Maximum Intel E-core and P-core availability */ > #define ASUS_WMI_DEVID_CORES_MAX 0x001200D3 > > +/* Set the memory available to the APU */ > +#define ASUS_WMI_DEVID_APU_MEM 0x000600C1 > + > /* MCU powersave mode */ > #define ASUS_WMI_DEVID_MCU_POWERSAVE 0x001200E2 >
On Tue, 28 May 2024, at 2:19 PM, Limonciello, Mario wrote: > > > On 5/27/2024 8:36 PM, Luke D. Jones wrote: > > Exposes the APU memory setting available on a few ASUS models such as > > the ROG Ally. > > > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > > --- > > .../ABI/testing/sysfs-platform-asus-wmi | 8 ++ > > drivers/platform/x86/asus-wmi.c | 109 ++++++++++++++++++ > > include/linux/platform_data/x86/asus-wmi.h | 3 + > > 3 files changed, 120 insertions(+) > > > > diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi > > index ac881e72e374..d221a3bc1a81 100644 > > --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi > > +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi > > @@ -245,3 +245,11 @@ Description: > > Show the maximum performance and efficiency core countin format > > 0x[E][P] where [E] is the efficiency core count, and [P] is > > the perfromance core count. > > + > > +What: /sys/devices/platform/<platform>/apu_mem > > +Date: Jun 2024 > > +KernelVersion: 6.11 > > +Contact: "Luke Jones" <luke@ljones.dev> > > +Description: > > + Set the maximum available system memory for the APU. > > + * Min=0, Max=8 > > What is the unit? It seems like multiples of something? It's GB, looks like I didn't save my work when I did a rebase and update of this patch. I'll add to my todo list for next version > > diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > > index f62a36dfcd4b..4b5fbae8c563 100644 > > --- a/drivers/platform/x86/asus-wmi.c > > +++ b/drivers/platform/x86/asus-wmi.c > > @@ -855,6 +855,112 @@ static DEVICE_ATTR_RW(cores_enabled); > > WMI_SIMPLE_SHOW(cores_max, "0x%x\n", ASUS_WMI_DEVID_CORES_MAX); > > static DEVICE_ATTR_RO(cores_max); > > > > +/* Device memory available to APU */ > > + > > +static ssize_t apu_mem_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct asus_wmi *asus = dev_get_drvdata(dev); > > + int err; > > + u32 mem; > > + > > + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_APU_MEM, &mem); > > + if (err < 0) > > + return err; > > + > > + switch (mem) { > > + case 256: > > + mem = 0; > > + break; > > + case 258: > > + mem = 1; > > + break; > > + case 259: > > + mem = 2; > > + break; > > + case 260: > > + mem = 3; > > + break; > > + case 261: > > + mem = 4; > > + break; > > + case 262: > > + mem = 8; > > + break; > > + case 263: > > + mem = 5; > > + break; > > + case 264: > > + mem = 6; > > + break; > > + case 265: > > + mem = 7; > > + break; > > + default: > > + mem = 4; > > + break; > > + } > > + > > + return sysfs_emit(buf, "%d\n", mem); > > +} > > + > > +static ssize_t apu_mem_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + struct asus_wmi *asus = dev_get_drvdata(dev); > > + int result, err; > > + u32 mem; > > + > > + result = kstrtou32(buf, 10, &mem); > > + if (result) > > + return result; > > + > > + switch (mem) { > > + case 0: > > + mem = 0; > > + break; > > + case 1: > > + mem = 258; > > + break; > > + case 2: > > + mem = 259; > > + break; > > + case 3: > > + mem = 260; > > + break; > > + case 4: > > + mem = 261; > > + break; > > + case 5: > > + mem = 263; > > + break; > > + case 6: > > + mem = 264; > > + break; > > + case 7: > > + mem = 265; > > + break; > > + case 8: > > + mem = 262; > > Is case 8 a mistake, or intentionally out of order? Do you mean the `mem = <val>`? Those aren't in order, and I thought it was easier to read if the switch was ordered. > > > + break; > > + default: > > + return -EIO; > > + } > > + > > + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_APU_MEM, mem, &result); > > + if (err) { > > + pr_warn("Failed to set apu_mem: %d\n", err); > > + return err; > > + } > > + > > + pr_info("APU memory changed, reboot required\n"); > > If you're logging something into the logs for this, I'd say make it more > useful. > > "APU memory changed to %d MB" Agreed. There's probably a few other spots I can do this also. > > > + sysfs_notify(&asus->platform_device->dev.kobj, NULL, "apu_mem"); > > So this is a case that the BIOS attributes API I mentioned before would > be REALLY useful. There is a pending_reboot sysfs file that userspace > can query to know if a given setting requires a reboot or not. > > Fwupd also uses this attribute to know to delay BIOS updates until the > system has been rebooted. Oh! Yes I'll queue that as an additional patch. There's at least 2 or 3 other spots where that would be good to have. > > + > > + return count; > > +} > > +static DEVICE_ATTR_RW(apu_mem); > > + > > /* Tablet mode ****************************************************************/ > > > > static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus) > > @@ -4100,6 +4206,7 @@ static struct attribute *platform_attributes[] = { > > &dev_attr_panel_fhd.attr, > > &dev_attr_cores_enabled.attr, > > &dev_attr_cores_max.attr, > > + &dev_attr_apu_mem.attr, > > &dev_attr_mini_led_mode.attr, > > &dev_attr_available_mini_led_mode.attr, > > NULL > > @@ -4176,6 +4283,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj, > > else if (attr == &dev_attr_cores_enabled.attr > > || attr == &dev_attr_cores_max.attr) > > ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CORES_SET); > > + else if (attr == &dev_attr_apu_mem.attr) > > + ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_APU_MEM); > > else if (attr == &dev_attr_mini_led_mode.attr) > > ok = asus->mini_led_dev_id != 0; > > else if (attr == &dev_attr_available_mini_led_mode.attr) > > diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h > > index 5a56e7e97785..efe608861e55 100644 > > --- a/include/linux/platform_data/x86/asus-wmi.h > > +++ b/include/linux/platform_data/x86/asus-wmi.h > > @@ -121,6 +121,9 @@ > > /* Maximum Intel E-core and P-core availability */ > > #define ASUS_WMI_DEVID_CORES_MAX 0x001200D3 > > > > +/* Set the memory available to the APU */ > > +#define ASUS_WMI_DEVID_APU_MEM 0x000600C1 > > + > > /* MCU powersave mode */ > > #define ASUS_WMI_DEVID_MCU_POWERSAVE 0x001200E2 > > >
>>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c >>> index f62a36dfcd4b..4b5fbae8c563 100644 >>> --- a/drivers/platform/x86/asus-wmi.c >>> +++ b/drivers/platform/x86/asus-wmi.c >>> @@ -855,6 +855,112 @@ static DEVICE_ATTR_RW(cores_enabled); >>> WMI_SIMPLE_SHOW(cores_max, "0x%x\n", ASUS_WMI_DEVID_CORES_MAX); >>> static DEVICE_ATTR_RO(cores_max); >>> >>> +/* Device memory available to APU */ >>> + >>> +static ssize_t apu_mem_show(struct device *dev, >>> + struct device_attribute *attr, char *buf) >>> +{ >>> + struct asus_wmi *asus = dev_get_drvdata(dev); >>> + int err; >>> + u32 mem; >>> + >>> + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_APU_MEM, &mem); >>> + if (err < 0) >>> + return err; >>> + >>> + switch (mem) { >>> + case 256: >>> + mem = 0; >>> + break; >>> + case 258: >>> + mem = 1; >>> + break; >>> + case 259: >>> + mem = 2; >>> + break; >>> + case 260: >>> + mem = 3; >>> + break; >>> + case 261: >>> + mem = 4; >>> + break; >>> + case 262: >>> + mem = 8; >>> + break; >>> + case 263: >>> + mem = 5; >>> + break; >>> + case 264: >>> + mem = 6; >>> + break; >>> + case 265: >>> + mem = 7; >>> + break; >>> + default: >>> + mem = 4; >>> + break; >>> + } >>> + >>> + return sysfs_emit(buf, "%d\n", mem); >>> +} >>> + >>> +static ssize_t apu_mem_store(struct device *dev, >>> + struct device_attribute *attr, >>> + const char *buf, size_t count) >>> +{ >>> + struct asus_wmi *asus = dev_get_drvdata(dev); >>> + int result, err; >>> + u32 mem; >>> + >>> + result = kstrtou32(buf, 10, &mem); >>> + if (result) >>> + return result; >>> + >>> + switch (mem) { >>> + case 0: >>> + mem = 0; >>> + break; >>> + case 1: >>> + mem = 258; >>> + break; >>> + case 2: >>> + mem = 259; >>> + break; >>> + case 3: >>> + mem = 260; >>> + break; >>> + case 4: >>> + mem = 261; >>> + break; >>> + case 5: >>> + mem = 263; >>> + break; >>> + case 6: >>> + mem = 264; >>> + break; >>> + case 7: >>> + mem = 265; >>> + break; >>> + case 8: >>> + mem = 262; >> >> Is case 8 a mistake, or intentionally out of order? > > Do you mean the `mem = <val>`? Those aren't in order, and I thought it was easier to read if the switch was ordered. > I'm wondering if case 5 should be 262, case 6 263, case 7 264 and case 8 265. It just stood out to me. If that's all intended then no concerns and I agree sorting the case is better. >> >>> + break; >>> + default: >>> + return -EIO; >>> + } >>> + >>> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_APU_MEM, mem, &result); >>> + if (err) { >>> + pr_warn("Failed to set apu_mem: %d\n", err); >>> + return err; >>> + } >>> + >>> + pr_info("APU memory changed, reboot required\n"); >> >> If you're logging something into the logs for this, I'd say make it more >> useful. >> >> "APU memory changed to %d MB" > > Agreed. There's probably a few other spots I can do this also. > >> >>> + sysfs_notify(&asus->platform_device->dev.kobj, NULL, "apu_mem"); >> >> So this is a case that the BIOS attributes API I mentioned before would >> be REALLY useful. There is a pending_reboot sysfs file that userspace >> can query to know if a given setting requires a reboot or not. >> >> Fwupd also uses this attribute to know to delay BIOS updates until the >> system has been rebooted. > > Oh! Yes I'll queue that as an additional patch. There's at least 2 or 3 other spots where that would be good to have. > For any "new" attributes it's better to put them in that API than code duplication of the BIOS attributes API as well as a random sysfs file API that you can never discard. >>> + >>> + return count; >>> +} >>> +static DEVICE_ATTR_RW(apu_mem); >>> + >>> /* Tablet mode ****************************************************************/ >>> >>> static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus) >>> @@ -4100,6 +4206,7 @@ static struct attribute *platform_attributes[] = { >>> &dev_attr_panel_fhd.attr, >>> &dev_attr_cores_enabled.attr, >>> &dev_attr_cores_max.attr, >>> + &dev_attr_apu_mem.attr, >>> &dev_attr_mini_led_mode.attr, >>> &dev_attr_available_mini_led_mode.attr, >>> NULL >>> @@ -4176,6 +4283,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj, >>> else if (attr == &dev_attr_cores_enabled.attr >>> || attr == &dev_attr_cores_max.attr) >>> ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CORES_SET); >>> + else if (attr == &dev_attr_apu_mem.attr) >>> + ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_APU_MEM); >>> else if (attr == &dev_attr_mini_led_mode.attr) >>> ok = asus->mini_led_dev_id != 0; >>> else if (attr == &dev_attr_available_mini_led_mode.attr) >>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h >>> index 5a56e7e97785..efe608861e55 100644 >>> --- a/include/linux/platform_data/x86/asus-wmi.h >>> +++ b/include/linux/platform_data/x86/asus-wmi.h >>> @@ -121,6 +121,9 @@ >>> /* Maximum Intel E-core and P-core availability */ >>> #define ASUS_WMI_DEVID_CORES_MAX 0x001200D3 >>> >>> +/* Set the memory available to the APU */ >>> +#define ASUS_WMI_DEVID_APU_MEM 0x000600C1 >>> + >>> /* MCU powersave mode */ >>> #define ASUS_WMI_DEVID_MCU_POWERSAVE 0x001200E2 >>> >>
On Wed, 29 May 2024, at 1:27 AM, Mario Limonciello wrote: > >>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c > >>> index f62a36dfcd4b..4b5fbae8c563 100644 > >>> --- a/drivers/platform/x86/asus-wmi.c > >>> +++ b/drivers/platform/x86/asus-wmi.c > >>> @@ -855,6 +855,112 @@ static DEVICE_ATTR_RW(cores_enabled); > >>> WMI_SIMPLE_SHOW(cores_max, "0x%x\n", ASUS_WMI_DEVID_CORES_MAX); > >>> static DEVICE_ATTR_RO(cores_max); > >>> > >>> +/* Device memory available to APU */ > >>> + > >>> +static ssize_t apu_mem_show(struct device *dev, > >>> + struct device_attribute *attr, char *buf) > >>> +{ > >>> + struct asus_wmi *asus = dev_get_drvdata(dev); > >>> + int err; > >>> + u32 mem; > >>> + > >>> + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_APU_MEM, &mem); > >>> + if (err < 0) > >>> + return err; > >>> + > >>> + switch (mem) { > >>> + case 256: > >>> + mem = 0; > >>> + break; > >>> + case 258: > >>> + mem = 1; > >>> + break; > >>> + case 259: > >>> + mem = 2; > >>> + break; > >>> + case 260: > >>> + mem = 3; > >>> + break; > >>> + case 261: > >>> + mem = 4; > >>> + break; > >>> + case 262: > >>> + mem = 8; > >>> + break; > >>> + case 263: > >>> + mem = 5; > >>> + break; > >>> + case 264: > >>> + mem = 6; > >>> + break; > >>> + case 265: > >>> + mem = 7; > >>> + break; > >>> + default: > >>> + mem = 4; > >>> + break; > >>> + } > >>> + > >>> + return sysfs_emit(buf, "%d\n", mem); > >>> +} > >>> + > >>> +static ssize_t apu_mem_store(struct device *dev, > >>> + struct device_attribute *attr, > >>> + const char *buf, size_t count) > >>> +{ > >>> + struct asus_wmi *asus = dev_get_drvdata(dev); > >>> + int result, err; > >>> + u32 mem; > >>> + > >>> + result = kstrtou32(buf, 10, &mem); > >>> + if (result) > >>> + return result; > >>> + > >>> + switch (mem) { > >>> + case 0: > >>> + mem = 0; > >>> + break; > >>> + case 1: > >>> + mem = 258; > >>> + break; > >>> + case 2: > >>> + mem = 259; > >>> + break; > >>> + case 3: > >>> + mem = 260; > >>> + break; > >>> + case 4: > >>> + mem = 261; > >>> + break; > >>> + case 5: > >>> + mem = 263; > >>> + break; > >>> + case 6: > >>> + mem = 264; > >>> + break; > >>> + case 7: > >>> + mem = 265; > >>> + break; > >>> + case 8: > >>> + mem = 262; > >> > >> Is case 8 a mistake, or intentionally out of order? > > > > Do you mean the `mem = <val>`? Those aren't in order, and I thought it was easier to read if the switch was ordered. > > > > I'm wondering if case 5 should be 262, case 6 263, case 7 264 and case 8 > 265. It just stood out to me. Yeah it's weird but that is what it is. Also verified in ghelper which calls the same WMI interfaces in Windows. > > If that's all intended then no concerns and I agree sorting the case is > better. > > >> > >>> + break; > >>> + default: > >>> + return -EIO; > >>> + } > >>> + > >>> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_APU_MEM, mem, &result); > >>> + if (err) { > >>> + pr_warn("Failed to set apu_mem: %d\n", err); > >>> + return err; > >>> + } > >>> + > >>> + pr_info("APU memory changed, reboot required\n"); > >> > >> If you're logging something into the logs for this, I'd say make it more > >> useful. > >> > >> "APU memory changed to %d MB" > > > > Agreed. There's probably a few other spots I can do this also. > > > >> > >>> + sysfs_notify(&asus->platform_device->dev.kobj, NULL, "apu_mem"); > >> > >> So this is a case that the BIOS attributes API I mentioned before would > >> be REALLY useful. There is a pending_reboot sysfs file that userspace > >> can query to know if a given setting requires a reboot or not. > >> > >> Fwupd also uses this attribute to know to delay BIOS updates until the > >> system has been rebooted. > > > > Oh! Yes I'll queue that as an additional patch. There's at least 2 or 3 other spots where that would be good to have. > > > > For any "new" attributes it's better to put them in that API than code > duplication of the BIOS attributes API as well as a random sysfs file > API that you can never discard. Do you mean the firmware_attributes API? If so, I'm not opposed to adding all the existing ROG attributes to it also. If I'm understanding the docs correctly, for example this apu_mem attr would then become: - /sys/class/firmware-attributes/asus-bios/attributes/apu_mem/type - /sys/class/firmware-attributes/*/attributes/apu_mem/current_value - /sys/class/firmware-attributes/*/attributes/apu_mem/default_value - /sys/class/firmware-attributes/*/attributes/apu_mem/display_name - /sys/class/firmware-attributes/*/attributes/apu_mem/possible_values - ..etc That's absolutely much better than what I've been doing and I wish I'd known about it sooner. So if I go ahead and convert all the new attr to this are there any issues with also converting much of the previous attr? And I'm aware of "don't break userspace" so really I'm a bit unsure how best to manage that (would a new module be better here also? "asus-bios.c" perhaps). What I don't want is a split between platform and firmware_attributes. > > >>> + > >>> + return count; > >>> +} > >>> +static DEVICE_ATTR_RW(apu_mem); > >>> + > >>> /* Tablet mode ****************************************************************/ > >>> > >>> static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus) > >>> @@ -4100,6 +4206,7 @@ static struct attribute *platform_attributes[] = { > >>> &dev_attr_panel_fhd.attr, > >>> &dev_attr_cores_enabled.attr, > >>> &dev_attr_cores_max.attr, > >>> + &dev_attr_apu_mem.attr, > >>> &dev_attr_mini_led_mode.attr, > >>> &dev_attr_available_mini_led_mode.attr, > >>> NULL > >>> @@ -4176,6 +4283,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj, > >>> else if (attr == &dev_attr_cores_enabled.attr > >>> || attr == &dev_attr_cores_max.attr) > >>> ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CORES_SET); > >>> + else if (attr == &dev_attr_apu_mem.attr) > >>> + ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_APU_MEM); > >>> else if (attr == &dev_attr_mini_led_mode.attr) > >>> ok = asus->mini_led_dev_id != 0; > >>> else if (attr == &dev_attr_available_mini_led_mode.attr) > >>> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h > >>> index 5a56e7e97785..efe608861e55 100644 > >>> --- a/include/linux/platform_data/x86/asus-wmi.h > >>> +++ b/include/linux/platform_data/x86/asus-wmi.h > >>> @@ -121,6 +121,9 @@ > >>> /* Maximum Intel E-core and P-core availability */ > >>> #define ASUS_WMI_DEVID_CORES_MAX 0x001200D3 > >>> > >>> +/* Set the memory available to the APU */ > >>> +#define ASUS_WMI_DEVID_APU_MEM 0x000600C1 > >>> + > >>> /* MCU powersave mode */ > >>> #define ASUS_WMI_DEVID_MCU_POWERSAVE 0x001200E2 > >>> > >> > >
>> For any "new" attributes it's better to put them in that API than code >> duplication of the BIOS attributes API as well as a random sysfs file >> API that you can never discard. > > Do you mean the firmware_attributes API? If so, I'm not opposed to adding all the existing ROG attributes to it also. > > If I'm understanding the docs correctly, for example this apu_mem attr would then become: > - /sys/class/firmware-attributes/asus-bios/attributes/apu_mem/type > - /sys/class/firmware-attributes/*/attributes/apu_mem/current_value > - /sys/class/firmware-attributes/*/attributes/apu_mem/default_value > - /sys/class/firmware-attributes/*/attributes/apu_mem/display_name > - /sys/class/firmware-attributes/*/attributes/apu_mem/possible_values > - ..etc > > That's absolutely much better than what I've been doing and I wish I'd known about it sooner. Yup! And even better "fwupdmgr get-bios-setting" and "fwupdmgr set-bios-setting" [1] will work with bash completion too. If you want to build a GUI around it, it could use the fwupd dbus interface for getting those settings and manipulating them. The dbus interface policy is protected by PK. > > So if I go ahead and convert all the new attr to this are there any issues with also converting much of the previous attr? And I'm aware of "don't break userspace" so really I'm a bit unsure how best to manage that (would a new module be better here also? "asus-bios.c" perhaps). > > What I don't want is a split between platform and firmware_attributes. > My personal thought is add everything to the new API and then hide the "old" interface behind some boolean Kconfig option that defaults to disabled. If someone enables it then when it's "used" (IE read from / written to) then do something like a pr_warn_once() that it's a deprecated interface and it will go away. Wait until next LTS kernel, and then rip it out after that. Need Hans and/or Ilpo agree to make with that though before you spend too much effort on it though. [1] https://fwupd.github.io/libfwupdplugin/bios-settings.html
On Wed, 29 May 2024, at 9:16 AM, Mario Limonciello wrote: > >> For any "new" attributes it's better to put them in that API than code > >> duplication of the BIOS attributes API as well as a random sysfs file > >> API that you can never discard. > > > > Do you mean the firmware_attributes API? If so, I'm not opposed to adding all the existing ROG attributes to it also. > > > > If I'm understanding the docs correctly, for example this apu_mem attr would then become: > > - /sys/class/firmware-attributes/asus-bios/attributes/apu_mem/type > > - /sys/class/firmware-attributes/*/attributes/apu_mem/current_value > > - /sys/class/firmware-attributes/*/attributes/apu_mem/default_value > > - /sys/class/firmware-attributes/*/attributes/apu_mem/display_name > > - /sys/class/firmware-attributes/*/attributes/apu_mem/possible_values > > - ..etc > > > > That's absolutely much better than what I've been doing and I wish I'd known about it sooner. > > Yup! And even better "fwupdmgr get-bios-setting" and "fwupdmgr > set-bios-setting" [1] will work with bash completion too. > > If you want to build a GUI around it, it could use the fwupd dbus > interface for getting those settings and manipulating them. My existing GUI effort revolves around me adding these attrs to both kernel and a dbus interface. So if I can drop that then all the better > The dbus > interface policy is protected by PK. What is PK? And I'm guessing this restricts users to requiring permissions to change settings? > > > > So if I go ahead and convert all the new attr to this are there any issues with also converting much of the previous attr? And I'm aware of "don't break userspace" so really I'm a bit unsure how best to manage that (would a new module be better here also? "asus-bios.c" perhaps). > > > > What I don't want is a split between platform and firmware_attributes. > > > > My personal thought is add everything to the new API and then hide the > "old" interface behind some boolean Kconfig option that defaults to > disabled. If someone enables it then when it's "used" (IE read from / > written to) then do something like a pr_warn_once() that it's a > deprecated interface and it will go away. Wait until next LTS kernel, > and then rip it out after that. I'm happy to do this. On a serious note though, I should make a new module for this right? It would make it much much easier to manage the "conversion" I think, and then it would act as a separation of concerns. > > Need Hans and/or Ilpo agree to make with that though before you spend > too much effort on it though. Ack > > [1] https://fwupd.github.io/libfwupdplugin/bios-settings.html >
On 5/28/2024 16:34, Luke Jones wrote: > > > On Wed, 29 May 2024, at 9:16 AM, Mario Limonciello wrote: >>>> For any "new" attributes it's better to put them in that API than code >>>> duplication of the BIOS attributes API as well as a random sysfs file >>>> API that you can never discard. >>> >>> Do you mean the firmware_attributes API? If so, I'm not opposed to adding all the existing ROG attributes to it also. >>> >>> If I'm understanding the docs correctly, for example this apu_mem attr would then become: >>> - /sys/class/firmware-attributes/asus-bios/attributes/apu_mem/type >>> - /sys/class/firmware-attributes/*/attributes/apu_mem/current_value >>> - /sys/class/firmware-attributes/*/attributes/apu_mem/default_value >>> - /sys/class/firmware-attributes/*/attributes/apu_mem/display_name >>> - /sys/class/firmware-attributes/*/attributes/apu_mem/possible_values >>> - ..etc >>> >>> That's absolutely much better than what I've been doing and I wish I'd known about it sooner. >> >> Yup! And even better "fwupdmgr get-bios-setting" and "fwupdmgr >> set-bios-setting" [1] will work with bash completion too. >> >> If you want to build a GUI around it, it could use the fwupd dbus >> interface for getting those settings and manipulating them. > > My existing GUI effort revolves around me adding these attrs to both kernel and a dbus interface. So if I can drop that then all the better > >> The dbus >> interface policy is protected by PK. > > What is PK? And I'm guessing this restricts users to requiring permissions to change settings? > >>> >>> So if I go ahead and convert all the new attr to this are there any issues with also converting much of the previous attr? And I'm aware of "don't break userspace" so really I'm a bit unsure how best to manage that (would a new module be better here also? "asus-bios.c" perhaps). >>> >>> What I don't want is a split between platform and firmware_attributes. >>> >> >> My personal thought is add everything to the new API and then hide the >> "old" interface behind some boolean Kconfig option that defaults to >> disabled. If someone enables it then when it's "used" (IE read from / >> written to) then do something like a pr_warn_once() that it's a >> deprecated interface and it will go away. Wait until next LTS kernel, >> and then rip it out after that. > > I'm happy to do this. On a serious note though, I should make a new module for this right? It would make it much much easier to manage the "conversion" I think, and then it would act as a separation of concerns. Yeah; I agree with you I think this is a good idea. > >> >> Need Hans and/or Ilpo agree to make with that though before you spend >> too much effort on it though. > > Ack > >> >> [1] https://fwupd.github.io/libfwupdplugin/bios-settings.html >>
On 5/28/2024 16:34, Luke Jones wrote: > > > On Wed, 29 May 2024, at 9:16 AM, Mario Limonciello wrote: >>>> For any "new" attributes it's better to put them in that API than code >>>> duplication of the BIOS attributes API as well as a random sysfs file >>>> API that you can never discard. >>> >>> Do you mean the firmware_attributes API? If so, I'm not opposed to adding all the existing ROG attributes to it also. >>> >>> If I'm understanding the docs correctly, for example this apu_mem attr would then become: >>> - /sys/class/firmware-attributes/asus-bios/attributes/apu_mem/type >>> - /sys/class/firmware-attributes/*/attributes/apu_mem/current_value >>> - /sys/class/firmware-attributes/*/attributes/apu_mem/default_value >>> - /sys/class/firmware-attributes/*/attributes/apu_mem/display_name >>> - /sys/class/firmware-attributes/*/attributes/apu_mem/possible_values >>> - ..etc >>> >>> That's absolutely much better than what I've been doing and I wish I'd known about it sooner. >> >> Yup! And even better "fwupdmgr get-bios-setting" and "fwupdmgr >> set-bios-setting" [1] will work with bash completion too. >> >> If you want to build a GUI around it, it could use the fwupd dbus >> interface for getting those settings and manipulating them. > > My existing GUI effort revolves around me adding these attrs to both kernel and a dbus interface. So if I can drop that then all the better Awesome! You can either use libfwupd or the dbus interface. After you're done with the kernel work ping me or Richard if you have problems. > >> The dbus >> interface policy is protected by PK. > > What is PK? And I'm guessing this restricts users to requiring permissions to change settings? > Whoops forgot to respond to these. PK = PolicyKit.
diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi index ac881e72e374..d221a3bc1a81 100644 --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi @@ -245,3 +245,11 @@ Description: Show the maximum performance and efficiency core countin format 0x[E][P] where [E] is the efficiency core count, and [P] is the perfromance core count. + +What: /sys/devices/platform/<platform>/apu_mem +Date: Jun 2024 +KernelVersion: 6.11 +Contact: "Luke Jones" <luke@ljones.dev> +Description: + Set the maximum available system memory for the APU. + * Min=0, Max=8 diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index f62a36dfcd4b..4b5fbae8c563 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -855,6 +855,112 @@ static DEVICE_ATTR_RW(cores_enabled); WMI_SIMPLE_SHOW(cores_max, "0x%x\n", ASUS_WMI_DEVID_CORES_MAX); static DEVICE_ATTR_RO(cores_max); +/* Device memory available to APU */ + +static ssize_t apu_mem_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct asus_wmi *asus = dev_get_drvdata(dev); + int err; + u32 mem; + + err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_APU_MEM, &mem); + if (err < 0) + return err; + + switch (mem) { + case 256: + mem = 0; + break; + case 258: + mem = 1; + break; + case 259: + mem = 2; + break; + case 260: + mem = 3; + break; + case 261: + mem = 4; + break; + case 262: + mem = 8; + break; + case 263: + mem = 5; + break; + case 264: + mem = 6; + break; + case 265: + mem = 7; + break; + default: + mem = 4; + break; + } + + return sysfs_emit(buf, "%d\n", mem); +} + +static ssize_t apu_mem_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct asus_wmi *asus = dev_get_drvdata(dev); + int result, err; + u32 mem; + + result = kstrtou32(buf, 10, &mem); + if (result) + return result; + + switch (mem) { + case 0: + mem = 0; + break; + case 1: + mem = 258; + break; + case 2: + mem = 259; + break; + case 3: + mem = 260; + break; + case 4: + mem = 261; + break; + case 5: + mem = 263; + break; + case 6: + mem = 264; + break; + case 7: + mem = 265; + break; + case 8: + mem = 262; + break; + default: + return -EIO; + } + + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_APU_MEM, mem, &result); + if (err) { + pr_warn("Failed to set apu_mem: %d\n", err); + return err; + } + + pr_info("APU memory changed, reboot required\n"); + sysfs_notify(&asus->platform_device->dev.kobj, NULL, "apu_mem"); + + return count; +} +static DEVICE_ATTR_RW(apu_mem); + /* Tablet mode ****************************************************************/ static void asus_wmi_tablet_mode_get_state(struct asus_wmi *asus) @@ -4100,6 +4206,7 @@ static struct attribute *platform_attributes[] = { &dev_attr_panel_fhd.attr, &dev_attr_cores_enabled.attr, &dev_attr_cores_max.attr, + &dev_attr_apu_mem.attr, &dev_attr_mini_led_mode.attr, &dev_attr_available_mini_led_mode.attr, NULL @@ -4176,6 +4283,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj, else if (attr == &dev_attr_cores_enabled.attr || attr == &dev_attr_cores_max.attr) ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_CORES_SET); + else if (attr == &dev_attr_apu_mem.attr) + ok = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_APU_MEM); else if (attr == &dev_attr_mini_led_mode.attr) ok = asus->mini_led_dev_id != 0; else if (attr == &dev_attr_available_mini_led_mode.attr) diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h index 5a56e7e97785..efe608861e55 100644 --- a/include/linux/platform_data/x86/asus-wmi.h +++ b/include/linux/platform_data/x86/asus-wmi.h @@ -121,6 +121,9 @@ /* Maximum Intel E-core and P-core availability */ #define ASUS_WMI_DEVID_CORES_MAX 0x001200D3 +/* Set the memory available to the APU */ +#define ASUS_WMI_DEVID_APU_MEM 0x000600C1 + /* MCU powersave mode */ #define ASUS_WMI_DEVID_MCU_POWERSAVE 0x001200E2
Exposes the APU memory setting available on a few ASUS models such as the ROG Ally. Signed-off-by: Luke D. Jones <luke@ljones.dev> --- .../ABI/testing/sysfs-platform-asus-wmi | 8 ++ drivers/platform/x86/asus-wmi.c | 109 ++++++++++++++++++ include/linux/platform_data/x86/asus-wmi.h | 3 + 3 files changed, 120 insertions(+)