| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <09b21404-2788-463b-966a-fe3f35fe661a@gmx.de>
Date: Wed, 5 Nov 2025 23:20:58 +0100
From: Armin Wolf <W_Armin@....de>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Antheas Kapenekakis <lkml@...heas.dev>
Cc: platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
linux-hwmon@...r.kernel.org, Hans de Goede <hansg@...nel.org>,
Derek John Clark <derekjohn.clark@...il.com>,
Joaquín Ignacio Aramendía <samsagax@...il.com>,
Jean Delvare <jdelvare@...e.com>, Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH v3 4/6] platform/x86: ayaneo-ec: Add controller power and
modules attributes
Am 05.11.25 um 14:24 schrieb Ilpo Järvinen:
> On Wed, 5 Nov 2025, Antheas Kapenekakis wrote:
>
>> On Tue, 4 Nov 2025 at 21:04, Armin Wolf <W_Armin@....de> wrote:
>>> Am 02.11.25 um 19:46 schrieb Antheas Kapenekakis:
>>>
>>>> On Sun, 2 Nov 2025 at 19:30, Armin Wolf <W_Armin@....de> wrote:
>>>>> Am 31.10.25 um 17:36 schrieb Antheas Kapenekakis:
>>>>>
>>>>>> The Ayaneo 3 features hot-swappable controller modules. The ejection
>>>>>> and management is done through HID. However, after ejecting the modules,
>>>>>> the controller needs to be power cycled via the EC to re-initialize.
>>>>>>
>>>>>> For this, the EC provides a variable that holds whether the left or
>>>>>> right modules are connected, and a power control register to turn
>>>>>> the controller on or off. After ejecting the modules, the controller
>>>>>> should be turned off. Then, after both modules are reinserted,
>>>>>> the controller may be powered on again to re-initialize.
>>>>>>
>>>>>> This patch introduces two new sysfs attributes:
>>>>>> - `controller_modules`: a read-only attribute that indicates whether
>>>>>> the left and right modules are connected (none, left, right, both).
>>>>>> - `controller_power`: a read-write attribute that allows the user
>>>>>> to turn the controller on or off (with '1'/'0').
>>>>>>
>>>>>> Therefore, after ejection is complete, userspace can power off the
>>>>>> controller, then wait until both modules have been reinserted
>>>>>> (`controller_modules` will return 'both') to turn on the controller.
>>>>>>
>>>>>> Reviewed-by: Armin Wolf <W_Armin@....de>
>>>>>> Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
>>>>>> ---
>>>>>> .../ABI/testing/sysfs-platform-ayaneo-ec | 19 ++++
>>>>>> MAINTAINERS | 1 +
>>>>>> drivers/platform/x86/ayaneo-ec.c | 106 ++++++++++++++++++
>>>>>> 3 files changed, 126 insertions(+)
>>>>>> create mode 100644 Documentation/ABI/testing/sysfs-platform-ayaneo-ec
>>>>>>
>>>>>> diff --git a/Documentation/ABI/testing/sysfs-platform-ayaneo-ec b/Documentation/ABI/testing/sysfs-platform-ayaneo-ec
>>>>>> new file mode 100644
>>>>>> index 000000000000..3c9c3580c685
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/ABI/testing/sysfs-platform-ayaneo-ec
>>>>>> @@ -0,0 +1,19 @@
>>>>>> +What: /sys/devices/platform/ayaneo-ec/controller_power
>>>>>> +Date: Oct 2025
>>>>> I think you need to update those dates.
>>>>>
>>>>>> +KernelVersion: 6.19
>>>>>> +Contact: "Antheas Kapenekakis" <lkml@...heas.dev>
>>>>>> +Description:
>>>>>> + Current controller power state. Allows turning on and off
>>>>>> + the controller power (e.g. for power savings). Write 1 to
>>>>>> + turn on, 0 to turn off. File is readable and writable.
>>>>>> +
>>>>>> +What: /sys/devices/platform/ayaneo-ec/controller_modules
>>>>>> +Date: Oct 2025
>>>>>> +KernelVersion: 6.19
>>>>>> +Contact: "Antheas Kapenekakis" <lkml@...heas.dev>
>>>>>> +Description:
>>>>>> + Shows which controller modules are currently connected to
>>>>>> + the device. Possible values are "left", "right" and "both".
>>>>>> + File is read-only. The Windows software for this device
>>>>>> + will only set controller power to 1 if both module sides
>>>>>> + are connected (i.e. this file returns "both").
>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>> index da9498d8cc89..b4d62ea9a926 100644
>>>>>> --- a/MAINTAINERS
>>>>>> +++ b/MAINTAINERS
>>>>>> @@ -4191,6 +4191,7 @@ AYANEO PLATFORM EC DRIVER
>>>>>> M: Antheas Kapenekakis <lkml@...heas.dev>
>>>>>> L: platform-driver-x86@...r.kernel.org
>>>>>> S: Maintained
>>>>>> +F: Documentation/ABI/testing/sysfs-platform-ayaneo
>>>>>> F: drivers/platform/x86/ayaneo-ec.c
>>>>>>
>>>>>> AZ6007 DVB DRIVER
>>>>>> diff --git a/drivers/platform/x86/ayaneo-ec.c b/drivers/platform/x86/ayaneo-ec.c
>>>>>> index 697bb053a7d6..0652c044ad76 100644
>>>>>> --- a/drivers/platform/x86/ayaneo-ec.c
>>>>>> +++ b/drivers/platform/x86/ayaneo-ec.c
>>>>>> @@ -8,6 +8,7 @@
>>>>>> */
>>>>>>
>>>>>> #include <linux/acpi.h>
>>>>>> +#include <linux/bits.h>
>>>>>> #include <linux/dmi.h>
>>>>>> #include <linux/err.h>
>>>>>> #include <linux/hwmon.h>
>>>>>> @@ -16,6 +17,7 @@
>>>>>> #include <linux/module.h>
>>>>>> #include <linux/platform_device.h>
>>>>>> #include <linux/power_supply.h>
>>>>>> +#include <linux/sysfs.h>
>>>>>> #include <acpi/battery.h>
>>>>>>
>>>>>> #define AYANEO_PWM_ENABLE_REG 0x4A
>>>>>> @@ -32,9 +34,17 @@
>>>>>> #define AYANEO_CHARGE_VAL_AUTO 0xaa
>>>>>> #define AYANEO_CHARGE_VAL_INHIBIT 0x55
>>>>>>
>>>>>> +#define AYANEO_POWER_REG 0x2d
>>>>>> +#define AYANEO_POWER_OFF 0xfe
>>>>>> +#define AYANEO_POWER_ON 0xff
>>>>>> +#define AYANEO_MODULE_REG 0x2f
>>>>>> +#define AYANEO_MODULE_LEFT BIT(0)
>>>>>> +#define AYANEO_MODULE_RIGHT BIT(1)
>>>>> Using GENMASK() would make sense here.
>>>> Only a single bit is being used though? GENMASK is used for a contiguous series?
>>> I was thinking of using GENMASK() for both bits:
>>>
>>> #define AYANEO_MODULE_MASK GENMASK(1, 0)
>>>
>>> You can then retrieve both bits using FIELD_GET() and simply use a switch statement
>>> together with an enum in controller_modules_show().
>> I will look at it closer if I revise the first 5 patches. The logic of
>> the register is complicated due to the flip, so I would not be eager
>> to.
>>
>> I could do #define AYANEO_MODULE_MASK (AYANEO_MODULE_LEFT |
>> AYANEO_MODULE_RIGHT) for stylistic reasons.
> Hi,
>
> Quickly looking at the code below, a mask define certainly be useful to
> avoid having to embed that into the switch itself below.
>
>> @Ilpo: for the first 5 of this series is there something missing other
>> than perhaps the month? If not, I can respin the last patch on its own
>> later today. I would like this driver + the asus stuff to go into 6.19
>> if possible (there: i dealt with the asusctl bug by skipping the
>> quirk), I am unsure for the timeline for that. I want to say merge
>> window starts in two weeks?
> I've been sick for almost a week so I'm a bit lost with all the progress
> and status with various series so it takes time to catch up until can
> say something when it comes to any particular series (I'm sorry).
I wish you a speedy recovery :)
Thanks,
Armin Wolf
> However, we're only at -rc4 (since Nov 2) so there's 4 or 5 weeks until
> the merge window. And this is new driver so no regression potential other
> than build failures so I don't see big schedule pressure yet, tbh.
>
> I try to take it into the consideration with my review scheduling even
> under normal circumstances that submitters have the opportunity to make it
> assuming one makes suggested changes (if any) in a reasonable time frame.
>
> It generally helps to pay attention to the kernel cycle as closer to the
> merge window we get, it usually helps to prioritize things right, e.g.,
> sending a series on the last week or close to it, getting trivial
> feedback, and not addressing it timely with an update will result in
> missing that kernel cycle; honestly, I always get a bit sad when I see
> people do that.
>
>
>> Antheas
>>
>>> Thanks,
>>> Armin Wolf
>>>
>>>>> With those issues being fixed:
>>>>> Reviewed-by: Armin Wolf <W_Armin@....de>
>>>>>
>>>>>> +
>>>>>> struct ayaneo_ec_quirk {
>>>>>> bool has_fan_control;
>>>>>> bool has_charge_control;
>>>>>> + bool has_magic_modules;
>>>>>> };
>>>>>>
>>>>>> struct ayaneo_ec_platform_data {
>>>>>> @@ -46,6 +56,7 @@ struct ayaneo_ec_platform_data {
>>>>>> static const struct ayaneo_ec_quirk quirk_ayaneo3 = {
>>>>>> .has_fan_control = true,
>>>>>> .has_charge_control = true,
>>>>>> + .has_magic_modules = true,
>>>>>> };
>>>>>>
>>>>>> static const struct dmi_system_id dmi_table[] = {
>>>>>> @@ -266,6 +277,100 @@ static int ayaneo_remove_battery(struct power_supply *battery,
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> +static ssize_t controller_power_store(struct device *dev,
>>>>>> + struct device_attribute *attr,
>>>>>> + const char *buf,
>>>>>> + size_t count)
>>>>>> +{
>>>>>> + bool value;
>>>>>> + int ret;
>>>>>> +
>>>>>> + ret = kstrtobool(buf, &value);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + ret = ec_write(AYANEO_POWER_REG, value ? AYANEO_POWER_ON : AYANEO_POWER_OFF);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + return count;
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t controller_power_show(struct device *dev,
>>>>>> + struct device_attribute *attr,
>>>>>> + char *buf)
>>>>>> +{
>>>>>> + int ret;
>>>>>> + u8 val;
>>>>>> +
>>>>>> + ret = ec_read(AYANEO_POWER_REG, &val);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + return sysfs_emit(buf, "%d\n", val == AYANEO_POWER_ON);
>>>>>> +}
>>>>>> +
>>>>>> +static DEVICE_ATTR_RW(controller_power);
>>>>>> +
>>>>>> +static ssize_t controller_modules_show(struct device *dev,
>>>>>> + struct device_attribute *attr, char *buf)
>>>>>> +{
>>>>>> + char *out;
>>>>>> + int ret;
>>>>>> + u8 val;
>>>>>> +
>>>>>> + ret = ec_read(AYANEO_MODULE_REG, &val);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + switch (~val & (AYANEO_MODULE_LEFT | AYANEO_MODULE_RIGHT)) {
> Why's the extra inversion here? Do those bits actually have the opposite
> meaning compared with their names?
>
> --
> i.
>
>>>>>> + case AYANEO_MODULE_LEFT | AYANEO_MODULE_RIGHT:
>>>>>> + out = "both";
>>>>>> + break;
>>>>>> + case AYANEO_MODULE_LEFT:
>>>>>> + out = "left";
>>>>>> + break;
>>>>>> + case AYANEO_MODULE_RIGHT:
>>>>>> + out = "right";
>>>>>> + break;
>>>>>> + default:
>>>>>> + out = "none";
>>>>>> + break;
>>>>>> + }
>>>>>> +
>>>>>> + return sysfs_emit(buf, "%s\n", out);
>>>>>> +}
>>>>>> +
>>>>>> +static DEVICE_ATTR_RO(controller_modules);
>>>>>> +
>>>>>> +static struct attribute *aya_mm_attrs[] = {
>>>>>> + &dev_attr_controller_power.attr,
>>>>>> + &dev_attr_controller_modules.attr,
>>>>>> + NULL
>>>>>> +};
>>>>>> +
>>>>>> +static umode_t aya_mm_is_visible(struct kobject *kobj,
>>>>>> + struct attribute *attr, int n)
>>>>>> +{
>>>>>> + struct device *dev = kobj_to_dev(kobj);
>>>>>> + struct platform_device *pdev = to_platform_device(dev);
>>>>>> + struct ayaneo_ec_platform_data *data = platform_get_drvdata(pdev);
>>>>>> +
>>>>>> + if (data->quirks->has_magic_modules)
>>>>>> + return attr->mode;
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static const struct attribute_group aya_mm_attribute_group = {
>>>>>> + .is_visible = aya_mm_is_visible,
>>>>>> + .attrs = aya_mm_attrs,
>>>>>> +};
>>>>>> +
>>>>>> +static const struct attribute_group *ayaneo_ec_groups[] = {
>>>>>> + &aya_mm_attribute_group,
>>>>>> + NULL
>>>>>> +};
>>>>>> +
>>>>>> static int ayaneo_ec_probe(struct platform_device *pdev)
>>>>>> {
>>>>>> const struct dmi_system_id *dmi_entry;
>>>>>> @@ -307,6 +412,7 @@ static int ayaneo_ec_probe(struct platform_device *pdev)
>>>>>> static struct platform_driver ayaneo_platform_driver = {
>>>>>> .driver = {
>>>>>> .name = "ayaneo-ec",
>>>>>> + .dev_groups = ayaneo_ec_groups,
>>>>>> },
>>>>>> .probe = ayaneo_ec_probe,
>>>>>> };
Powered by blists - more mailing lists