linux-kernel - Re: [PATCH v6 08/10] cpufreq: st: Provide runtime initialised driver for ST's platforms

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 10 Dec 2015 07:37:37 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	kernel@...inux.com, maxime.coquelin@...com,
	linux-pm@...r.kernel.org, rjw@...ysocki.net,
	devicetree@...r.kernel.org, ajitpal.singh@...com
Subject: Re: [PATCH v6 08/10] cpufreq: st: Provide runtime initialised driver
 for ST's platforms

On 09-12-15, 15:58, Lee Jones wrote:
> +/*
> + * Only match on "suitable for ALL versions" entries
> + *
> + * This will be used with the BIT() macro.  It sets the
> + * top bit of a 32bit value and is equal to 0x80000000.
> + */
> +#define DEFAULT_VERSION		31

Okay, I misread it in the earlier version. This looks fine.

> +static int sti_cpufreq_set_opp_info(void)
> +{

> +	sprintf(name, "pcode%d", pcode);

I would use snprintf here, in case I haven't counted the string
properly. That should guarantee that we don't access the anything else
than the 'name' array.

> +
> +	ret = dev_pm_opp_set_prop_name(dev, name);
> +	if (ret) {
> +		dev_err(dev, "Failed to set prop name\n");
> +		return ret;
> +	}
> +
> +	version[0] = BIT(major);
> +	version[1] = BIT(minor);
> +	version[2] = BIT(substrate);
> +
> +	ret = dev_pm_opp_set_supported_hw(dev, version, VERSION_ELEMENTS);
> +	if (ret) {
> +		dev_err(dev, "Failed to set supported hardware\n");
> +		return ret;
> +	}
> +
> +	dev_err(dev, "pcode: %d major: %d minor: %d substrate: %d\n",
> +		pcode, major, minor, substrate);
> +	dev_err(dev, "version[0]: %x version[1]: %x version[2]: %x\n",
> +		version[0], version[1], version[2]);

These aren't errors.. dev_info ?

> +
> +	return 0;
> +}
> +

> +static int sti_cpufreq_init(void)
> +{
> +	int ret;
> +
> +	ddata.cpu = get_cpu_device(0);
> +	if (!ddata.cpu) {
> +		dev_err(ddata.cpu, "Failed to get device for CPU0\n");
> +		goto skip_voltage_scaling;
> +	}
> +
> +	if (!of_get_property(ddata.cpu->of_node, "operating-points-v2", NULL)) {
> +		dev_err(ddata.cpu, "OPP-v2 not supported\n");

Should be:
                dev_dbg(ddata.cpu, "OPP-v2 not supported, skip voltage scaling\n");

> +		goto skip_voltage_scaling;
> +	}
> +
> +	ret = sti_cpufreq_fetch_syscon_regsiters();
> +	if (ret)
> +		goto skip_voltage_scaling;
> +
> +	ret = sti_cpufreq_set_opp_info();
> +	if (ret)

Shouldn't this be !ret ? You should have jumped on success here.

> +		goto register_cpufreq_dt;
> +
> +skip_voltage_scaling:
> +	dev_err(ddata.cpu, "Not doing voltage scaling\n");

This doesn't look nice as you are adding unnecessary jumps to just
save on a print message. And because of the earlier suggestion you can
do:

	ret = sti_cpufreq_set_opp_info();
	if (ret)
                dev_err(ddata.cpu, "Skip voltage scaling\n");

> +
> +register_cpufreq_dt:
> +	platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> +
> +	return 0;
> +}
> +module_init(sti_cpufreq_init);
> +
> +MODULE_DESCRIPTION("STMicroelectronics CPUFreq/OPP driver");
> +MODULE_AUTHOR("Ajitpal Singh <ajitpal.singh@...com>");
> +MODULE_AUTHOR("Lee Jones <lee.jones@...aro.org>");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.9.1

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/