| Message ID | 20241105153316.378-13-mario.limonciello@amd.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add support for binding ACPI platform profile to multiple drivers | expand |
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@amd.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. > + 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. > + 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. 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);
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@amd.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. > >> + 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); >
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@amd.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); >> >
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); + } + sysfs_notify(&handler->class_dev->kobj, NULL, "platform_profile"); + kobject_uevent(&handler->class_dev->kobj, KOBJ_CHANGE); + } + + sysfs_notify(acpi_kobj, NULL, "platform_profile"); + 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); + + 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);
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@amd.com> --- drivers/acpi/platform_profile.c | 118 ++++++++++++++++++++++++++++++++ 1 file changed, 118 insertions(+)