| 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: <7a08bb90-84b3-4536-81f1-8542a11dbff3@gmx.de>
Date: Fri, 8 Nov 2024 19:00:21 +0100
From: Armin Wolf <W_Armin@....de>
To: 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 v5 12/20] ACPI: platform_profile: Add profile attribute
for class interface
Am 07.11.24 um 22:41 schrieb Mario Limonciello:
> On 11/7/2024 02:34, Armin Wolf wrote:
>> Am 07.11.24 um 07:02 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.
>>>
>>> Tested-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>>> ---
>>> v5:
>>> * Drop recovery flow
>>> * Don't get profile before setting (not needed)
>>> * Simplify casting for call to _store_class_profile()
>>> * Only notify legacy interface of changes
>>> * Adjust mutex use
>>> ---
>>> drivers/acpi/platform_profile.c | 110
>>> ++++++++++++++++++++++++++++++++
>>> 1 file changed, 110 insertions(+)
>>>
>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/
>>> platform_profile.c
>>> index 5e0bb91c5f451..35e0e8f666072 100644
>>> --- a/drivers/acpi/platform_profile.c
>>> +++ b/drivers/acpi/platform_profile.c
>>> @@ -65,6 +65,62 @@ 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)
>>> +{
>>> + struct platform_profile_handler *handler;
>>> + unsigned long choices;
>>> + int *i = (int *)data;
>>> + int err;
>>> +
>>> + err = _get_class_choices(dev, &choices);
>>> + if (err)
>>> + return err;
>>> +
>>> + lockdep_assert_held(&profile_lock);
>>> + if (!test_bit(*i, &choices))
>>> + return -EOPNOTSUPP;
>>> +
>>> + handler = dev_get_drvdata(dev);
>>> + err = handler->profile_set(handler, *i);
>>> + if (err)
>>> + return err;
>>> +
>>> + return err ? err : 0;
>>
>> Please just return 0 here.
>>
>>> +}
>>> +
>>> +/**
>>> + * 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;
>>> +
>>> + lockdep_assert_held(&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
>>> * @dev: The device
>>> @@ -106,12 +162,66 @@ 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;
>>> +
>>> + scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> + err = get_class_profile(dev, &profile);
>>> + if (err)
>>> + return err;
>>> + }
>>> +
>>> + return sysfs_emit(buf, "%s\n", profile_names[profile]);
>>
>> AFAIK we do not need to take the mutex here, since querying the
>> current platform profile
>> should not change any state.
>
> I think it's still needed, in case someone attempts to unload the driver
> at the same time as it's being read. It's not static information
> because it needs to use the function pointer into the driver to get it.
>
> This will protect from that occurring.
>
> That's the same reason I was thinking name needed protection too.
>
I see, good point.
Thanks,
Armin Wolf
>>
>> Thanks,
>> Armin Wolf
>>
>>> +}
>>> +
>>> +/**
>>> + * 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;
>>> +
>>> + scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> + ret = _store_class_profile(dev, &i);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>> +
>>> + return 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