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@xxxxxxx>
> ---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
