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
| ||
|
Date: Wed, 2 Nov 2022 20:20:13 +1000 From: James Calligeros <jcalligeros99@...il.com> To: viresh.kumar@...aro.org Cc: asahi@...ts.linux.dev, jcalligeros99@...il.com, linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, nm@...com, sboyd@...nel.org, vireshk@...nel.org Subject: Re: [PATCH v2] OPP: decouple dt properties in opp_parse_supplies() >> The opp-microwatt property was added with the intention of providing >> platforms a way to specify a precise value for the power consumption >> of a device at a given OPP to enable better energy-aware scheduling >> decisions by informing the kernel of the total static and dynamic >> power of a device at a given OPP, removing the reliance on the EM >> subsystem's often flawed estimations. This property is parsed by >> opp_parse_supplies(), which creates a hard dependency on the >> opp-microvolt property. >> >> Some platforms, such as Apple Silicon, do not describe their devices' >> voltage regulators in the DT as they cannot be controlled by the kernel >> and/or rely on opaque firmware algorithms to control their voltage and >> current characteristics at runtime. We can, however, experimentally >> determine the power consumption of a given device at a given OPP, taking >> advantage of opp-microwatt to provide EAS on such devices as was initially >> intended. >Do you supply a regulator to the OPP core for your platform ? On Apple SoCs, all the regulators are controlled transparently by the hardware/firmware. The CPUfreq driver requests a pstate as described in the OPP table by setting some bits in some registers, and the platform handles everything else. Not only is there no use for the voltage and current information from the kernel's perspective since there's nothing to control, we don't even really have a way to reliably model their behaviour. What we can do, however, is use energy counter registers in the core clusters to determine energy consumption at a given pstate and register that with the OPP core using the opp-microwatt property. Given that its stated purpose is to enable such behaviour, requiring it to be coupled to opp-microvolt is a major design deficiency. >> Fixes: 4f9a7a1dc2a2 ("OPP: Add "opp-microwatt" supporting code") >I won't call it a fix, we are trying to use this information in a >different way here, that's all. My interpretation of the commit message for 32bf8bc9a077 is that this is the way in which the property was intended to be used, and that it not working like this is a bug. Quoting the commit message, "Some of the platforms don't expose voltage information, thus it's not possible to use EM registration using DT. This patch aims to fix it." There is probably a bigger discussion to be had on whether or not parsing opp-microwatt for the purpose of EM registration should be entangled in opp_parse_supplies() at all, but that's by the by. >If there is a regulator, then we must have microvolt property. >amps/watts are optional. I noticed no adverse effects from not having opp-microvolt in my testing of this patch, possibly because the data is not used to actually puppeteer any supplies. This goes back to the greater discussion, though. If we're going to use opp_parse_supplies() to do EM registration stuff then having the check for a valid representation of an actual VRM should probably happen elsewhere, where a valid VRM is actually a hard requirement of the code being run. >> - return -EINVAL; >> - } >> } >> >> - if (unlikely(supplies == -1)) { >> - /* Initialize regulator_count */ >> - supplies = opp_table->regulator_count = 1; >> - } else if (unlikely(!supplies)) { >> - dev_err(dev, "%s: opp-microvolt wasn't expected\n", __func__); >> - return -EINVAL; >> + if (prop_mv) { >> + vcount = of_property_count_u32_elems(opp->np, name); >> + if (unlikely(supplies == -1)) >> + supplies = opp_table->regulator_count = vcount; >This is wrong. There can be one or three entries per regulator here. >Target or min/max/target. If the supplies value is -1, we can only >support one regulator, i.e. one or three entries total. Ack. - James
Powered by blists - more mailing lists