Re: [PATCHv3 6/6] drivers/cpufreq: Add a CPUFreq driver for AMD processors (Fam17h and later)

[Janakarajan Natarajan]

On 7/11/2019 1:12 AM, Viresh Kumar wrote:
> On 10-07-19, 18:37, Natarajan, Janakarajan wrote:
>> diff --git a/drivers/cpufreq/amd-cpufreq.c b/drivers/cpufreq/amd-cpufreq.c
>> +#define pr_fmt(fmt)	"AMD Cpufreq: " fmt
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/cpu.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/cpufreq.h>
>> +#include <linux/acpi.h>
>> +#include <linux/delay.h>
> Please keep them in alphabetical order.


Ok.


>
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#include <acpi/cppc_acpi.h>
>> +
>> +struct amd_desc {
>> +	int cpu_id;
>> +	struct cppc_ctrls ctrls;
>> +	struct kobject kobj;
>> +};
>> +
>> +struct amd_desc **all_cpu_data;
>> +
>> +static unsigned int cppc_enable;
>> +module_param(cppc_enable, uint, 0644);
>> +MODULE_PARM_DESC(cppc_enable,
>> +		 "1 - enable AMD CpuFreq, create CPPC sysfs entries.");
>> +
>> +#define to_amd_desc(a) container_of(a, struct amd_desc, kobj)
>> +
>> +#define show_func(access_fn, struct_name, member_name)			\
>> +	static ssize_t show_##member_name(struct kobject *kobj,		\
>> +					  struct kobj_attribute *attr,	\
>> +					  char *buf)			\
>> +	{								\
>> +		struct amd_desc *desc = to_amd_desc(kobj);		\
>> +		struct struct_name st_name = {0};			\
>> +		int ret;						\
>> +									\
>> +		ret = access_fn(desc->cpu_id, &st_name);		\
>> +		if (ret)						\
>> +			return ret;					\
>> +									\
>> +		return scnprintf(buf, PAGE_SIZE, "%llu\n",		\
>> +				 (u64)st_name.member_name);		\
>> +	}								\
>> +
>> +#define store_func(struct_name, member_name, reg_idx)			\
>> +	static ssize_t store_##member_name(struct kobject *kobj,	\
>> +					   struct kobj_attribute *attr,	\
>> +					   const char *buf, size_t count)\
>> +	{								\
>> +		struct amd_desc *desc = to_amd_desc(kobj);		\
>> +		struct struct_name st_name = {0};			\
>> +		u32 val;						\
>> +		int ret;						\
>> +									\
>> +		ret = kstrtou32(buf, 0, &val);				\
>> +		if (ret)						\
>> +			return ret;					\
>> +									\
>> +		st_name.member_name = val;				\
>> +									\
>> +		ret = cppc_set_reg(desc->cpu_id, &st_name, reg_idx);	\
>> +		if (ret)						\
>> +			return ret;					\
>> +									\
>> +		return count;						\
>> +	}								\
>> +
>> +#define define_one_rw(struct_name, access_fn, member_name, reg_idx)	\
>> +	show_func(access_fn, struct_name, member_name)			\
>> +	store_func(struct_name, member_name, reg_idx)			\
>> +	define_one_global_rw(member_name)
>> +
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, enable, ENABLE);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, max_perf, MAX_PERF);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, min_perf, MIN_PERF);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, desired_perf, DESIRED_PERF);
>> +define_one_rw(cppc_ctrls, cppc_get_ctrls, auto_sel_enable, AUTO_SEL_ENABLE);
>> +
>> +static struct attribute *amd_cpufreq_attributes[] = {
>> +	&enable.attr,
>> +	&max_perf.attr,
>> +	&min_perf.attr,
>> +	&desired_perf.attr,
>> +	&auto_sel_enable.attr,
>> +	NULL
>> +};
>> +
>> +static const struct attribute_group amd_cpufreq_attr_group = {
>> +	.attrs = amd_cpufreq_attributes,
>> +};
>> +
>> +static struct kobj_type amd_cpufreq_type = {
>> +	.sysfs_ops = &kobj_sysfs_ops,
>> +	.default_attrs = amd_cpufreq_attributes,
>> +};
>> +
>> +static int amd_cpufreq_cpu_init(struct cpufreq_policy *policy)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int amd_cpufreq_cpu_exit(struct cpufreq_policy *policy)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int amd_cpufreq_cpu_verify(struct cpufreq_policy *policy)
>> +{
>> +	return 0;
>> +}
>> +
>> +static int amd_cpufreq_cpu_target_index(struct cpufreq_policy *policy,
>> +					unsigned int index)
>> +{
>> +	return 0;
>> +}
> All empty helpers ? There is nothing you need to do ?


When we posted v2 of this patchset, Rafael let us know that he was 
uncomfortable

going behind the (acpi-cpufreq) drivers back by letting the user 
communicate directly

with the platform. That's the reason we have an empty driver whose 
primary purpose

is to expose sysfs entries for the user.


>
>> +
>> +static struct cpufreq_driver amd_cpufreq_driver = {
>> +	.name = "amd_cpufreq",
>> +	.init = amd_cpufreq_cpu_init,
>> +	.exit = amd_cpufreq_cpu_exit,
>> +	.verify = amd_cpufreq_cpu_verify,
>> +	.target_index = amd_cpufreq_cpu_target_index,
>> +};
>> +
>> +static void amd_cpufreq_sysfs_delete_params(void)
>> +{
>> +	int i;
>> +
>> +	for_each_possible_cpu(i) {
>> +		if (all_cpu_data[i]) {
>> +			kobject_del(&all_cpu_data[i]->kobj);
> Shouldn't you use kobject_put() instead of this ?


Ok.


>
>> +			kfree(all_cpu_data[i]);
>> +		}
>> +	}
>> +
>> +	kfree(all_cpu_data);
>> +}
>> +
>> +static int __init amd_cpufreq_sysfs_expose_params(void)
>> +{
>> +	struct device *cpu_dev;
>> +	int i, ret;
>> +
>> +	all_cpu_data = kcalloc(num_possible_cpus(), sizeof(void *),
>> +			       GFP_KERNEL);
>> +
>> +	if (!all_cpu_data)
>> +		return -ENOMEM;
>> +
>> +	for_each_possible_cpu(i) {
>> +		all_cpu_data[i] = kzalloc(sizeof(struct amd_desc), GFP_KERNEL);
>> +		if (!all_cpu_data[i]) {
>> +			ret = -ENOMEM;
>> +			goto free;
>> +		}
>> +
>> +		all_cpu_data[i]->cpu_id = i;
>> +		cpu_dev = get_cpu_device(i);
>> +		ret = kobject_init_and_add(&all_cpu_data[i]->kobj, &amd_cpufreq_type,
>> +					   &cpu_dev->kobj, "amd_cpufreq");
>> +		if (ret)
>> +			goto free;
>> +	}
>> +
>> +	return 0;
>> +free:
>> +	amd_cpufreq_sysfs_delete_params();
>> +	return ret;
>> +}
> Instead of doing this once for all CPUs, it would be better to do it
> every time the ->init() callback of the driver gets called. If you
> have one cpufreq policy for each CPU (i.e. no CPUs share clock lines),
> then the init() callback will get called once for each CPU.


We are trying to avoid any use of the cpufreq mechanism.


Thanks,

Janakarajan


>
>> +
>> +static int __init amd_cpufreq_init(void)
>> +{
>> +	int ret = 0;
>> +
>> +	/*
>> +	 * Use only if:
>> +	 * - AMD,
>> +	 * - Family 17h (or) newer and,
>> +	 * - Explicitly enabled
>> +	 */
>> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
>> +	    boot_cpu_data.x86 < 0x17 || !cppc_enable)
>> +		return -ENODEV;
>> +
>> +	ret = cpufreq_register_driver(&amd_cpufreq_driver);
>> +	if (ret) {
>> +		pr_info("Failed to register driver\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = amd_cpufreq_sysfs_expose_params();
>> +	if (ret) {
>> +		pr_info("Could not create sysfs entries\n");
>> +		cpufreq_unregister_driver(&amd_cpufreq_driver);
>> +		goto out;
>> +	}
>> +
>> +	pr_info("Using amd-cpufreq driver\n");
>> +	return ret;
>> +
>> +out:
>> +	return ret;
>> +}
>> +
>> +static void __exit amd_cpufreq_exit(void)
>> +{
>> +	amd_cpufreq_sysfs_delete_params();
>> +	cpufreq_unregister_driver(&amd_cpufreq_driver);
>> +}
>> +
>> +static const struct acpi_device_id amd_acpi_ids[] __used = {
>> +	{ACPI_PROCESSOR_DEVICE_HID, },
>> +	{}
>> +};
>> +
>> +device_initcall(amd_cpufreq_init);
>> +module_exit(amd_cpufreq_exit);
>> +MODULE_DEVICE_TABLE(acpi, amd_acpi_ids);
> All three should be placed directly below the struct/function they
> represent without any blank lines in between. As suggested in
> kernel documentation.
>
>> +
>> +MODULE_AUTHOR("Janakarajan Natarajan");
>> +MODULE_DESCRIPTION("AMD CPUFreq driver based on ACPI CPPC v6.1 spec");
>> +MODULE_LICENSE("GPL");
> Should this be "GPL v2" ?
>
>> -- 
>> 2.17.1