Received-SPF: pass (google.com: domain of linux-kernel+bounces-191655-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Feedback-ID: i5ec1447f:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-491-g033e30d24-fm-20240520.001-g033e30d2
Precedence: bulk
MIME-Version: 1.0
Message-Id: <4d6b9171-7248-4937-87de-7e921ed8e507@app.fastmail.com>
In-Reply-To: <6f4bc109-00d0-47b0-a581-b96a6152545c@amd.com>
References: <20240528013626.14066-1-luke@ljones.dev>
 <20240528013626.14066-9-luke@ljones.dev>
 <6f4bc109-00d0-47b0-a581-b96a6152545c@amd.com>
Date: Tue, 28 May 2024 14:40:16 +1200
From: "Luke Jones" <luke@ljones.dev>
To: "Limonciello, Mario" <mario.limonciello@amd.com>,
 "Hans de Goede" <hdegoede@redhat.com>
Cc: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>,
 corentin.chary@gmail.com, platform-driver-x86@vger.kernel.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH 8/9] platform/x86: asus-wmi: add apu_mem setting
Content-Type: text/plain


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
> >   
>