Received-SPF: pass (google.com: domain of linux-kernel+bounces-192014-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Tue, 28 May 2024 12:27:26 +0300 (EEST)
To: "Luke D. Jones" <luke@ljones.dev>, Hans de Goede <hdegoede@redhat.com>
cc: corentin.chary@gmail.com, platform-driver-x86@vger.kernel.org, 
    LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 7/9] platform/x86: asus-wmi: add enable/disable CPU
 cores
In-Reply-To: <20240528013626.14066-8-luke@ljones.dev>
Message-ID: <4660daf1-f42f-9b65-eaf5-30daf2931058@linux.intel.com>
References: <20240528013626.14066-1-luke@ljones.dev> <20240528013626.14066-8-luke@ljones.dev>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

Hi,

Hans, please check my question below.

On Tue, 28 May 2024, Luke D. Jones wrote:

> Exposes the WMI functions for enable/disable of performance and
> efficiency cores on some laptop models (largely Intel only).
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---

> diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> index 3b4eeea75b7b..ac881e72e374 100644
> --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi
> +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi
> @@ -226,3 +226,22 @@ Description:
>  		Set panel to UHD or FHD mode
>  			* 0 - UHD,
>  			* 1 - FHD
> +
> +What:		/sys/devices/platform/<platform>/cores_enabled
> +Date:		Jun 2024
> +KernelVersion:	6.11
> +Contact:	"Luke Jones" <luke@ljones.dev>
> +Description:
> +		Enable/disable efficiency and performance cores. The format is
> +		0x[E][P] where [E] is the efficiency core count, and [P] is
> +		the perfromance core count. If the core count is a single digit

performance

> +		it is preceded by a 0 such as 0x0406; E=4, P=6, 0x1006; E=10, P=6
> +
> +What:		/sys/devices/platform/<platform>/cores_max
> +Date:		Jun 2024
> +KernelVersion:	6.11
> +Contact:	"Luke Jones" <luke@ljones.dev>
> +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.

performance

> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 4b045f1828f1..f62a36dfcd4b 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -815,6 +815,46 @@ static ssize_t panel_fhd_store(struct device *dev,
>  WMI_SIMPLE_SHOW(panel_fhd, "%d\n", ASUS_WMI_DEVID_PANEL_FHD);
>  static DEVICE_ATTR_RW(panel_fhd);
>  
> +/* Efficiency and Performance core control **********************************/
> +static ssize_t cores_enabled_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 cores, max;
> +
> +	result = kstrtou32(buf, 16, &cores);
> +	if (result)
> +		return result;
> +
> +	err = asus_wmi_get_devstate(asus, ASUS_WMI_DEVID_CORES_MAX, &max);
> +	if (err < 0)
> +		return err;
> +
> +	if (cores > max) {

This only checks one part of it and the P part can contain whatever 
garbage as long as E is small enough?

But I'm not sure if it's good idea to have these two changed through the 
same sysfs file, I'm leaning more on that it would be better to split the 
interface for P and E.

Hans, what you think about this?

> +		pr_warn("Core count 0x%x exceeds max: 0x%x\n", cores, max);
> +		return -EIO;
> +	}
> +
> +	err = asus_wmi_set_devstate(ASUS_WMI_DEVID_CORES_SET, cores, &result);
> +	if (err) {
> +		pr_warn("Failed to set cores_enabled: %d\n", err);
> +		return err;
> +	}
> +
> +	pr_info("Enabled core count changed, reboot required\n");
> +	sysfs_notify(&asus->platform_device->dev.kobj, NULL, "cores_enabled");
> +
> +	return count;
> +}

> @@ -4131,6 +4173,9 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj,
>  		devid = ASUS_WMI_DEVID_PANEL_OD;
>  	else if (attr == &dev_attr_panel_fhd.attr)
>  		devid = ASUS_WMI_DEVID_PANEL_FHD;
> +	else if (attr == &dev_attr_cores_enabled.attr
> +		|| attr == &dev_attr_cores_max.attr)

Wrong alignment.

-- 
 i.