| 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: <9035b49c-d276-484c-9eb0-a4a5c0f62bc8@gmx.de>
Date: Wed, 6 Nov 2024 20:15:36 +0100
From: Armin Wolf <W_Armin@....de>
To: Mario Limonciello <superm1@...nel.org>,
Mario Limonciello <mario.limonciello@....com>,
Hans de Goede <hdegoede@...hat.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: "Rafael J . Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
Maximilian Luz <luzmaximilian@...il.com>, Lee Chun-Yi <jlee@...e.com>,
Shyam Sundar S K <Shyam-sundar.S-k@....com>,
Corentin Chary <corentin.chary@...il.com>, "Luke D . Jones"
<luke@...nes.dev>, Ike Panhc <ike.pan@...onical.com>,
Henrique de Moraes Holschuh <hmh@....eng.br>,
Alexis Belmonte <alexbelm48@...il.com>,
Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
Ai Chao <aichao@...inos.cn>, Gergo Koteles <soyer@....hu>,
open list <linux-kernel@...r.kernel.org>,
"open list:ACPI" <linux-acpi@...r.kernel.org>,
"open list:MICROSOFT SURFACE PLATFORM PROFILE DRIVER"
<platform-driver-x86@...r.kernel.org>,
"open list:THINKPAD ACPI EXTRAS DRIVER"
<ibm-acpi-devel@...ts.sourceforge.net>,
Mark Pearson <mpearson-lenovo@...ebb.ca>,
Matthew Schwartz <matthew.schwartz@...ux.dev>
Subject: Re: [PATCH v4 12/20] ACPI: platform_profile: Add profile attribute
for class interface
Am 06.11.24 um 06:46 schrieb Mario Limonciello:
>
>
> On 11/5/24 22:10, Armin Wolf wrote:
>> Am 05.11.24 um 16:33 schrieb Mario Limonciello:
>>
>>> Reading and writing the `profile` sysfs file will use the callbacks for
>>> the platform profile handler to read or set the given profile.
>>>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>>> ---
>>> drivers/acpi/platform_profile.c | 118
>>> ++++++++++++++++++++++++++++++++
>>> 1 file changed, 118 insertions(+)
>>>
>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/
>>> platform_profile.c
>>> index e1b6569c4ee70..79083d0bb22e3 100644
>>> --- a/drivers/acpi/platform_profile.c
>>> +++ b/drivers/acpi/platform_profile.c
>>> @@ -65,6 +65,78 @@ static int _get_class_choices(struct device *dev,
>>> unsigned long *choices)
>>> return 0;
>>> }
>>>
>>> +/**
>>> + * _store_class_profile - Set the profile for a class device
>>> + * @dev: The class device
>>> + * @data: The profile to set
>>> + */
>>> +static int _store_class_profile(struct device *dev, void *data)
>>> +{
>>> + enum platform_profile_option profile;
>>> + unsigned long choices;
>>> + int *i = (int *)data;
>>> + int err;
>>> +
>>> + err = _get_class_choices(dev, &choices);
>>> + if (err)
>>> + return err;
>>> +
>>> + scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> + struct platform_profile_handler *handler;
>>> +
>>> + if (!test_bit(*i, &choices))
>>> + return -EOPNOTSUPP;
>>> +
>>> + handler = dev_get_drvdata(dev);
>>> + err = handler->profile_get(handler, &profile);
>>> + if (err)
>>> + return err;
>>> +
>>> + err = handler->profile_set(handler, *i);
>>> + if (err) {
>>> + int recover_err;
>>> +
>>> + dev_err(dev, "Failed to set profile: %d\n", err);
>>> + recover_err = handler->profile_set(handler, profile);
>>> + if (recover_err)
>>> + dev_err(dev, "Failed to reset profile: %d\n",
>>> recover_err);
>>> + }
>>
>> The whole recovery handling seems unnecessary to me. In setting the
>> platform profile fails, then
>> we should just return an error. The platform profile handler will
>> tell us the current platform
>> profile anyway.
>
> Sure, makes sense. That also means no need to capture the profile
> before setting it.
>
>>
>>> + sysfs_notify(&handler->class_dev->kobj, NULL, "platform_profile");
>>> + kobject_uevent(&handler->class_dev->kobj, KOBJ_CHANGE);
>>
>> Please avoid sending those events when the platform profile is
>> changed through the class sysfs interface.
>>
>>> + }
>>> +
>>> + sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>
>> Please avoid sending this event when the platform profile is changed
>> through the legacy sysfs interface.
>
> In both above cases - why?
>
> * If I change using class interface then that implicitly means that
> legacy interface changes.
> * If I change using legacy interface that implicitly means class
> interface changes too.
>
I meant that:
- if the profile is changed using the class interface then only the legacy interface should be notified
- if the profile is changed using the legacy interface then only the class interface should be notified
Thanks,
Armin Wolf
>>
>>> + return err ? err : 0;
>>> +}
>>> +
>>> +/**
>>> + * get_class_profile - Show the current profile for a class device
>>> + * @dev: The class device
>>> + * @profile: The profile to return
>>> + * Return: 0 on success, -errno on failure
>>> + */
>>> +static int get_class_profile(struct device *dev,
>>> + enum platform_profile_option *profile)
>>> +{
>>> + struct platform_profile_handler *handler;
>>> + enum platform_profile_option val;
>>> + int err;
>>> +
>>> + scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> + handler = dev_get_drvdata(dev);
>>> + err = handler->profile_get(handler, &val);
>>> + if (err) {
>>> + pr_err("Failed to get profile for handler %s\n",
>>> handler- >name);
>>> + return err;
>>> + }
>>> + }
>>> +
>>> + if (WARN_ON(val >= PLATFORM_PROFILE_LAST))
>>> + return -EINVAL;
>>> + *profile = val;
>>> +
>>> + return 0;
>>> +}
>>>
>>> /**
>>> * name_show - Show the name of the profile handler
>>> @@ -102,12 +174,58 @@ static ssize_t choices_show(struct device *dev,
>>> return _commmon_choices_show(choices, buf);
>>> }
>>>
>>> +/**
>>> + * profile_show - Show the current profile for a class device
>>> + * @dev: The device
>>> + * @attr: The attribute
>>> + * @buf: The buffer to write to
>>> + * Return: The number of bytes written
>>> + */
>>> +static ssize_t profile_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
>>> + int err;
>>> +
>>> + err = get_class_profile(dev, &profile);
>>> + if (err)
>>> + return err;
>>> +
>>> + return sysfs_emit(buf, "%s\n", profile_names[profile]);
>>> +}
>>> +
>>> +/**
>>> + * profile_store - Set the profile for a class device
>>> + * @dev: The device
>>> + * @attr: The attribute
>>> + * @buf: The buffer to read from
>>> + * @count: The number of bytes to read
>>> + * Return: The number of bytes read
>>> + */
>>> +static ssize_t profile_store(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + int i, ret;
>>> +
>>> + i = sysfs_match_string(profile_names, buf);
>>> + if (i < 0)
>>> + return -EINVAL;
>>> +
>>> + ret = _store_class_profile(dev, (void *)(long)&i);
>>
>> Please just pass &i.
>
> Ack.
>
>>
>> Thanks,
>> Armin Wolf
>>
>>> +
>>> + return ret ? ret : count;
>>> +}
>>>
>>> static DEVICE_ATTR_RO(name);
>>> static DEVICE_ATTR_RO(choices);
>>> +static DEVICE_ATTR_RW(profile);
>>> +
>>> static struct attribute *profile_attrs[] = {
>>> &dev_attr_name.attr,
>>> &dev_attr_choices.attr,
>>> + &dev_attr_profile.attr,
>>> NULL
>>> };
>>> ATTRIBUTE_GROUPS(profile);
>>
>
Powered by blists - more mailing lists