| Message ID | 20220912090641.111658-3-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86/amd/pmf: Introduce CnQF feature for AMD PMF | expand |
On 9/12/2022 04:06, Shyam Sundar S K wrote: > Whether to turn CnQF on/off by default upon driver load would be decided > by a BIOS flag. Add a sysfs node to provide a way to the user whether to > use static slider or CnQF . > > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > drivers/platform/x86/amd/pmf/cnqf.c | 57 +++++++++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c > index aebcef778a0b..8d0c1eff1659 100644 > --- a/drivers/platform/x86/amd/pmf/cnqf.c > +++ b/drivers/platform/x86/amd/pmf/cnqf.c > @@ -294,9 +294,64 @@ void amd_pmf_load_defaults_cnqf(struct amd_pmf_dev *dev) > config_store.trans_prio[i] = i; > } > > +static ssize_t feat_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct amd_pmf_dev *pdev = dev_get_drvdata(dev); > + int mode = amd_pmf_get_pprof_modes(pdev); > + int result, src; > + bool input; > + > + result = kstrtobool(buf, &input); > + if (result) > + return result; > + > + src = amd_pmf_get_power_source(); > + pdev->cnqf_enabled = input; > + > + if (mode < 0) > + return mode; > + > + if (pdev->cnqf_enabled) { > + amd_pmf_set_cnqf(pdev, src, config_store.current_mode, NULL); > + } else { > + if (is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) { > + amd_pmf_init_sps(pdev); > + amd_pmf_update_slider(pdev, SLIDER_OP_SET, mode, NULL); > + } > + } > + > + dev_dbg(pdev->dev, "Received CnQF %s\n", input ? "on" : "off"); > + return count; > +} > + > +static ssize_t feat_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct amd_pmf_dev *pdev = dev_get_drvdata(dev); > + > + return sprintf(buf, "%s\n", pdev->cnqf_enabled ? "on" : "off"); > +} > + > +static DEVICE_ATTR_RW(feat); > + > +static struct attribute *cnqf_feature_attrs[] = { > + &dev_attr_feat.attr, > + NULL > +}; > + > +static const struct attribute_group cnqf_feature_attribute_group = { > + .attrs = cnqf_feature_attrs, > + .name = "cnqf" Perhaps you should have a "is_visible" controlled by a function that checks "is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR)". This will then let you adjust "feat_store" to not have to check this and also only expose the attribute on supported systems. > +}; > + > void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev) > { > cancel_delayed_work_sync(&dev->work_buffer); > + sysfs_remove_group(&dev->dev->kobj, &cnqf_feature_attribute_group); > + kobject_uevent(&dev->dev->kobj, KOBJ_CHANGE); > } > > void amd_pmf_init_cnqf(struct amd_pmf_dev *dev) > @@ -316,4 +371,6 @@ void amd_pmf_init_cnqf(struct amd_pmf_dev *dev) > /* update the thermal for CnQF */ > src = amd_pmf_get_power_source(); > amd_pmf_set_cnqf(dev, src, config_store.current_mode, NULL); > + ret = sysfs_create_group(&dev->dev->kobj, &cnqf_feature_attribute_group); > + kobject_uevent(&dev->dev->kobj, KOBJ_CHANGE); > }
Hi Mario, On 9/12/2022 7:36 PM, Limonciello, Mario wrote: > On 9/12/2022 04:06, Shyam Sundar S K wrote: >> Whether to turn CnQF on/off by default upon driver load would be decided >> by a BIOS flag. Add a sysfs node to provide a way to the user whether to >> use static slider or CnQF . >> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >> --- >> drivers/platform/x86/amd/pmf/cnqf.c | 57 +++++++++++++++++++++++++++++ >> 1 file changed, 57 insertions(+) >> >> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c >> b/drivers/platform/x86/amd/pmf/cnqf.c >> index aebcef778a0b..8d0c1eff1659 100644 >> --- a/drivers/platform/x86/amd/pmf/cnqf.c >> +++ b/drivers/platform/x86/amd/pmf/cnqf.c >> @@ -294,9 +294,64 @@ void amd_pmf_load_defaults_cnqf(struct >> amd_pmf_dev *dev) >> config_store.trans_prio[i] = i; >> } >> +static ssize_t feat_store(struct device *dev, >> + struct device_attribute *attr, >> + const char *buf, size_t count) >> +{ >> + struct amd_pmf_dev *pdev = dev_get_drvdata(dev); >> + int mode = amd_pmf_get_pprof_modes(pdev); >> + int result, src; >> + bool input; >> + >> + result = kstrtobool(buf, &input); >> + if (result) >> + return result; >> + >> + src = amd_pmf_get_power_source(); >> + pdev->cnqf_enabled = input; >> + >> + if (mode < 0) >> + return mode; >> + >> + if (pdev->cnqf_enabled) { >> + amd_pmf_set_cnqf(pdev, src, config_store.current_mode, NULL); >> + } else { >> + if (is_apmf_func_supported(pdev, >> APMF_FUNC_STATIC_SLIDER_GRANULAR)) { >> + amd_pmf_init_sps(pdev); >> + amd_pmf_update_slider(pdev, SLIDER_OP_SET, mode, NULL); >> + } >> + } >> + >> + dev_dbg(pdev->dev, "Received CnQF %s\n", input ? "on" : "off"); >> + return count; >> +} >> + >> +static ssize_t feat_show(struct device *dev, >> + struct device_attribute *attr, >> + char *buf) >> +{ >> + struct amd_pmf_dev *pdev = dev_get_drvdata(dev); >> + >> + return sprintf(buf, "%s\n", pdev->cnqf_enabled ? "on" : "off"); >> +} >> + >> +static DEVICE_ATTR_RW(feat); >> + >> +static struct attribute *cnqf_feature_attrs[] = { >> + &dev_attr_feat.attr, >> + NULL >> +}; >> + >> +static const struct attribute_group cnqf_feature_attribute_group = { >> + .attrs = cnqf_feature_attrs, >> + .name = "cnqf" > > Perhaps you should have a "is_visible" controlled by a function that > checks "is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR)". > > This will then let you adjust "feat_store" to not have to check this and > also only expose the attribute on supported systems. > OK. Will do this in the v3. Thanks, Shyam >> +}; >> + >> void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev) >> { >> cancel_delayed_work_sync(&dev->work_buffer); >> + sysfs_remove_group(&dev->dev->kobj, &cnqf_feature_attribute_group); >> + kobject_uevent(&dev->dev->kobj, KOBJ_CHANGE); >> } >> void amd_pmf_init_cnqf(struct amd_pmf_dev *dev) >> @@ -316,4 +371,6 @@ void amd_pmf_init_cnqf(struct amd_pmf_dev *dev) >> /* update the thermal for CnQF */ >> src = amd_pmf_get_power_source(); >> amd_pmf_set_cnqf(dev, src, config_store.current_mode, NULL); >> + ret = sysfs_create_group(&dev->dev->kobj, >> &cnqf_feature_attribute_group); >> + kobject_uevent(&dev->dev->kobj, KOBJ_CHANGE); >> } >
Hi, On 9/12/22 10:06, Shyam Sundar S K wrote: > Whether to turn CnQF on/off by default upon driver load would be decided > by a BIOS flag. Add a sysfs node to provide a way to the user whether to > use static slider or CnQF . > > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > ---1 > drivers/platform/x86/amd/pmf/cnqf.c | 57 +++++++++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c > index aebcef778a0b..8d0c1eff1659 100644 > --- a/drivers/platform/x86/amd/pmf/cnqf.c > +++ b/drivers/platform/x86/amd/pmf/cnqf.c > @@ -294,9 +294,64 @@ void amd_pmf_load_defaults_cnqf(struct amd_pmf_dev *dev) > config_store.trans_prio[i] = i; > } > > +static ssize_t feat_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) I don't like the "feat" name, why not just call it "enable", or even better call it "cnqf_enable" and drop the extra cnqf subdir in sysfs? > +{ > + struct amd_pmf_dev *pdev = dev_get_drvdata(dev); > + int mode = amd_pmf_get_pprof_modes(pdev); > + int result, src; > + bool input; > + > + result = kstrtobool(buf, &input); > + if (result) > + return result; > + > + src = amd_pmf_get_power_source(); > + pdev->cnqf_enabled = input; > + > + if (mode < 0) > + return mode; You have already set pdev->cnqf_enabled here, while you are returning an error. If you return an error then no changes to the internal state should be made, so please move this check up. > + > + if (pdev->cnqf_enabled) { > + amd_pmf_set_cnqf(pdev, src, config_store.current_mode, NULL); This is missing a "if (dev->current_profile == PLATFORM_PROFILE_BALANCED)" check. Also amd_pmf_init_cnqf() will call: amd_pmf_load_defaults_cnqf(dev); amd_pmf_init_metrics_table(dev); Only when the BIOS has things enabled, so if the user now writes 1 or on to this sysfs attribute then cnqf will be enabled but none of its limits / other settings will be set so it will be very broken in this case! It seems to me that: amd_pmf_load_defaults_cnqf(dev); amd_pmf_init_metrics_table(dev); Should always be called on systems which support cnqf. As mentioned in my review of patch 1/4 Please use 2 separate cnqf_supported and cnqf_enabled flags. Also this function should then never be able to run when cnqf_supported is false, as Mario already mentioned you should use a is_visible callback for this. > + } else { > + if (is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) { > + amd_pmf_init_sps(pdev); > + amd_pmf_update_slider(pdev, SLIDER_OP_SET, mode, NULL); > + } > + } > + > + dev_dbg(pdev->dev, "Received CnQF %s\n", input ? "on" : "off"); > + return count; > +} > + > +static ssize_t feat_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct amd_pmf_dev *pdev = dev_get_drvdata(dev); > + > + return sprintf(buf, "%s\n", pdev->cnqf_enabled ? "on" : "off"); > +} > + > +static DEVICE_ATTR_RW(feat); > + > +static struct attribute *cnqf_feature_attrs[] = { > + &dev_attr_feat.attr, > + NULL > +}; > + > +static const struct attribute_group cnqf_feature_attribute_group = { > + .attrs = cnqf_feature_attrs, > + .name = "cnqf" > +}; > + > void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev) > { > cancel_delayed_work_sync(&dev->work_buffer); > + sysfs_remove_group(&dev->dev->kobj, &cnqf_feature_attribute_group); > + kobject_uevent(&dev->dev->kobj, KOBJ_CHANGE); Drop these 2 lines (see below). > } > > void amd_pmf_init_cnqf(struct amd_pmf_dev *dev) > @@ -316,4 +371,6 @@ void amd_pmf_init_cnqf(struct amd_pmf_dev *dev) > /* update the thermal for CnQF */ > src = amd_pmf_get_power_source(); > amd_pmf_set_cnqf(dev, src, config_store.current_mode, NULL); > + ret = sysfs_create_group(&dev->dev->kobj, &cnqf_feature_attribute_group); > + kobject_uevent(&dev->dev->kobj, KOBJ_CHANGE); > } Manual sysfs attr registration like this is known to be racy. Please use the .groups member of the driver struct instead to let the core handle this, combined with an is_visible callback to hide the attribute on systems where this is not supported. See e.g. how thinkpad_acpi.c does this. Regards, Hans
diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c index aebcef778a0b..8d0c1eff1659 100644 --- a/drivers/platform/x86/amd/pmf/cnqf.c +++ b/drivers/platform/x86/amd/pmf/cnqf.c @@ -294,9 +294,64 @@ void amd_pmf_load_defaults_cnqf(struct amd_pmf_dev *dev) config_store.trans_prio[i] = i; } +static ssize_t feat_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct amd_pmf_dev *pdev = dev_get_drvdata(dev); + int mode = amd_pmf_get_pprof_modes(pdev); + int result, src; + bool input; + + result = kstrtobool(buf, &input); + if (result) + return result; + + src = amd_pmf_get_power_source(); + pdev->cnqf_enabled = input; + + if (mode < 0) + return mode; + + if (pdev->cnqf_enabled) { + amd_pmf_set_cnqf(pdev, src, config_store.current_mode, NULL); + } else { + if (is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) { + amd_pmf_init_sps(pdev); + amd_pmf_update_slider(pdev, SLIDER_OP_SET, mode, NULL); + } + } + + dev_dbg(pdev->dev, "Received CnQF %s\n", input ? "on" : "off"); + return count; +} + +static ssize_t feat_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct amd_pmf_dev *pdev = dev_get_drvdata(dev); + + return sprintf(buf, "%s\n", pdev->cnqf_enabled ? "on" : "off"); +} + +static DEVICE_ATTR_RW(feat); + +static struct attribute *cnqf_feature_attrs[] = { + &dev_attr_feat.attr, + NULL +}; + +static const struct attribute_group cnqf_feature_attribute_group = { + .attrs = cnqf_feature_attrs, + .name = "cnqf" +}; + void amd_pmf_deinit_cnqf(struct amd_pmf_dev *dev) { cancel_delayed_work_sync(&dev->work_buffer); + sysfs_remove_group(&dev->dev->kobj, &cnqf_feature_attribute_group); + kobject_uevent(&dev->dev->kobj, KOBJ_CHANGE); } void amd_pmf_init_cnqf(struct amd_pmf_dev *dev) @@ -316,4 +371,6 @@ void amd_pmf_init_cnqf(struct amd_pmf_dev *dev) /* update the thermal for CnQF */ src = amd_pmf_get_power_source(); amd_pmf_set_cnqf(dev, src, config_store.current_mode, NULL); + ret = sysfs_create_group(&dev->dev->kobj, &cnqf_feature_attribute_group); + kobject_uevent(&dev->dev->kobj, KOBJ_CHANGE); }
Whether to turn CnQF on/off by default upon driver load would be decided by a BIOS flag. Add a sysfs node to provide a way to the user whether to use static slider or CnQF . Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> --- drivers/platform/x86/amd/pmf/cnqf.c | 57 +++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+)