* Re: [PATCH V2] cpufreq: reinitialize new policy min/max when writing scaling_(max|min)_freq
From: Viresh Kumar @ 2018-05-29 10:26 UTC (permalink / raw)
To: Kevin Wangtao; +Cc: rjw, linux-pm, linux-kernel, gengyanping, sunzhaosheng
In-Reply-To: <1527319008-66663-1-git-send-email-kevin.wangtao@hisilicon.com>
On 26-05-18, 15:16, Kevin Wangtao wrote:
> consider such situation, current user_policy.min is 1000000,
> current user_policy.max is 1200000, in cpufreq_set_policy,
> other driver may update policy.min to 1200000, policy.max to
> 1300000. After that, If we input "echo 1300000 > scaling_min_freq",
> then user_policy.min will be 1300000, and user_policy.max is
> still 1200000, because the input value is checked with policy.max
> not user_policy.max. if we get all related cpus offline and
> online again, it will cause cpufreq_init_policy fail because
> user_policy.min is higher than user_policy.max.
>
> The solution is when user space tries to write scaling_(max|min)_freq,
> the min/max of new_policy should be reinitialized with min/max
> of user_policy, like what cpufreq_update_policy does.
>
> Signed-off-by: Kevin Wangtao <kevin.wangtao@hisilicon.com>
> ---
> drivers/cpufreq/cpufreq.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index b79c532..82123a1 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -697,6 +697,8 @@ static ssize_t store_##file_name \
> struct cpufreq_policy new_policy; \
> \
> memcpy(&new_policy, policy, sizeof(*policy)); \
Maybe add a comment here on why this is required ?
> + new_policy.min = policy->user_policy.min; \
> + new_policy.max = policy->user_policy.max; \
> \
> ret = sscanf(buf, "%u", &new_policy.object); \
> if (ret != 1) \
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* [PATCH v3] Optimize C3 entry on Centaur CPUs
From: David Wang @ 2018-05-29 10:12 UTC (permalink / raw)
To: rjw, mingo, len.brown, pavel, tglx, hpa, x86, linux-pm,
linux-kernel
Cc: brucechang, cooperyan, qiyuanwang, benjaminpan, lukelin, timguo,
David Wang
For new Centaur CPUs the ucode will take care of the preservation of cache coherence
between CPU cores in C-states regardless of how deep the C-states are. So, it is not
necessary to flush the caches in software befor entering C3.
Signed-off-by: David Wang <davidwang@zhaoxin.com>
Changes from v2 to v3:
*1, Replace "c->x86_mask" with "c->x86_stepping".
Changes from v1 to v2:
* 1, Add some Family/Model/Stepping contrains to let this patch only apply
* to new centaur CPUs.
* 2, The arbiter disable/enable operations maybe needed for old VIA/Centaur
* platform. So, delete "flags->bm_control=0" in patch v1.
---
arch/x86/kernel/acpi/cstate.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/arch/x86/kernel/acpi/cstate.c b/arch/x86/kernel/acpi/cstate.c
index dde437f..a82fed6 100644
--- a/arch/x86/kernel/acpi/cstate.c
+++ b/arch/x86/kernel/acpi/cstate.c
@@ -51,6 +51,19 @@ void acpi_processor_power_init_bm_check(struct acpi_processor_flags *flags,
if (c->x86_vendor == X86_VENDOR_INTEL &&
(c->x86 > 0xf || (c->x86 == 6 && c->x86_model >= 0x0f)))
flags->bm_control = 0;
+
+ /*
+ * For all recent Centaur CPUs, the ucode will make sure that each
+ * core can keep cache coherence with each other while entering C3
+ * type state. So, set bm_check to 1 to indicate that the kernel
+ * need not execute a cache flush operation (WBINVD) when entering
+ * C3 type state.
+ */
+ if (c->x86_vendor == X86_VENDOR_CENTAUR) {
+ if (c->x86 > 6 || (c->x86 == 6 && c->x86_model == 0x0f &&
+ c->x86_stepping >= 0x0e))
+ flags->bm_check = 1;
+ }
}
EXPORT_SYMBOL(acpi_processor_power_init_bm_check);
--
1.9.1
^ permalink raw reply related
* [PATCH v2 9/9] PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM domains
From: Ulf Hansson @ 2018-05-29 10:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Ulf Hansson, Greg Kroah-Hartman, Jon Hunter, Geert Uytterhoeven,
Todor Tomov, Rajendra Nayak, Viresh Kumar, Vincent Guittot,
Kevin Hilman, linux-kernel, linux-arm-kernel, linux-tegra
In-Reply-To: <20180529100421.31022-1-ulf.hansson@linaro.org>
The existing dev_pm_domain_attach() function, allows a single PM domain to
be attached per device. To be able to support devices that are partitioned
across multiple PM domains, let's introduce a new interface,
dev_pm_domain_attach_by_id().
The dev_pm_domain_attach_by_id() returns a new allocated struct device with
the corresponding attached PM domain. This enables for example a driver to
operate on the new device from a power management point of view. The driver
may then also benefit from using the received device, to set up so called
device-links towards its original device. Depending on the situation, these
links may then be dynamically changed.
The new interface is typically called by drivers during their probe phase,
in case they manages devices which uses multiple PM domains. If that is the
case, the driver also becomes responsible of managing the detaching of the
PM domains, which typically should be done at the remove phase. Detaching
is done by calling the existing dev_pm_domain_detach() function and for
each of the received devices from dev_pm_domain_attach_by_id().
Note, currently its only genpd that supports multiple PM domains per
device, but dev_pm_domain_attach_by_id() can easily by extended to cover
other PM domain types, if/when needed.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
Changes in v2:
- Fixed comments from Jon. Clarified function descriptions/changelog and
return ERR_PTR(-EEXIST) in case a PM domain is already assigned.
- Fix build error when CONFIG_PM is unset.
---
drivers/base/power/common.c | 43 ++++++++++++++++++++++++++++++++++---
include/linux/pm_domain.h | 7 ++++++
2 files changed, 47 insertions(+), 3 deletions(-)
diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index 7ae62b6355b8..5e5ea0c239de 100644
--- a/drivers/base/power/common.c
+++ b/drivers/base/power/common.c
@@ -116,14 +116,51 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
}
EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
+/**
+ * dev_pm_domain_attach_by_id - Attach a device to one of its PM domains.
+ * @dev: Device to attach.
+ * @index: The index of the PM domain.
+ *
+ * As @dev may only be attached to a single PM domain, the backend PM domain
+ * provider creates a virtual device to attach instead. If attachment succeeds,
+ * the ->detach() callback in the struct dev_pm_domain are assigned by the
+ * corresponding backend attach function, as to deal with detaching of the
+ * created virtual device.
+ *
+ * This function should typically be invoked by a driver during the probe phase,
+ * in case its device requires power management through multiple PM domains. The
+ * driver may benefit from using the received device, to configure device-links
+ * towards its original device. Depending on the use-case and if needed, the
+ * links may be dynamically changed by the driver, which allows it to control
+ * the power to the PM domains independently from each other.
+ *
+ * Callers must ensure proper synchronization of this function with power
+ * management callbacks.
+ *
+ * Returns the virtual created device when successfully attached to its PM
+ * domain, NULL in case @dev don't need a PM domain, else an ERR_PTR().
+ * Note that, to detach the returned virtual device, the driver shall call
+ * dev_pm_domain_detach() on it, typically during the remove phase.
+ */
+struct device *dev_pm_domain_attach_by_id(struct device *dev,
+ unsigned int index)
+{
+ if (dev->pm_domain)
+ return ERR_PTR(-EEXIST);
+
+ return genpd_dev_pm_attach_by_id(dev, index);
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_attach_by_id);
+
/**
* dev_pm_domain_detach - Detach a device from its PM domain.
* @dev: Device to detach.
* @power_off: Used to indicate whether we should power off the device.
*
- * This functions will reverse the actions from dev_pm_domain_attach() and thus
- * try to detach the @dev from its PM domain. Typically it should be invoked
- * from subsystem level code during the remove phase.
+ * This functions will reverse the actions from dev_pm_domain_attach() and
+ * dev_pm_domain_attach_by_id(), thus it detaches @dev from its PM domain.
+ * Typically it should be invoked during the remove phase, either from
+ * subsystem level code or from drivers.
*
* Callers must ensure proper synchronization of this function with power
* management callbacks.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 82458e8e2e01..9206a4fef9ac 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -299,6 +299,8 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
#ifdef CONFIG_PM
int dev_pm_domain_attach(struct device *dev, bool power_on);
+struct device *dev_pm_domain_attach_by_id(struct device *dev,
+ unsigned int index);
void dev_pm_domain_detach(struct device *dev, bool power_off);
void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd);
#else
@@ -306,6 +308,11 @@ static inline int dev_pm_domain_attach(struct device *dev, bool power_on)
{
return 0;
}
+static inline struct device *dev_pm_domain_attach_by_id(struct device *dev,
+ unsigned int index)
+{
+ return NULL;
+}
static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {}
static inline void dev_pm_domain_set(struct device *dev,
struct dev_pm_domain *pd) {}
--
2.17.0
^ permalink raw reply related
* [PATCH v2 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Ulf Hansson @ 2018-05-29 10:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Ulf Hansson, Greg Kroah-Hartman, Jon Hunter, Geert Uytterhoeven,
Todor Tomov, Rajendra Nayak, Viresh Kumar, Vincent Guittot,
Kevin Hilman, linux-kernel, linux-arm-kernel, linux-tegra
In-Reply-To: <20180529100421.31022-1-ulf.hansson@linaro.org>
To support devices being partitioned across multiple PM domains, let's
begin with extending genpd to cope with these kind of configurations.
Therefore, add a new exported function genpd_dev_pm_attach_by_id(), which
is similar to the existing genpd_dev_pm_attach(), but with the difference
that it allows its callers to provide an index to the PM domain that it
wants to attach.
Note that, genpd_dev_pm_attach_by_id() shall only be called by the driver
core / PM core, similar to how the existing dev_pm_domain_attach() makes
use of genpd_dev_pm_attach(). However, this is implemented by following
changes on top.
Because, only one PM domain can be attached per device, genpd needs to
create a virtual device that it can attach/detach instead. More precisely,
let the new function genpd_dev_pm_attach_by_id() register a virtual struct
device via calling device_register(). Then let it attach this device to the
corresponding PM domain, rather than the one that is provided by the
caller. The actual attaching is done via re-using the existing genpd OF
functions.
At successful attachment, genpd_dev_pm_attach_by_id() returns the created
virtual device, which allows the caller to operate on it to deal with power
management. Following changes on top, provides more details in this
regards.
To deal with detaching of a PM domain for the multiple PM domains case,
let's also extend the existing genpd_dev_pm_detach() function, to cover the
cleanup of the created virtual device, via make it call device_unregister()
on it. In this way, there is no need to introduce a new function to deal
with detach for the multiple PM domain case, but instead the existing one
is re-used.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
Changes in v2:
- Fixed comments from Jon. Clarified function descriptions
and changelog.
---
drivers/base/power/domain.c | 80 +++++++++++++++++++++++++++++++++++++
include/linux/pm_domain.h | 8 ++++
2 files changed, 88 insertions(+)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 2af99bfcbe3c..2b496d79159d 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2171,6 +2171,15 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
}
EXPORT_SYMBOL_GPL(of_genpd_remove_last);
+static void genpd_release_dev(struct device *dev)
+{
+ kfree(dev);
+}
+
+static struct bus_type genpd_bus_type = {
+ .name = "genpd",
+};
+
/**
* genpd_dev_pm_detach - Detach a device from its PM domain.
* @dev: Device to detach.
@@ -2208,6 +2217,10 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
/* Check if PM domain can be powered off after removing this device. */
genpd_queue_power_off_work(pd);
+
+ /* Unregister the device if it was created by genpd. */
+ if (dev->bus == &genpd_bus_type)
+ device_unregister(dev);
}
static void genpd_dev_pm_sync(struct device *dev)
@@ -2298,6 +2311,67 @@ int genpd_dev_pm_attach(struct device *dev)
}
EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
+/**
+ * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
+ * @dev: Device to attach.
+ * @index: The index of the PM domain.
+ *
+ * Parse device's OF node to find a PM domain specifier at the provided @index.
+ * If such is found, creates a virtual device and attaches it to the retrieved
+ * pm_domain ops. To deal with detaching of the virtual device, the ->detach()
+ * callback in the struct dev_pm_domain are assigned to genpd_dev_pm_detach().
+ *
+ * Returns the created virtual device if successfully attached PM domain, NULL
+ * when the device don't need a PM domain, else an ERR_PTR() in case of
+ * failures. If a power-domain exists for the device, but cannot be found or
+ * turned on, then ERR_PTR(-EPROBE_DEFER) is returned to ensure that the device
+ * is not probed and to re-try again later.
+ */
+struct device *genpd_dev_pm_attach_by_id(struct device *dev,
+ unsigned int index)
+{
+ struct device *genpd_dev;
+ int num_domains;
+ int ret;
+
+ if (!dev->of_node)
+ return NULL;
+
+ /* Deal only with devices using multiple PM domains. */
+ num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
+ "#power-domain-cells");
+ if (num_domains < 2 || index >= num_domains)
+ return NULL;
+
+ /* Allocate and register device on the genpd bus. */
+ genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
+ if (!genpd_dev)
+ return ERR_PTR(-ENOMEM);
+
+ dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
+ genpd_dev->bus = &genpd_bus_type;
+ genpd_dev->release = genpd_release_dev;
+
+ ret = device_register(genpd_dev);
+ if (ret) {
+ kfree(genpd_dev);
+ return ERR_PTR(ret);
+ }
+
+ /* Try to attach the device to the PM domain at the specified index. */
+ ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
+ if (ret < 1) {
+ device_unregister(genpd_dev);
+ return ret ? ERR_PTR(ret) : NULL;
+ }
+
+ pm_runtime_set_active(genpd_dev);
+ pm_runtime_enable(genpd_dev);
+
+ return genpd_dev;
+}
+EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
+
static const struct of_device_id idle_state_match[] = {
{ .compatible = "domain-idle-state", },
{ }
@@ -2457,6 +2531,12 @@ unsigned int of_genpd_opp_to_performance_state(struct device *dev,
}
EXPORT_SYMBOL_GPL(of_genpd_opp_to_performance_state);
+static int __init genpd_bus_init(void)
+{
+ return bus_register(&genpd_bus_type);
+}
+core_initcall(genpd_bus_init);
+
#endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 42e0d649e653..82458e8e2e01 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -237,6 +237,8 @@ unsigned int of_genpd_opp_to_performance_state(struct device *dev,
struct device_node *opp_node);
int genpd_dev_pm_attach(struct device *dev);
+struct device *genpd_dev_pm_attach_by_id(struct device *dev,
+ unsigned int index);
#else /* !CONFIG_PM_GENERIC_DOMAINS_OF */
static inline int of_genpd_add_provider_simple(struct device_node *np,
struct generic_pm_domain *genpd)
@@ -282,6 +284,12 @@ static inline int genpd_dev_pm_attach(struct device *dev)
return 0;
}
+static inline struct device *genpd_dev_pm_attach_by_id(struct device *dev,
+ unsigned int index)
+{
+ return NULL;
+}
+
static inline
struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
{
--
2.17.0
^ permalink raw reply related
* [PATCH v2 7/9] PM / Domains: Split genpd_dev_pm_attach()
From: Ulf Hansson @ 2018-05-29 10:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Ulf Hansson, Greg Kroah-Hartman, Jon Hunter, Geert Uytterhoeven,
Todor Tomov, Rajendra Nayak, Viresh Kumar, Vincent Guittot,
Kevin Hilman, linux-kernel, linux-arm-kernel, linux-tegra
In-Reply-To: <20180529100421.31022-1-ulf.hansson@linaro.org>
To extend genpd to deal with allowing multiple PM domains per device, some
of the code in genpd_dev_pm_attach() can be re-used. Let's prepare for this
by moving some of the code into a sub-function.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/base/power/domain.c | 60 ++++++++++++++++++++-----------------
1 file changed, 33 insertions(+), 27 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 12a20f21974d..2af99bfcbe3c 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2221,38 +2221,15 @@ static void genpd_dev_pm_sync(struct device *dev)
genpd_queue_power_off_work(pd);
}
-/**
- * genpd_dev_pm_attach - Attach a device to its PM domain using DT.
- * @dev: Device to attach.
- *
- * Parse device's OF node to find a PM domain specifier. If such is found,
- * attaches the device to retrieved pm_domain ops.
- *
- * Returns 1 on successfully attached PM domain, 0 when the device don't need a
- * PM domain or when multiple power-domains exists for it, else a negative error
- * code. Note that if a power-domain exists for the device, but it cannot be
- * found or turned on, then return -EPROBE_DEFER to ensure that the device is
- * not probed and to re-try again later.
- */
-int genpd_dev_pm_attach(struct device *dev)
+static int __genpd_dev_pm_attach(struct device *dev, struct device_node *np,
+ unsigned int index)
{
struct of_phandle_args pd_args;
struct generic_pm_domain *pd;
int ret;
- if (!dev->of_node)
- return 0;
-
- /*
- * Devices with multiple PM domains must be attached separately, as we
- * can only attach one PM domain per device.
- */
- if (of_count_phandle_with_args(dev->of_node, "power-domains",
- "#power-domain-cells") != 1)
- return 0;
-
- ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
- "#power-domain-cells", 0, &pd_args);
+ ret = of_parse_phandle_with_args(np, "power-domains",
+ "#power-domain-cells", index, &pd_args);
if (ret < 0)
return ret;
@@ -2290,6 +2267,35 @@ int genpd_dev_pm_attach(struct device *dev)
return ret ? -EPROBE_DEFER : 1;
}
+
+/**
+ * genpd_dev_pm_attach - Attach a device to its PM domain using DT.
+ * @dev: Device to attach.
+ *
+ * Parse device's OF node to find a PM domain specifier. If such is found,
+ * attaches the device to retrieved pm_domain ops.
+ *
+ * Returns 1 on successfully attached PM domain, 0 when the device don't need a
+ * PM domain or when multiple power-domains exists for it, else a negative error
+ * code. Note that if a power-domain exists for the device, but it cannot be
+ * found or turned on, then return -EPROBE_DEFER to ensure that the device is
+ * not probed and to re-try again later.
+ */
+int genpd_dev_pm_attach(struct device *dev)
+{
+ if (!dev->of_node)
+ return 0;
+
+ /*
+ * Devices with multiple PM domains must be attached separately, as we
+ * can only attach one PM domain per device.
+ */
+ if (of_count_phandle_with_args(dev->of_node, "power-domains",
+ "#power-domain-cells") != 1)
+ return 0;
+
+ return __genpd_dev_pm_attach(dev, dev->of_node, 0);
+}
EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
static const struct of_device_id idle_state_match[] = {
--
2.17.0
^ permalink raw reply related
* [PATCH v2 6/9] PM / Domains: Don't attach devices in genpd with multi PM domains
From: Ulf Hansson @ 2018-05-29 10:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Ulf Hansson, Greg Kroah-Hartman, Jon Hunter, Geert Uytterhoeven,
Todor Tomov, Rajendra Nayak, Viresh Kumar, Vincent Guittot,
Kevin Hilman, linux-kernel, linux-arm-kernel, linux-tegra,
Rob Herring, devicetree
In-Reply-To: <20180529100421.31022-1-ulf.hansson@linaro.org>
The power-domain DT property may now contain a list of PM domain
specifiers, which represents that a device are partitioned across multiple
PM domains. This leads to a new situation in genpd_dev_pm_attach(), as only
one PM domain can be attached per device.
To remain things simple for the most common configuration, when a single PM
domain is used, let's treat the multiple PM domain case as being specific.
In other words, let's change genpd_dev_pm_attach() to check for multiple PM
domains and prevent it from attach any PM domain for this case. Instead,
leave this to be managed separately, from following changes to genpd.
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Suggested-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
Changes in v2:
- Minor update to changelog to mention "PM domain specifiers" rather
than a "list of phandles".
---
drivers/base/power/domain.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 7ebf7993273a..12a20f21974d 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2229,10 +2229,10 @@ static void genpd_dev_pm_sync(struct device *dev)
* attaches the device to retrieved pm_domain ops.
*
* Returns 1 on successfully attached PM domain, 0 when the device don't need a
- * PM domain or a negative error code in case of failures. Note that if a
- * power-domain exists for the device, but it cannot be found or turned on,
- * then return -EPROBE_DEFER to ensure that the device is not probed and to
- * re-try again later.
+ * PM domain or when multiple power-domains exists for it, else a negative error
+ * code. Note that if a power-domain exists for the device, but it cannot be
+ * found or turned on, then return -EPROBE_DEFER to ensure that the device is
+ * not probed and to re-try again later.
*/
int genpd_dev_pm_attach(struct device *dev)
{
@@ -2243,10 +2243,18 @@ int genpd_dev_pm_attach(struct device *dev)
if (!dev->of_node)
return 0;
+ /*
+ * Devices with multiple PM domains must be attached separately, as we
+ * can only attach one PM domain per device.
+ */
+ if (of_count_phandle_with_args(dev->of_node, "power-domains",
+ "#power-domain-cells") != 1)
+ return 0;
+
ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
"#power-domain-cells", 0, &pd_args);
if (ret < 0)
- return 0;
+ return ret;
mutex_lock(&gpd_list_lock);
pd = genpd_get_from_provider(&pd_args);
--
2.17.0
^ permalink raw reply related
* [PATCH v2 5/9] PM / Domains: dt: Allow power-domain property to be a list of specifiers
From: Ulf Hansson @ 2018-05-29 10:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Ulf Hansson, Greg Kroah-Hartman, Jon Hunter, Geert Uytterhoeven,
Todor Tomov, Rajendra Nayak, Viresh Kumar, Vincent Guittot,
Kevin Hilman, linux-kernel, linux-arm-kernel, linux-tegra,
Rob Herring, devicetree
In-Reply-To: <20180529100421.31022-1-ulf.hansson@linaro.org>
To be able to describe topologies where devices are partitioned across
multiple power domains, let's extend the power-domain property to allow
being a list of PM domain specifiers.
Cc: Rob Herring <robh+dt@kernel.org>
Cc: devicetree@vger.kernel.org
Suggested-by: Jon Hunter <jonathanh@nvidia.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
Changes in v2:
- Fixed comments from Geert. Re-worded to "PM domain specifiers" and
clarified DT example.
---
.../bindings/power/power_domain.txt | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/power/power_domain.txt b/Documentation/devicetree/bindings/power/power_domain.txt
index 4733f76cbe48..9b387f861aed 100644
--- a/Documentation/devicetree/bindings/power/power_domain.txt
+++ b/Documentation/devicetree/bindings/power/power_domain.txt
@@ -111,8 +111,8 @@ Example 3:
==PM domain consumers==
Required properties:
- - power-domains : A phandle and PM domain specifier as defined by bindings of
- the power controller specified by phandle.
+ - power-domains : A list of PM domain specifiers, as defined by bindings of
+ the power controller that is the PM domain provider.
Example:
@@ -122,9 +122,18 @@ Example:
power-domains = <&power 0>;
};
-The node above defines a typical PM domain consumer device, which is located
-inside a PM domain with index 0 of a power controller represented by a node
-with the label "power".
+ leaky-device@12351000 {
+ compatible = "foo,i-leak-current";
+ reg = <0x12351000 0x1000>;
+ power-domains = <&power 0>, <&power 1> ;
+ };
+
+The first example above defines a typical PM domain consumer device, which is
+located inside a PM domain with index 0 of a power controller represented by a
+node with the label "power".
+In the second example the consumer device are partitioned across two PM domains,
+the first with index 0 and the second with index 1, of a power controller that
+is represented by a node with the label "power.
Optional properties:
- required-opps: This contains phandle to an OPP node in another device's OPP
--
2.17.0
^ permalink raw reply related
* [PATCH v2 4/9] PM / Domains: Drop unused parameter in genpd_allocate_dev_data()
From: Ulf Hansson @ 2018-05-29 10:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Ulf Hansson, Greg Kroah-Hartman, Jon Hunter, Geert Uytterhoeven,
Todor Tomov, Rajendra Nayak, Viresh Kumar, Vincent Guittot,
Kevin Hilman, linux-kernel, linux-arm-kernel, linux-tegra
In-Reply-To: <20180529100421.31022-1-ulf.hansson@linaro.org>
The in-parameter struct generic_pm_domain *genpd to
genpd_allocate_dev_data() is unused, so let's drop it.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/base/power/domain.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index c89b0ad068b7..7ebf7993273a 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1316,7 +1316,6 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
#endif /* CONFIG_PM_SLEEP */
static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
- struct generic_pm_domain *genpd,
struct gpd_timing_data *td)
{
struct generic_pm_domain_data *gpd_data;
@@ -1385,7 +1384,7 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
return -EINVAL;
- gpd_data = genpd_alloc_dev_data(dev, genpd, td);
+ gpd_data = genpd_alloc_dev_data(dev, td);
if (IS_ERR(gpd_data))
return PTR_ERR(gpd_data);
--
2.17.0
^ permalink raw reply related
* [PATCH v2 3/9] PM / Domains: Drop genpd as in-param for pm_genpd_remove_device()
From: Ulf Hansson @ 2018-05-29 10:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Ulf Hansson, Greg Kroah-Hartman, Jon Hunter, Geert Uytterhoeven,
Todor Tomov, Rajendra Nayak, Viresh Kumar, Vincent Guittot,
Kevin Hilman, linux-kernel, linux-arm-kernel, linux-tegra,
Alex Deucher, Christian König, David Zhou
In-Reply-To: <20180529100421.31022-1-ulf.hansson@linaro.org>
There is no need to pass a genpd struct to pm_genpd_remove_device(), as we
already have the information about the PM domain (genpd) through the device
structure.
Additionally, we don't allow to remove a PM domain from a device, other
than the one it may have assigned to it, so really it does not make sense
to have a separate in-param for it.
For these reason, drop it and update the current only call to
pm_genpd_remove_device() from amdgpu_acp.
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: David (ChunMing) Zhou <David1.Zhou@amd.com>
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/base/power/domain.c | 8 ++++----
drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 2 +-
include/linux/pm_domain.h | 5 ++---
3 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 469edb401811..c89b0ad068b7 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1475,13 +1475,13 @@ static int genpd_remove_device(struct generic_pm_domain *genpd,
/**
* pm_genpd_remove_device - Remove a device from an I/O PM domain.
- * @genpd: PM domain to remove the device from.
* @dev: Device to be removed.
*/
-int pm_genpd_remove_device(struct generic_pm_domain *genpd,
- struct device *dev)
+int pm_genpd_remove_device(struct device *dev)
{
- if (!genpd || genpd != genpd_lookup_dev(dev))
+ struct generic_pm_domain *genpd = genpd_lookup_dev(dev);
+
+ if (!genpd)
return -EINVAL;
return genpd_remove_device(genpd, dev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
index a29362f9ef41..12558044acd4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
@@ -513,7 +513,7 @@ static int acp_hw_fini(void *handle)
if (adev->acp.acp_genpd) {
for (i = 0; i < ACP_DEVS ; i++) {
dev = get_mfd_cell_dev(adev->acp.acp_cell[i].name, i);
- ret = pm_genpd_remove_device(&adev->acp.acp_genpd->gpd, dev);
+ ret = pm_genpd_remove_device(dev);
/* If removal fails, dont giveup and try rest */
if (ret)
dev_err(dev, "remove dev from genpd failed\n");
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 79888fb4a81f..42e0d649e653 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -144,7 +144,7 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
}
int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev);
-int pm_genpd_remove_device(struct generic_pm_domain *genpd, struct device *dev);
+int pm_genpd_remove_device(struct device *dev);
int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
struct generic_pm_domain *new_subdomain);
int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
@@ -167,8 +167,7 @@ static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
{
return -ENOSYS;
}
-static inline int pm_genpd_remove_device(struct generic_pm_domain *genpd,
- struct device *dev)
+static inline int pm_genpd_remove_device(struct device *dev)
{
return -ENOSYS;
}
--
2.17.0
^ permalink raw reply related
* [PATCH v2 2/9] PM / Domains: Drop __pm_genpd_add_device()
From: Ulf Hansson @ 2018-05-29 10:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Ulf Hansson, Greg Kroah-Hartman, Jon Hunter, Geert Uytterhoeven,
Todor Tomov, Rajendra Nayak, Viresh Kumar, Vincent Guittot,
Kevin Hilman, linux-kernel, linux-arm-kernel, linux-tegra
In-Reply-To: <20180529100421.31022-1-ulf.hansson@linaro.org>
There are still a few non-DT existing users of genpd, however neither of
them uses __pm_genpd_add_device(), hence let's drop it.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
drivers/base/power/domain.c | 10 ++++------
include/linux/pm_domain.h | 14 +++-----------
2 files changed, 7 insertions(+), 17 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 71a1cc79fbaa..469edb401811 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1414,23 +1414,21 @@ static int genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
}
/**
- * __pm_genpd_add_device - Add a device to an I/O PM domain.
+ * pm_genpd_add_device - Add a device to an I/O PM domain.
* @genpd: PM domain to add the device to.
* @dev: Device to be added.
- * @td: Set of PM QoS timing parameters to attach to the device.
*/
-int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
- struct gpd_timing_data *td)
+int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev)
{
int ret;
mutex_lock(&gpd_list_lock);
- ret = genpd_add_device(genpd, dev, td);
+ ret = genpd_add_device(genpd, dev, NULL);
mutex_unlock(&gpd_list_lock);
return ret;
}
-EXPORT_SYMBOL_GPL(__pm_genpd_add_device);
+EXPORT_SYMBOL_GPL(pm_genpd_add_device);
static int genpd_remove_device(struct generic_pm_domain *genpd,
struct device *dev)
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index c847e9a3033d..79888fb4a81f 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -143,8 +143,7 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
return to_gpd_data(dev->power.subsys_data->domain_data);
}
-int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
- struct gpd_timing_data *td);
+int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev);
int pm_genpd_remove_device(struct generic_pm_domain *genpd, struct device *dev);
int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
struct generic_pm_domain *new_subdomain);
@@ -163,9 +162,8 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
{
return ERR_PTR(-ENOSYS);
}
-static inline int __pm_genpd_add_device(struct generic_pm_domain *genpd,
- struct device *dev,
- struct gpd_timing_data *td)
+static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
+ struct device *dev)
{
return -ENOSYS;
}
@@ -204,12 +202,6 @@ static inline int dev_pm_genpd_set_performance_state(struct device *dev,
#define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL))
#endif
-static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
- struct device *dev)
-{
- return __pm_genpd_add_device(genpd, dev, NULL);
-}
-
#ifdef CONFIG_PM_GENERIC_DOMAINS_SLEEP
void pm_genpd_syscore_poweroff(struct device *dev);
void pm_genpd_syscore_poweron(struct device *dev);
--
2.17.0
^ permalink raw reply related
* [PATCH v2 1/9] PM / Domains: Drop extern declarations of functions in pm_domain.h
From: Ulf Hansson @ 2018-05-29 10:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Ulf Hansson, Rajendra Nayak, Geert Uytterhoeven, Kevin Hilman,
Greg Kroah-Hartman, linux-kernel, Jon Hunter, Todor Tomov,
Viresh Kumar, linux-tegra, Vincent Guittot, linux-arm-kernel
In-Reply-To: <20180529100421.31022-1-ulf.hansson@linaro.org>
Using "extern" to declare a function in a public header file is somewhat
pointless, but also doesn't hurt. However, to make all the function
declarations in pm_domain.h to be consistent, let's drop the use of
"extern".
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
include/linux/pm_domain.h | 51 ++++++++++++++++++---------------------
1 file changed, 23 insertions(+), 28 deletions(-)
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 4e5764083fd8..c847e9a3033d 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -143,21 +143,17 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
return to_gpd_data(dev->power.subsys_data->domain_data);
}
-extern int __pm_genpd_add_device(struct generic_pm_domain *genpd,
- struct device *dev,
- struct gpd_timing_data *td);
-
-extern int pm_genpd_remove_device(struct generic_pm_domain *genpd,
- struct device *dev);
-extern int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
- struct generic_pm_domain *new_subdomain);
-extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
- struct generic_pm_domain *target);
-extern int pm_genpd_init(struct generic_pm_domain *genpd,
- struct dev_power_governor *gov, bool is_off);
-extern int pm_genpd_remove(struct generic_pm_domain *genpd);
-extern int dev_pm_genpd_set_performance_state(struct device *dev,
- unsigned int state);
+int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
+ struct gpd_timing_data *td);
+int pm_genpd_remove_device(struct generic_pm_domain *genpd, struct device *dev);
+int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
+ struct generic_pm_domain *new_subdomain);
+int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
+ struct generic_pm_domain *target);
+int pm_genpd_init(struct generic_pm_domain *genpd,
+ struct dev_power_governor *gov, bool is_off);
+int pm_genpd_remove(struct generic_pm_domain *genpd);
+int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state);
extern struct dev_power_governor simple_qos_governor;
extern struct dev_power_governor pm_domain_always_on_gov;
@@ -215,8 +211,8 @@ static inline int pm_genpd_add_device(struct generic_pm_domain *genpd,
}
#ifdef CONFIG_PM_GENERIC_DOMAINS_SLEEP
-extern void pm_genpd_syscore_poweroff(struct device *dev);
-extern void pm_genpd_syscore_poweron(struct device *dev);
+void pm_genpd_syscore_poweroff(struct device *dev);
+void pm_genpd_syscore_poweron(struct device *dev);
#else
static inline void pm_genpd_syscore_poweroff(struct device *dev) {}
static inline void pm_genpd_syscore_poweron(struct device *dev) {}
@@ -240,14 +236,13 @@ int of_genpd_add_provider_simple(struct device_node *np,
int of_genpd_add_provider_onecell(struct device_node *np,
struct genpd_onecell_data *data);
void of_genpd_del_provider(struct device_node *np);
-extern int of_genpd_add_device(struct of_phandle_args *args,
- struct device *dev);
-extern int of_genpd_add_subdomain(struct of_phandle_args *parent,
- struct of_phandle_args *new_subdomain);
-extern struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
-extern int of_genpd_parse_idle_states(struct device_node *dn,
- struct genpd_power_state **states, int *n);
-extern unsigned int of_genpd_opp_to_performance_state(struct device *dev,
+int of_genpd_add_device(struct of_phandle_args *args, struct device *dev);
+int of_genpd_add_subdomain(struct of_phandle_args *parent,
+ struct of_phandle_args *new_subdomain);
+struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
+int of_genpd_parse_idle_states(struct device_node *dn,
+ struct genpd_power_state **states, int *n);
+unsigned int of_genpd_opp_to_performance_state(struct device *dev,
struct device_node *opp_node);
int genpd_dev_pm_attach(struct device *dev);
@@ -304,9 +299,9 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
#endif /* CONFIG_PM_GENERIC_DOMAINS_OF */
#ifdef CONFIG_PM
-extern int dev_pm_domain_attach(struct device *dev, bool power_on);
-extern void dev_pm_domain_detach(struct device *dev, bool power_off);
-extern void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd);
+int dev_pm_domain_attach(struct device *dev, bool power_on);
+void dev_pm_domain_detach(struct device *dev, bool power_off);
+void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd);
#else
static inline int dev_pm_domain_attach(struct device *dev, bool power_on)
{
--
2.17.0
^ permalink raw reply related
* [PATCH v2 0/9] PM / Domains: Add support for multi PM domains per device
From: Ulf Hansson @ 2018-05-29 10:04 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm
Cc: Ulf Hansson, Greg Kroah-Hartman, Jon Hunter, Geert Uytterhoeven,
Todor Tomov, Rajendra Nayak, Viresh Kumar, Vincent Guittot,
Kevin Hilman, linux-kernel, linux-arm-kernel, linux-tegra
Changes in v2:
- Addressed comments from Geert around DT doc.
- Addressed comments from Jon around clarification of how to use this
and changes to returned error codes.
- Fixed build error in case CONFIG_PM was unset.
There are devices that are partitioned across multiple PM domains. Currently
these can't be supported well by the available PM infrastructures we have in
the kernel. This series is an attempt to address this.
The interesting parts happens from patch 5 an onwards, including a minor DT
update to the existing power-domain bindings, the 4 earlier are just trivial
clean-ups of some related code in genpd, which I happened to stumble over.
Some additional background:
One existing case where devices are partitioned across multiple PM domains, is
the Nvida Tegra 124/210 X-USB subsystem. A while ago Jon Hunter (Nvidia) sent a
series, trying to address these issues, however this is a new approach, while
it re-uses the same concepts from DT point of view.
The Tegra 124/210 X-USB subsystem contains of a host controller and a device
controller. Each controller have its own independent PM domain, but are being
partitioned across another shared PM domain for the USB super-speed logic.
Currently to make the drivers work, either the related PM domains needs to stay
powered on always or the PM domain topology needs to be in-correctly modelled
through sub-domains. In both cases PM domains may be powered on while they
don't need to be, so in the end this means - wasting power -.
As stated above, this series intends to address these problem from a PM
infrastructure point of view. More details are available in each changelog.
It should be noted that this series has been tested on HW, however only by using
a home-cooked test PM domain driver for genpd and together with a test driver.
This allowed me to play with PM domain (genpd), runtime PM and device links.
Any further deployment for real use cases are greatly appreciated. I am happy to
to help, if needed!
Kind regards
Ulf Hansson
Ulf Hansson (9):
PM / Domains: Drop extern declarations of functions in pm_domain.h
PM / Domains: Drop __pm_genpd_add_device()
PM / Domains: Drop genpd as in-param for pm_genpd_remove_device()
PM / Domains: Drop unused parameter in genpd_allocate_dev_data()
PM / Domains: dt: Allow power-domain property to be a list of
specifiers
PM / Domains: Don't attach devices in genpd with multi PM domains
PM / Domains: Split genpd_dev_pm_attach()
PM / Domains: Add support for multi PM domains per device to genpd
PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM
domains
.../bindings/power/power_domain.txt | 19 ++-
drivers/base/power/common.c | 39 ++++-
drivers/base/power/domain.c | 155 ++++++++++++++----
drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 2 +-
include/linux/pm_domain.h | 79 ++++-----
5 files changed, 216 insertions(+), 78 deletions(-)
--
2.17.0
^ permalink raw reply
* Re: [PATCH V4] powercap/drivers/idle_injection: Add an idle injection framework
From: Viresh Kumar @ 2018-05-29 9:31 UTC (permalink / raw)
To: Daniel Lezcano
Cc: rjw, edubezval, kevin.wangtao, leo.yan, vincent.guittot,
linux-kernel, javi.merino, rui.zhang, linux-pm, daniel.thompson
In-Reply-To: <1527241792-5860-1-git-send-email-daniel.lezcano@linaro.org>
Hi Daniel,
Thanks for yet another version :)
On 25-05-18, 11:49, Daniel Lezcano wrote:
> +++ b/drivers/powercap/idle_injection.c
> +static void idle_injection_wakeup(struct idle_injection_device *ii_dev)
> +{
> + struct idle_injection_thread *iit;
> + int cpu;
> +
> + for_each_cpu_and(cpu, ii_dev->cpumask, cpu_online_mask) {
> + iit = per_cpu_ptr(&idle_injection_thread, cpu);
> + iit->should_run = 1;
> + wake_up_process(iit->tsk);
> + }
> +}
> +
> +/**
> + * idle_injection_wakeup_fn - idle injection timer callback
> + * @timer: a hrtimer structure
> + *
> + * This function is called when the idle injection timer expires which
> + * will wake up the idle injection tasks and these ones, in turn, play
> + * idle a specified amount of time.
> + *
> + * Return: HRTIMER_NORESTART.
> + */
> +static enum hrtimer_restart idle_injection_wakeup_fn(struct hrtimer *timer)
> +{
> + struct idle_injection_device *ii_dev =
> + container_of(timer, struct idle_injection_device, timer);
> +
> + idle_injection_wakeup(ii_dev);
> +
> + return HRTIMER_NORESTART;
> +}
> +
> +/**
> + * idle_injection_fn - idle injection routine
> + * @cpu: the CPU number the tasks belongs to
> + *
> + * The idle injection routine will stay idle the specified amount of
> + * time
> + */
> +static void idle_injection_fn(unsigned int cpu)
> +{
> + struct idle_injection_device *ii_dev;
> + struct idle_injection_thread *iit;
> + int run_duration_ms, idle_duration_ms;
> +
> + ii_dev = per_cpu(idle_injection_device, cpu);
> +
> + if (WARN_ON_ONCE(!ii_dev))
Yes, this is marked as "unlikely" and is kind of not that harmful, but
I would suggest to just drop it and let the kernel crash if that
serious of a bug is present in this code where ii_dev can be NULL
here.
> + return;
> +
> + iit = per_cpu_ptr(&idle_injection_thread, cpu);
See, we don't check this one, why check only ii_dev ? :)
> +
> + /*
> + * Boolean used by the smpboot main loop and used as a
> + * flip-flop in this function
> + */
> + iit->should_run = 0;
> +
> + atomic_inc(&ii_dev->count);
> +
> + idle_duration_ms = atomic_read(&ii_dev->idle_duration_ms);
> + if (idle_duration_ms)
> + play_idle(idle_duration_ms);
> +
> + /*
> + * The last CPU waking up is in charge of setting the timer. If
> + * the CPU is hotplugged, the timer will move to another CPU
> + * (which may not belong to the same cluster) but that is not a
> + * problem as the timer will be set again by another CPU
> + * belonging to the cluster. This mechanism is self adaptive.
> + */
I am afraid that the above comment may not be completely true all the
time. For a quad-core platform, it is possible for 3 CPUs (0,1,2) to
run this function as soon as the kthread is woken up, but one of the
CPUs (3) may be stuck servicing an IRQ, Deadline or RT activity.
Because you do atomic_inc() also in this function (above) itself,
below decrement may return a true value for the CPU2 and that will
restart the hrtimer, while one of the CPUs never got a chance to
increment count in the first place.
The fix is simple though, do the increment in idle_injection_wakeup()
and things should be fine then.
> + if (!atomic_dec_and_test(&ii_dev->count))
> + return;
> +
> + run_duration_ms = atomic_read(&ii_dev->run_duration_ms);
> + if (run_duration_ms) {
> + hrtimer_start(&ii_dev->timer, ms_to_ktime(run_duration_ms),
> + HRTIMER_MODE_REL_PINNED);
> + return;
> + }
> +
> + complete(&ii_dev->stop_complete);
So you call complete() after hrtimer is potentially restarted. This
can happen if idle_injection_stop() is called right after the above
atomic_read() has finished :)
IOW, this doesn't look safe now as well.
> +}
> +/**
> + * idle_injection_stop - stops the idle injections
> + * @ii_dev: a pointer to an idle injection_device structure
> + *
> + * The function stops the idle injection by resetting the idle and
> + * running durations and wait for the threads to complete. If we are
> + * in the process of injecting an idle cycle, then this will wait the
> + * end of the cycle.
> + */
> +void idle_injection_stop(struct idle_injection_device *ii_dev)
> +{
> + pr_debug("Stopping injecting idle cycles on CPUs '%*pbl'\n",
> + cpumask_pr_args(ii_dev->cpumask));
> +
> + init_completion(&ii_dev->stop_complete);
This looks completely Borken (yeah, broken :)). complete() may be
running in parallel under spinlock and updating x->done while you just
set it to 0 here without any locking in place. init_completion()
should be used only once after ii_dev is allocated and I don't see
that being done either, so that looks incorrect as well.
> +
> + idle_injection_set_duration(ii_dev, 0, 0);
> +
> + wait_for_completion_interruptible(&ii_dev->stop_complete);
> +}
> +
> +/**
> + * idle_injection_setup - initialize the current task as a RT task
> + * @cpu: the CPU number where the kthread is running on (not used)
> + *
> + * Called one time, this function is in charge of setting the task
> + * scheduler parameters.
> + */
> +static void idle_injection_setup(unsigned int cpu)
> +{
> + struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO / 2 };
> +
> + set_freezable();
> +
> + sched_setscheduler(current, SCHED_FIFO, ¶m);
> +}
> +
> +/**
> + * idle_injection_should_run - function helper for the smpboot API
> + * @cpu: the CPU number where the kthread is running on
> + *
> + * Return: a boolean telling if the thread can run.
> + */
> +static int idle_injection_should_run(unsigned int cpu)
> +{
> + struct idle_injection_thread *iit =
> + per_cpu_ptr(&idle_injection_thread, cpu);
> +
> + return iit->should_run;
> +}
> +
> +static struct idle_injection_device *ii_dev_alloc(void)
> +{
> + struct idle_injection_device *ii_dev;
> +
> + ii_dev = kzalloc(sizeof(*ii_dev), GFP_KERNEL);
> + if (!ii_dev)
> + return NULL;
> +
> + if (!alloc_cpumask_var(&ii_dev->cpumask, GFP_KERNEL)) {
> + kfree(ii_dev);
> + return NULL;
> + }
> +
> + return ii_dev;
> +}
> +
> +static void ii_dev_free(struct idle_injection_device *ii_dev)
> +{
> + free_cpumask_var(ii_dev->cpumask);
> + kfree(ii_dev);
> +}
> +
> +/**
> + * idle_injection_register - idle injection init routine
> + * @cpumask: the list of CPUs managed by the idle injection device
> + *
> + * This is the initialization function in charge of creating the
> + * initializing of the timer and allocate the structures. It does not
> + * starts the idle injection cycles.
> + *
> + * Return: NULL if an allocation fails.
> + */
> +struct idle_injection_device *idle_injection_register(struct cpumask *cpumask)
> +{
> + struct idle_injection_device *ii_dev;
> + int cpu, cpu2;
> +
> + ii_dev = ii_dev_alloc();
> + if (!ii_dev)
> + return NULL;
> +
> + cpumask_copy(ii_dev->cpumask, cpumask);
> + hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + ii_dev->timer.function = idle_injection_wakeup_fn;
> +
> + for_each_cpu(cpu, ii_dev->cpumask) {
> +
> + if (per_cpu(idle_injection_device, cpu)) {
Maybe add unlikely here ?
> + pr_err("cpu%d is already registered\n", cpu);
> + goto out_rollback_per_cpu;
> + }
> +
> + per_cpu(idle_injection_device, cpu) = ii_dev;
> + }
> +
> + return ii_dev;
> +
> +out_rollback_per_cpu:
> + for_each_cpu(cpu2, ii_dev->cpumask) {
> + if (cpu == cpu2)
And is this really required? Perhaps this conditional is making it
worse and just setting the per-cpu variable forcefully to NULL would
be much faster :)
Maybe move this for-loop into iid_dev_free() and you can make this
look simple then.
> + break;
> +
> + per_cpu(idle_injection_device, cpu2) = NULL;
> + }
> +
> + ii_dev_free(ii_dev);
> +
> + return NULL;
> +}
--
viresh
^ permalink raw reply
* Re: [PATCH] PM / runtime: Fixup reference counting of device link suppliers at probe
From: Rafael J. Wysocki @ 2018-05-29 8:59 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Linux PM, Viresh Kumar,
Vincent Guittot, Linux Kernel Mailing List, Linux ARM
In-Reply-To: <CAPDyKFphMCQ_S5aBVToGUL=Rvd0cBQ4O3Top3AYPdM79Ak_LzA@mail.gmail.com>
On Friday, May 18, 2018 11:07:02 AM CEST Ulf Hansson wrote:
> On 18 May 2018 at 10:58, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Fri, May 18, 2018 at 10:48 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >> In the driver core, before it invokes really_probe() it runtime resumes the
> >> suppliers for the device via calling pm_runtime_get_suppliers(), which also
> >> increases the runtime PM usage count for each of the available supplier.
> >>
> >> This makes sense, as to be able to allow the consumer device to be probed
> >> by its driver. However, if the driver decides to add a new supplier link
> >> during ->probe(), hence updating the list of suppliers,
> >
> > Do any of the existing drivers do that?
>
> Yes.
>
> At least these, but possibly even more...
>
> drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> drivers/gpu/drm/tegra/dc.c
> drivers/gpu/ipu-v3/ipu-prg.c
> drivers/pci/dwc/pci-dra7xx.c
>
> >
> >> the following call to pm_runtime_put_suppliers(), invoked after really_probe()
> >> in the driver core, we get into trouble.
Patch applied, thanks!
^ permalink raw reply
* Re: [PATCH 1/4 v2] PM / suspend: Prevent might sleep splats
From: Rafael J. Wysocki @ 2018-05-29 8:49 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Rafael J. Wysocki, Linux PM, Len Brown, Pavel Machek,
Thomas Gleixner, Linux Kernel Mailing List
In-Reply-To: <20180525155440.jwqrpu3shocwcwjy@linutronix.de>
On Friday, May 25, 2018 5:54:41 PM CEST Sebastian Andrzej Siewior wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> timekeeping suspend/resume calls read_persistent_clock() which takes
> rtc_lock. That results in might sleep warnings because at that point
> we run with interrupts disabled.
>
> We cannot convert rtc_lock to a raw spinlock as that would trigger
> other might sleep warnings.
>
> As a workaround we disable the might sleep warnings by setting
> system_state to SYSTEM_SUSPEND before calling sysdev_suspend() and
> restoring it to SYSTEM_RUNNING afer sysdev_resume(). There is no lock
> contention because hibernate / suspend to RAM is single-CPU at this
> point.
>
> In s2idle's case the system_state is set to SYSTEM_SUSPEND before
> timekeeping_suspend() which is invoked by the last CPU. In the resume
> case it set back to SYSTEM_RUNNING after timekeeping_resume() which is
> invoked by the first CPU in the resume case. The other CPUs will block
> on tick_freeze_lock.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> [bigeasy: cover s2idle in tick_freeze() / tick_unfreeze()]
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> v1…v2: Moved the s2idle case into tick_freeze()/unfreeze() as suggested
> by Rafael (tglx was okay with this).
>
> include/linux/kernel.h | 1 +
> kernel/power/hibernate.c | 7 +++++++
> kernel/power/suspend.c | 4 ++++
> kernel/time/tick-common.c | 2 ++
> 4 files changed, 14 insertions(+)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 6a1eb0b0aad9..7aed92624531 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -542,6 +542,7 @@ extern enum system_states {
> SYSTEM_HALT,
> SYSTEM_POWER_OFF,
> SYSTEM_RESTART,
> + SYSTEM_SUSPEND,
> } system_state;
>
> /* This cannot be an enum because some may be used in assembly source. */
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 5454cc639a8d..9c85c7822383 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -287,6 +287,8 @@ static int create_image(int platform_mode)
>
> local_irq_disable();
>
> + system_state = SYSTEM_SUSPEND;
> +
> error = syscore_suspend();
> if (error) {
> pr_err("Some system devices failed to power down, aborting hibernation\n");
> @@ -317,6 +319,7 @@ static int create_image(int platform_mode)
> syscore_resume();
>
> Enable_irqs:
> + system_state = SYSTEM_RUNNING;
> local_irq_enable();
>
> Enable_cpus:
> @@ -445,6 +448,7 @@ static int resume_target_kernel(bool platform_mode)
> goto Enable_cpus;
>
> local_irq_disable();
> + system_state = SYSTEM_SUSPEND;
>
> error = syscore_suspend();
> if (error)
> @@ -478,6 +482,7 @@ static int resume_target_kernel(bool platform_mode)
> syscore_resume();
>
> Enable_irqs:
> + system_state = SYSTEM_RUNNING;
> local_irq_enable();
>
> Enable_cpus:
> @@ -563,6 +568,7 @@ int hibernation_platform_enter(void)
> goto Enable_cpus;
>
> local_irq_disable();
> + system_state = SYSTEM_SUSPEND;
> syscore_suspend();
> if (pm_wakeup_pending()) {
> error = -EAGAIN;
> @@ -575,6 +581,7 @@ int hibernation_platform_enter(void)
>
> Power_up:
> syscore_resume();
> + system_state = SYSTEM_RUNNING;
> local_irq_enable();
>
> Enable_cpus:
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 4c10be0f4843..5149c77506b3 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -428,6 +428,8 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
> arch_suspend_disable_irqs();
> BUG_ON(!irqs_disabled());
>
> + system_state = SYSTEM_SUSPEND;
> +
> error = syscore_suspend();
> if (!error) {
> *wakeup = pm_wakeup_pending();
> @@ -443,6 +445,8 @@ static int suspend_enter(suspend_state_t state, bool *wakeup)
> syscore_resume();
> }
>
> + system_state = SYSTEM_RUNNING;
> +
> arch_suspend_enable_irqs();
> BUG_ON(irqs_disabled());
>
> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
> index 49edc1c4f3e6..14de3727b18e 100644
> --- a/kernel/time/tick-common.c
> +++ b/kernel/time/tick-common.c
> @@ -490,6 +490,7 @@ void tick_freeze(void)
> if (tick_freeze_depth == num_online_cpus()) {
> trace_suspend_resume(TPS("timekeeping_freeze"),
> smp_processor_id(), true);
> + system_state = SYSTEM_SUSPEND;
> timekeeping_suspend();
> } else {
> tick_suspend_local();
> @@ -513,6 +514,7 @@ void tick_unfreeze(void)
>
> if (tick_freeze_depth == num_online_cpus()) {
> timekeeping_resume();
> + system_state = SYSTEM_RUNNING;
> trace_suspend_resume(TPS("timekeeping_freeze"),
> smp_processor_id(), false);
> } else {
>
Applied along with the rest of the series, thanks!
^ permalink raw reply
* Re: [PATCH] PM / hibernate: Fix oops at snapshot_write().
From: Rafael J. Wysocki @ 2018-05-29 8:48 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: Pavel Machek, Len Brown, linux-pm, linux-kernel, syzbot
In-Reply-To: <1527296376-3540-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp>
On Saturday, May 26, 2018 2:59:36 AM CEST Tetsuo Handa wrote:
> syzbot is reporting NULL pointer dereference at snapshot_write() [1].
> This is because data->handle is zero-cleared by ioctl(SNAPSHOT_FREE).
> Fix this by checking data_of(data->handle) != NULL before using it.
>
> [1] https://syzkaller.appspot.com/bug?id=828a3c71bd344a6de8b6a31233d51a72099f27fd
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Reported-by: syzbot <syzbot+ae590932da6e45d6564d@syzkaller.appspotmail.com>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> ---
> kernel/power/user.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/kernel/power/user.c b/kernel/power/user.c
> index 75c959d..abd2255 100644
> --- a/kernel/power/user.c
> +++ b/kernel/power/user.c
> @@ -186,6 +186,11 @@ static ssize_t snapshot_write(struct file *filp, const char __user *buf,
> res = PAGE_SIZE - pg_offp;
> }
>
> + if (!data_of(data->handle)) {
> + res = -EINVAL;
> + goto unlock;
> + }
> +
> res = simple_write_to_buffer(data_of(data->handle), res, &pg_offp,
> buf, count);
> if (res > 0)
>
Applied, thanks!
^ permalink raw reply
* Re: [PATCH] cpufreq: reinitialize new policy min/max when writing scaling_(max|min)_freq
From: Rafael J. Wysocki @ 2018-05-29 8:47 UTC (permalink / raw)
To: Wangtao (Kevin, Kirin)
Cc: Rafael J. Wysocki, Viresh Kumar, Linux PM,
Linux Kernel Mailing List, gengyanping, sunzhaosheng
In-Reply-To: <c6d1e102-bfdb-572c-2045-bf669ee9d674@hisilicon.com>
On Saturday, May 26, 2018 8:50:46 AM CEST Wangtao (Kevin, Kirin) wrote:
>
> 在 2018/5/24 15:45, Rafael J. Wysocki 写道:
> > On Thu, May 24, 2018 at 8:43 AM, Kevin Wangtao
> > <kevin.wangtao@hisilicon.com> wrote:
> >> consider such situation, current user_policy.min is 1000000,
> >> current user_policy.max is 1200000, in cpufreq_set_policy,
> >> other driver may update policy.min to 1200000, policy.max to
> >> 1300000. After that, If we input "echo 1300000 > scaling_min_freq",
> >> then user_policy.min will be 1300000, and user_policy.max is
> >> still 1200000, because the input value is checked with policy.max
> >> not user_policy.max. if we get all related cpus offline and
> >> online again, it will cause cpufreq_init_policy fail because
> >> user_policy.min is higher than user_policy.max.
> >
> > How do you reproduce this, exactly?
>
> I can also reproduce this issue with upstream code, write max frequency to scaling_max_freq
> and scaling_min_freq, run benchmark to let cpu cooling take effect to clip freq, then write
> the cliped freq to scaling_max_freq, thus user_policy.min is still max frequency but user_policy.max
> is cliped freq which is lower than max frequency.
OK, this is a bit more convincing.
It looks like bad interaction between cpufreq_update_policy() and updates of
the limits via sysfs.
^ permalink raw reply
* Re: [PATCH] cpufreq: reinitialize new policy min/max when writing scaling_(max|min)_freq
From: Rafael J. Wysocki @ 2018-05-29 8:45 UTC (permalink / raw)
To: Wangtao (Kevin, Kirin)
Cc: Rafael J. Wysocki, Viresh Kumar, Linux PM,
Linux Kernel Mailing List, gengyanping, sunzhaosheng
In-Reply-To: <7700fad7-211d-d8b2-e4ee-975c1cf1f9dc@hisilicon.com>
On Friday, May 25, 2018 4:54:04 AM CEST Wangtao (Kevin, Kirin) wrote:
>
> 在 2018/5/24 15:45, Rafael J. Wysocki 写道:
> > On Thu, May 24, 2018 at 8:43 AM, Kevin Wangtao
> > <kevin.wangtao@hisilicon.com> wrote:
> >> consider such situation, current user_policy.min is 1000000,
> >> current user_policy.max is 1200000, in cpufreq_set_policy,
> >> other driver may update policy.min to 1200000, policy.max to
> >> 1300000. After that, If we input "echo 1300000 > scaling_min_freq",
> >> then user_policy.min will be 1300000, and user_policy.max is
> >> still 1200000, because the input value is checked with policy.max
> >> not user_policy.max. if we get all related cpus offline and
> >> online again, it will cause cpufreq_init_policy fail because
> >> user_policy.min is higher than user_policy.max.
> >
> > How do you reproduce this, exactly?
>
> I write a driver register CPUFREQ_POLICY_NOTIFIER, and when event is CPUFREQ_ADJUST,
> it will modify policy's min/max according to some conditions, I test it with writing
> scaling_(max|min)_freq to traverse all frequencies repeatly, and also repeat hotplug
> as background.
You are expected to use cpufreq_update_policy() to update the limits in policy
notifiers. Do you use it in your driver?
^ permalink raw reply
* Re: [PATCH] PM / runtime: Drop usage count for suppliers at device link removal
From: Rafael J. Wysocki @ 2018-05-29 8:40 UTC (permalink / raw)
To: Ulf Hansson
Cc: Greg Kroah-Hartman, Linux PM, Viresh Kumar, Vincent Guittot,
Todor Tomov, Rajendra Nayak, Jon Hunter,
Linux Kernel Mailing List, Linux ARM
In-Reply-To: <CAJZ5v0gGLvKS6yDuBokJzm2gAhM=VMSaz5frvWq=eRPkAD-z6g@mail.gmail.com>
On Sunday, May 27, 2018 12:18:05 PM CEST Rafael J. Wysocki wrote:
> On Thu, May 24, 2018 at 10:33 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > In the case consumer device is runtime resumed, while the link to the
> > supplier is removed, the earlier call to pm_runtime_get_sync() made from
> > rpm_get_suppliers() does not get properly balanced with a corresponding
> > call to pm_runtime_put(). This leads to that suppliers remains to be
> > runtime resumed forever, while they don't need to.
> >
> > Let's fix the behaviour by calling rpm_put_suppliers() when dropping a
> > device link. Not that, since rpm_put_suppliers() checks the
> > link->rpm_active flag, we can correctly avoid to call pm_runtime_put() in
> > cases when we shouldn't.
> >
> > Reported-by: Todor Tomov <todor.tomov@linaro.org>
> > Fixes: 21d5c57b3726 ("PM / runtime: Use device links")
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Rafael, I am not sure if this is safe from locking point of view. The device
> > link write lock has been taken when pm_runtime_drop_link() is called, hence I
> > assume calling rpm_put_suppliers() should be fine!? If not, can you please
> > advise how to change?
>
> Holding the lock should be sufficient for the list to be stable, so
> AFAICS it is OK.
So the patch has been applied, thanks!
^ permalink raw reply
* Re: [PATCH 1/2] kernel/SRCU: provide a static initializer
From: Rafael J. Wysocki @ 2018-05-29 8:09 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Linux PM, Linux Kernel Mailing List, Rafael J. Wysocki,
Viresh Kumar, Paul E. McKenney, Thomas Gleixner
In-Reply-To: <20180525101958.13277-2-bigeasy@linutronix.de>
On Fri, May 25, 2018 at 12:19 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> There are macros for static initializer for the three out of four
> possible notifier types, that are:
> ATOMIC_NOTIFIER_HEAD()
> BLOCKING_NOTIFIER_HEAD()
> RAW_NOTIFIER_HEAD()
>
> This patch provides a static initilizer for the forth type to make it
> complete.
>
> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
I cannot apply this without an ACK from Paul.
> ---
> include/linux/notifier.h | 34 +++++++++++++++++++++++++++++-----
> include/linux/srcutiny.h | 6 +++---
> include/linux/srcutree.h | 6 +++---
> 3 files changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/notifier.h b/include/linux/notifier.h
> index 6d731110e0db..f35c7bf76143 100644
> --- a/include/linux/notifier.h
> +++ b/include/linux/notifier.h
> @@ -43,9 +43,7 @@
> * in srcu_notifier_call_chain(): no cache bounces and no memory barriers.
> * As compensation, srcu_notifier_chain_unregister() is rather expensive.
> * SRCU notifier chains should be used when the chain will be called very
> - * often but notifier_blocks will seldom be removed. Also, SRCU notifier
> - * chains are slightly more difficult to use because they require special
> - * runtime initialization.
> + * often but notifier_blocks will seldom be removed.
> */
>
> struct notifier_block;
> @@ -91,7 +89,7 @@ struct srcu_notifier_head {
> (name)->head = NULL; \
> } while (0)
>
> -/* srcu_notifier_heads must be initialized and cleaned up dynamically */
> +/* srcu_notifier_heads must be cleaned up dynamically */
> extern void srcu_init_notifier_head(struct srcu_notifier_head *nh);
> #define srcu_cleanup_notifier_head(name) \
> cleanup_srcu_struct(&(name)->srcu);
> @@ -104,7 +102,13 @@ extern void srcu_init_notifier_head(struct srcu_notifier_head *nh);
> .head = NULL }
> #define RAW_NOTIFIER_INIT(name) { \
> .head = NULL }
> -/* srcu_notifier_heads cannot be initialized statically */
> +
> +#define SRCU_NOTIFIER_INIT(name, pcpu) \
> + { \
> + .mutex = __MUTEX_INITIALIZER(name.mutex), \
> + .head = NULL, \
> + .srcu = __SRCU_STRUCT_INIT(name.srcu, pcpu), \
> + }
>
> #define ATOMIC_NOTIFIER_HEAD(name) \
> struct atomic_notifier_head name = \
> @@ -116,6 +120,26 @@ extern void srcu_init_notifier_head(struct srcu_notifier_head *nh);
> struct raw_notifier_head name = \
> RAW_NOTIFIER_INIT(name)
>
> +#ifdef CONFIG_TREE_SRCU
> +#define _SRCU_NOTIFIER_HEAD(name, mod) \
> + static DEFINE_PER_CPU(struct srcu_data, \
> + name##_head_srcu_data); \
> + mod struct srcu_notifier_head name = \
> + SRCU_NOTIFIER_INIT(name, name##_head_srcu_data)
> +
> +#else
> +#define _SRCU_NOTIFIER_HEAD(name, mod) \
> + mod struct srcu_notifier_head name = \
> + SRCU_NOTIFIER_INIT(name, name)
> +
> +#endif
> +
> +#define SRCU_NOTIFIER_HEAD(name) \
> + _SRCU_NOTIFIER_HEAD(name, /* not static */)
> +
> +#define SRCU_NOTIFIER_HEAD_STATIC(name) \
> + _SRCU_NOTIFIER_HEAD(name, static)
> +
> #ifdef __KERNEL__
>
> extern int atomic_notifier_chain_register(struct atomic_notifier_head *nh,
> diff --git a/include/linux/srcutiny.h b/include/linux/srcutiny.h
> index 261471f407a5..f41d2fb09f87 100644
> --- a/include/linux/srcutiny.h
> +++ b/include/linux/srcutiny.h
> @@ -43,7 +43,7 @@ struct srcu_struct {
>
> void srcu_drive_gp(struct work_struct *wp);
>
> -#define __SRCU_STRUCT_INIT(name) \
> +#define __SRCU_STRUCT_INIT(name, __ignored) \
> { \
> .srcu_wq = __SWAIT_QUEUE_HEAD_INITIALIZER(name.srcu_wq), \
> .srcu_cb_tail = &name.srcu_cb_head, \
> @@ -56,9 +56,9 @@ void srcu_drive_gp(struct work_struct *wp);
> * Tree SRCU, which needs some per-CPU data.
> */
> #define DEFINE_SRCU(name) \
> - struct srcu_struct name = __SRCU_STRUCT_INIT(name)
> + struct srcu_struct name = __SRCU_STRUCT_INIT(name, name)
> #define DEFINE_STATIC_SRCU(name) \
> - static struct srcu_struct name = __SRCU_STRUCT_INIT(name)
> + static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name)
>
> void synchronize_srcu(struct srcu_struct *sp);
>
> diff --git a/include/linux/srcutree.h b/include/linux/srcutree.h
> index 4eda108abee0..745d4ca4dd50 100644
> --- a/include/linux/srcutree.h
> +++ b/include/linux/srcutree.h
> @@ -104,9 +104,9 @@ struct srcu_struct {
> #define SRCU_STATE_SCAN1 1
> #define SRCU_STATE_SCAN2 2
>
> -#define __SRCU_STRUCT_INIT(name) \
> +#define __SRCU_STRUCT_INIT(name, pcpu_name) \
> { \
> - .sda = &name##_srcu_data, \
> + .sda = &pcpu_name, \
> .lock = __SPIN_LOCK_UNLOCKED(name.lock), \
> .srcu_gp_seq_needed = 0 - 1, \
> __SRCU_DEP_MAP_INIT(name) \
> @@ -133,7 +133,7 @@ struct srcu_struct {
> */
> #define __DEFINE_SRCU(name, is_static) \
> static DEFINE_PER_CPU(struct srcu_data, name##_srcu_data);\
> - is_static struct srcu_struct name = __SRCU_STRUCT_INIT(name)
> + is_static struct srcu_struct name = __SRCU_STRUCT_INIT(name, name##_srcu_data)
> #define DEFINE_SRCU(name) __DEFINE_SRCU(name, /* not static */)
> #define DEFINE_STATIC_SRCU(name) __DEFINE_SRCU(name, static)
>
> --
> 2.17.0
>
^ permalink raw reply
* RE: [RFC] platform: detach from PM domains on shutdown
From: Peng Fan @ 2018-05-29 7:52 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Ulf Hansson, Rafael J. Wysocki, Fabio Estevam,
Greg Kroah-Hartman, Linux Kernel Mailing List, Linux PM,
dl-linux-imx
In-Reply-To: <3452357.3GIrXi3WAv@aspire.rjw.lan>
> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: 2018年5月28日 16:32
> To: Peng Fan <peng.fan@nxp.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>; Ulf Hansson
> <ulf.hansson@linaro.org>; Rafael J. Wysocki <rafael.j.wysocki@intel.com>;
> Fabio Estevam <fabio.estevam@nxp.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; Linux PM <linux-pm@vger.kernel.org>;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [RFC] platform: detach from PM domains on shutdown
>
> On Monday, May 28, 2018 10:01:09 AM CEST Peng Fan wrote:
> > Hi, Rafael & Uffe
> >
> > > -----Original Message-----
> > > From: Peng Fan
> > > Sent: 2018年5月18日 16:53
> > > To: Rafael J. Wysocki <rjw@rjwysocki.net>
> > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Ulf Hansson
> > > <ulf.hansson@linaro.org>; Rafael J. Wysocki
> > > <rafael.j.wysocki@intel.com>; Fabio Estevam <fabio.estevam@nxp.com>;
> > > Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Linux Kernel
> > > Mailing List <linux-kernel@vger.kernel.org>; Linux PM
> > > <linux-pm@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>
> > > Subject: RE: [RFC] platform: detach from PM domains on shutdown
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > > > Sent: 2018年5月18日 15:55
> > > > To: Peng Fan <peng.fan@nxp.com>
> > > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Ulf Hansson
> > > > <ulf.hansson@linaro.org>; Rafael J. Wysocki
> > > > <rafael.j.wysocki@intel.com>; Fabio Estevam
> > > > <fabio.estevam@nxp.com>; Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org>; Linux Kernel Mailing List
> > > > <linux-kernel@vger.kernel.org>; Linux PM
> > > > <linux-pm@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>
> > > > Subject: Re: [RFC] platform: detach from PM domains on shutdown
> > > >
> > > > On Thursday, May 17, 2018 2:37:31 PM CEST Peng Fan wrote:
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On
> > > > > > Behalf Of Rafael J. Wysocki
> > > > > > Sent: 2018年5月17日 16:01
> > > > > > To: Peng Fan <peng.fan@nxp.com>
> > > > > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Ulf Hansson
> > > > > > <ulf.hansson@linaro.org>; Rafael J. Wysocki
> > > > > > <rafael.j.wysocki@intel.com>; Fabio Estevam
> > > > > > <fabio.estevam@nxp.com>; Greg Kroah-Hartman
> > > > > > <gregkh@linuxfoundation.org>; Linux Kernel Mailing List
> > > > > > <linux-kernel@vger.kernel.org>; Linux PM
> > > > > > <linux-pm@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>
> > > > > > Subject: Re: [RFC] platform: detach from PM domains on
> > > > > > shutdown
> > > > > >
> > > > > > On Thu, May 17, 2018 at 4:33 AM, Peng Fan <peng.fan@nxp.com>
> wrote:
> > > > > > >
> > > > > > >
> > > > > > >> -----Original Message-----
> > > > > > >> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On
> > > > > > >> Behalf Of Rafael J. Wysocki
> > > > > > >> Sent: 2018年5月17日 5:35
> > > > > > >> To: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > > >> Cc: Peng Fan <peng.fan@nxp.com>; Rafael J. Wysocki
> > > > > > >> <rafael.j.wysocki@intel.com>; Fabio Estevam
> > > > > > >> <fabio.estevam@nxp.com>; Greg Kroah-Hartman
> > > > > > >> <gregkh@linuxfoundation.org>; Linux Kernel Mailing List
> > > > > > >> <linux-kernel@vger.kernel.org>; Linux PM
> > > > > > >> <linux-pm@vger.kernel.org>; dl-linux-imx
> > > > > > >> <linux-imx@nxp.com>
> > > > > > >> Subject: Re: [RFC] platform: detach from PM domains on
> > > > > > >> shutdown
> > > > > > >>
> > > > > > >> On Wed, May 16, 2018 at 11:52 AM, Ulf Hansson
> > > > > > >> <ulf.hansson@linaro.org>
> > > > > > >> wrote:
> > > > > > >> > On 15 May 2018 at 11:01, Peng Fan <peng.fan@nxp.com> wrote:
> > > > > > >> >> When reboot Linux, the PM domains attached to a device
> > > > > > >> >> are not shutdown. To SoCs which relys on reset the whole
> > > > > > >> >> SoC, there is no need to shutdown PM domains, but to
> > > > > > >> >> Linux running in a virtual machine with devices
> > > > > > >> >> pass-through, we could not reset the
> > > > whole SoC.
> > > > > > >> >> Currently we need Linux to shutdown its PM domains when
> reboot.
> > > > > > >> >
> > > > > > >> > I am not sure I understand exactly why the PM domain
> > > > > > >> > needs to be shutdown for these cases, could you please elaborate
> a bit on that.
> > > > > > >> >
> > > > > > >> > BTW, what platform are you running on and also what PM
> > > > > > >> > domains are being
> > > > > > >> used?
> > > > > > >> >
> > > > > > >> > Anyway, it seems like there may be need for certain
> > > > > > >> > cases, but certainly not all - especially since it may
> > > > > > >> > slow down the shutdown process, when not needed.
> > > > > > >> >
> > > > > > >> > Can we make this runtime configurable, via sysfs or
> > > > > > >> > whatever that makes
> > > > > > >> sense!?
> > > > > > >> >
> > > > > > >> >>
> > > > > > >> >> commit 2d30bb0b3889 ("platform: Do not detach from PM
> > > > > > >> >> domains on shutdown"), removes what this patch tries to
> > > > > > >> >> add, because of a
> > > > warning.
> > > > > > >> >> commit e79aee49bcf9 ("PM: Avoid false-positive warnings
> > > > > > >> >> in
> > > > > > >> >> dev_pm_domain_set()") already fixes the false alarm warning.
> > > > > > >> >> So let's detach the power domain to shutdown PM domains
> > > > > > >> >> after driver
> > > > > > shutdown.
> > > > > > >> >>
> > > > > > >> >> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > > >> >> ---
> > > > > > >> >>
> > > > > > >> >> I do not find a better place to shutdown power domain
> > > > > > >> >> when reboot Linux, so add back the line that commit
> > > > > > >> >> 2d30bb0b3889 removes, because it is a false alarm
> > > > > > >> >> warning as commit
> > > > > > >> >> e79aee49bcf9
> > > > describes.
> > > > > > >> >>
> > > > > > >> >> drivers/base/platform.c | 1 +
> > > > > > >> >> 1 file changed, 1 insertion(+)
> > > > > > >> >>
> > > > > > >> >> diff --git a/drivers/base/platform.c
> > > > > > >> >> b/drivers/base/platform.c index
> > > > > > >> >> 8075ddc70a17..a5929f24dc3c
> > > > > > >> >> 100644
> > > > > > >> >> --- a/drivers/base/platform.c
> > > > > > >> >> +++ b/drivers/base/platform.c
> > > > > > >> >> @@ -616,6 +616,7 @@ static void
> > > > > > >> >> platform_drv_shutdown(struct device
> > > > > > >> >> *_dev)
> > > > > > >> >>
> > > > > > >> >> if (drv->shutdown)
> > > > > > >> >> drv->shutdown(dev);
> > > > > > >> >> + dev_pm_domain_detach(_dev, true);
> > > > > > >> >
> > > > > > >> > This would somewhat work, but only for platform devices.
> > > > > > >> > To make this fully work, we need to call
> > > > > > >> > dev_pm_domain_detach() from amba, spi, etc as well.
> > > > > > >> >
> > > > > > >> > Perhaps another option to manage this more generally, an
> > > > > > >> > without having detach devices, could be to extend the
> > > > > > >> > struct dev_pm_domain with a new callback, "->shutdown()"
> > > > > > >> > and then make the driver core call it from device_shutdown().
> > > > > > >>
> > > > > > >> I'm sensing a possible ordering slippery slope with this
> > > > > > >> (it will only work if all of the drivers/bus types etc do
> > > > > > >> the right thing in their
> > > > > > >> ->shutdown callbacks so nothing depends on the domain going
> > > forward).
> > > > > > >>
> > > > > > >> > Typically, for genpd, I would probably count the number
> > > > > > >> > of calls being made to ->shutdown() per PM domain, then
> > > > > > >> > when it reaches the number of attached devices to it, allow to
> power off it.
> > > > > > >> >
> > > > > > >> > Let's see what Rafael thinks about it.
> > > > > > >>
> > > > > > >> I'm not sure about the use case. The hypervisor should be
> > > > > > >> able to take care of turning power domains off on the
> > > > > > >> client OS reboot in theory. If the client OS leaving the
> > > > > > >> hypervisor needs to worry about what state it leaves
> > > > > > >> behind, the design of the hypervisor is sort of
> > > > > > questionable IMO.
> > > > > > >
> > > > > > > This is valid concern. But moving the power domain logic
> > > > > > > into hypervisor mostly micro-kernel design will introduce
> > > > > > > more complexity and
> > > > > > make certification harder.
> > > > > > > Currently, Let Linux shutdown it's power domain is the
> > > > > > > easiest way to me and make things work well after reboot.
> > > > > >
> > > > > > Well, to put it bluntly, if your hypervisor depends on the
> > > > > > guest to do the right thing on exit, it doesn't do its job. I
> > > > > > wouldn't have certified it for you if that was my decision.
> > > > >
> > > > > It is guest os not work well after guest os reboot. The
> > > > > hypervisor is not
> > > > affected.
> > > > >
> > > > > Thinking another case without hypervisor, M4 core run RTOS, A35
> > > > > Core run Linux, when Linux rebooting, RTOS should not be
> > > > > affected. After Linux reboot itself, because its power domain is
> > > > > not paired with
> > > > open/shutdown, some devices not function well.
> > > >
> > > > The question boils down to whether or not devices should be
> > > > detached from PM domains on shutdown IMO.
> > > >
> > > > They are detached from PM domains on driver removal, so I guess
> > > > one answer is "yes, in analogy with that". However, the point
> > > > about performace brought up by Ulf seems to be valid too.
> > > >
> > > > In any case, the change should be made for all of the bus types
> > > > using PM domains, not just one.
> > >
> > > Understand, it will increase shutdown time. How about shutdown the
> > > power domain in platform_driver->shutdown, let the driver handle
> > > it's power domain sthudown by itself?
> > > Then no need common framework change.
> >
> > Do you have more suggestions on how to handle this the power domain
> shutdown?
>
> I think you could add a platform syscore_shutdown hook to turn all power
> domains off.
Thanks, Rafael. This is simple enough to me.
Thanks,
Peng.
^ permalink raw reply
* Re: [RFC/RFT] [PATCH v2 2/6] cpufreq: intel_pstate: Add HWP boost utility functions
From: Rafael J. Wysocki @ 2018-05-29 7:47 UTC (permalink / raw)
To: Srinivas Pandruvada
Cc: Len Brown, Rafael J. Wysocki, Peter Zijlstra, Mel Gorman,
Linux PM, Linux Kernel Mailing List, Juri Lelli, Viresh Kumar
In-Reply-To: <20180524014738.52924-3-srinivas.pandruvada@linux.intel.com>
On Thu, May 24, 2018 at 3:47 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> Added two utility functions to HWP boost up gradually and boost down to
> the default cached HWP request values.
>
> Boost up:
> Boost up updates HWP request minimum value in steps. This minimum value
> can reach upto at HWP request maximum values depends on how frequently,
> the IOWAIT flag is set. At max, boost up will take three steps to reach
> the maximum, depending on the current HWP request levels and HWP
> capabilities. For example, if the current settings are:
> If P0 (Turbo max) = P1 (Guaranteed max) = min
> No boost at all.
> If P0 (Turbo max) > P1 (Guaranteed max) = min
> Should result in one level boost only for P0.
> If P0 (Turbo max) = P1 (Guaranteed max) > min
> Should result in two level boost:
> (min + p1)/2 and P1.
> If P0 (Turbo max) > P1 (Guaranteed max) > min
> Should result in three level boost:
> (min + p1)/2, P1 and P0.
> We don't set any level between P0 and P1 as there is no guarantee that
> they will be honored.
>
> Boost down:
> After the system is idle for hold time of 3ms, the HWP request is reset
> to the default cached value from HWP init or user modified one via sysfs.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> drivers/cpufreq/intel_pstate.c | 74 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 74 insertions(+)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index baed29c768e7..6ad46e07cad6 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -223,6 +223,7 @@ struct global_params {
> * operation
> * @hwp_req_cached: Cached value of the last HWP Request MSR
> * @hwp_cap_cached: Cached value of the last HWP Capabilities MSR
> + * @hwp_boost_min: Last HWP boosted min performance
> *
> * This structure stores per CPU instance data for all CPUs.
> */
> @@ -257,6 +258,7 @@ struct cpudata {
> s16 epp_saved;
> u64 hwp_req_cached;
> u64 hwp_cap_cached;
> + int hwp_boost_min;
> };
>
> static struct cpudata **all_cpu_data;
> @@ -1387,6 +1389,78 @@ static void intel_pstate_get_cpu_pstates(struct cpudata *cpu)
> intel_pstate_set_min_pstate(cpu);
> }
>
> +/*
> + * Long hold time will keep high perf limits for long time,
> + * which negatively impacts perf/watt for some workloads,
> + * like specpower. 3ms is based on experiements on some
> + * workoads.
> + */
> +static int hwp_boost_hold_time_ms = 3;
> +
> +static inline void intel_pstate_hwp_boost_up(struct cpudata *cpu)
> +{
> + u64 hwp_req = READ_ONCE(cpu->hwp_req_cached);
If user space updates the limits after this read, our decision below
may be based on a stale value, may it not?
> + int max_limit = (hwp_req & 0xff00) >> 8;
> + int min_limit = (hwp_req & 0xff);
> + int boost_level1;
> +
> + /*
> + * Cases to consider (User changes via sysfs or boot time):
> + * If, P0 (Turbo max) = P1 (Guaranteed max) = min:
> + * No boost, return.
> + * If, P0 (Turbo max) > P1 (Guaranteed max) = min:
> + * Should result in one level boost only for P0.
> + * If, P0 (Turbo max) = P1 (Guaranteed max) > min:
> + * Should result in two level boost:
> + * (min + p1)/2 and P1.
> + * If, P0 (Turbo max) > P1 (Guaranteed max) > min:
> + * Should result in three level boost:
> + * (min + p1)/2, P1 and P0.
> + */
> +
> + /* If max and min are equal or already at max, nothing to boost */
> + if (max_limit == min_limit || cpu->hwp_boost_min >= max_limit)
> + return;
> +
> + if (!cpu->hwp_boost_min)
> + cpu->hwp_boost_min = min_limit;
> +
> + /* level at half way mark between min and guranteed */
> + boost_level1 = (HWP_GUARANTEED_PERF(cpu->hwp_cap_cached) + min_limit) >> 1;
> +
> + if (cpu->hwp_boost_min < boost_level1)
> + cpu->hwp_boost_min = boost_level1;
> + else if (cpu->hwp_boost_min < HWP_GUARANTEED_PERF(cpu->hwp_cap_cached))
> + cpu->hwp_boost_min = HWP_GUARANTEED_PERF(cpu->hwp_cap_cached);
> + else if (cpu->hwp_boost_min == HWP_GUARANTEED_PERF(cpu->hwp_cap_cached) &&
> + max_limit != HWP_GUARANTEED_PERF(cpu->hwp_cap_cached))
> + cpu->hwp_boost_min = max_limit;
> + else
> + return;
> +
> + hwp_req = (hwp_req & ~GENMASK_ULL(7, 0)) | cpu->hwp_boost_min;
> + wrmsrl(MSR_HWP_REQUEST, hwp_req);
> + cpu->last_update = cpu->sample.time;
> +}
> +
> +static inline bool intel_pstate_hwp_boost_down(struct cpudata *cpu)
> +{
> + if (cpu->hwp_boost_min) {
> + bool expired;
> +
> + /* Check if we are idle for hold time to boost down */
> + expired = time_after64(cpu->sample.time, cpu->last_update +
> + (hwp_boost_hold_time_ms * NSEC_PER_MSEC));
> + if (expired) {
> + wrmsrl(MSR_HWP_REQUEST, cpu->hwp_req_cached);
> + cpu->hwp_boost_min = 0;
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> static inline void intel_pstate_calc_avg_perf(struct cpudata *cpu)
> {
> struct sample *sample = &cpu->sample;
> --
> 2.13.6
>
^ permalink raw reply
* Re: [RFC/RFT] [PATCH v2 4/6] cpufreq: intel_pstate: HWP boost performance on IO wakeup
From: Rafael J. Wysocki @ 2018-05-29 7:44 UTC (permalink / raw)
To: Srinivas Pandruvada
Cc: Len Brown, Rafael J. Wysocki, Peter Zijlstra, Mel Gorman,
Linux PM, Linux Kernel Mailing List, Juri Lelli, Viresh Kumar
In-Reply-To: <20180524014738.52924-5-srinivas.pandruvada@linux.intel.com>
On Thu, May 24, 2018 at 3:47 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> This change uses SCHED_CPUFREQ_IOWAIT flag to boost HWP performance.
> Since SCHED_CPUFREQ_IOWAIT flag is set frequently, we don't start
> boosting steps unless we see two consecutive flags in two ticks. This
> avoids boosting due to IO because of regular system activities.
>
> To avoid synchronization issues, the actual processing of the flag is
> done on the local CPU callback.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> drivers/cpufreq/intel_pstate.c | 44 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 382160570b5f..6d0ebe4fe1c7 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -223,6 +223,8 @@ struct global_params {
> * operation
> * @hwp_req_cached: Cached value of the last HWP Request MSR
> * @hwp_cap_cached: Cached value of the last HWP Capabilities MSR
> + * @last_io_update: Last time when IO wake flag was set
> + * @sched_flags: Store scheduler flags for possible cross CPU update
> * @hwp_boost_min: Last HWP boosted min performance
> *
> * This structure stores per CPU instance data for all CPUs.
> @@ -258,6 +260,8 @@ struct cpudata {
> s16 epp_saved;
> u64 hwp_req_cached;
> u64 hwp_cap_cached;
> + u64 last_io_update;
> + unsigned long sched_flags;
> int hwp_boost_min;
> };
>
> @@ -1462,9 +1466,49 @@ static inline bool intel_pstate_hwp_boost_down(struct cpudata *cpu)
> return false;
> }
>
> +static inline void intel_pstate_update_util_hwp_local(struct cpudata *cpu,
> + u64 time)
> +{
> + bool io_flag;
> +
> + cpu->sample.time = time;
> + io_flag = test_and_clear_bit(SCHED_CPUFREQ_IOWAIT, &cpu->sched_flags);
I don't think you need to use bit ops here.
_update_util() runs under rq->lock for the target CPU, so it will not
run concurrently on two different CPUs for the same target anyway.
> + if (io_flag) {
> + bool do_io = false;
> +
> + /*
> + * Set iowait_boost flag and update time. Since IO WAIT flag
> + * is set all the time, we can't just conclude that there is
> + * some IO bound activity is scheduled on this CPU with just
> + * one occurrence. If we receive at least two in two
> + * consecutive ticks, then we treat as boost candidate.
> + */
> + if (time_before64(time, cpu->last_io_update + 2 * TICK_NSEC))
> + do_io = true;
> +
> + cpu->last_io_update = time;
> +
> + if (do_io)
> + intel_pstate_hwp_boost_up(cpu);
But what happens if user space wants to update the limits while
boosting is in effect? Shouldn't it take hwp_boost_min into account
then?
> +
> + } else {
> + if (intel_pstate_hwp_boost_down(cpu))
> + return;
> + }
> +
> + cpu->last_update = time;
> +}
> +
> static inline void intel_pstate_update_util_hwp(struct update_util_data *data,
> u64 time, unsigned int flags)
> {
> + struct cpudata *cpu = container_of(data, struct cpudata, update_util);
> +
> + if (flags & SCHED_CPUFREQ_IOWAIT)
> + set_bit(SCHED_CPUFREQ_IOWAIT, &cpu->sched_flags);
> +
> + if (smp_processor_id() == cpu->cpu)
> + intel_pstate_update_util_hwp_local(cpu, time);
> }
>
> static inline void intel_pstate_calc_avg_perf(struct cpudata *cpu)
> --
> 2.13.6
>
^ permalink raw reply
* Re: [RFC/RFT] [PATCH v2 3/6] cpufreq: intel_pstate: Add update_util_hook for HWP
From: Rafael J. Wysocki @ 2018-05-29 7:37 UTC (permalink / raw)
To: Srinivas Pandruvada
Cc: Len Brown, Rafael J. Wysocki, Peter Zijlstra, Mel Gorman,
Linux PM, Linux Kernel Mailing List, Juri Lelli, Viresh Kumar
In-Reply-To: <20180524014738.52924-4-srinivas.pandruvada@linux.intel.com>
On Thu, May 24, 2018 at 3:47 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> When HWP dynamic boost is active then set the HWP specific update util
> hook.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Splitting this patch out of the next one is sort of artificial.
> ---
> drivers/cpufreq/intel_pstate.c | 21 +++++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 6ad46e07cad6..382160570b5f 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -291,6 +291,7 @@ static struct pstate_funcs pstate_funcs __read_mostly;
>
> static int hwp_active __read_mostly;
> static bool per_cpu_limits __read_mostly;
> +static bool hwp_boost __read_mostly;
Because of this, among other things.
>
> static struct cpufreq_driver *intel_pstate_driver __read_mostly;
>
> @@ -1461,6 +1462,11 @@ static inline bool intel_pstate_hwp_boost_down(struct cpudata *cpu)
> return false;
> }
>
> +static inline void intel_pstate_update_util_hwp(struct update_util_data *data,
> + u64 time, unsigned int flags)
> +{
> +}
> +
> static inline void intel_pstate_calc_avg_perf(struct cpudata *cpu)
> {
> struct sample *sample = &cpu->sample;
> @@ -1764,7 +1770,7 @@ static void intel_pstate_set_update_util_hook(unsigned int cpu_num)
> {
> struct cpudata *cpu = all_cpu_data[cpu_num];
>
> - if (hwp_active)
> + if (hwp_active && !hwp_boost)
> return;
>
> if (cpu->update_util_set)
> @@ -1772,8 +1778,12 @@ static void intel_pstate_set_update_util_hook(unsigned int cpu_num)
>
> /* Prevent intel_pstate_update_util() from using stale data. */
> cpu->sample.time = 0;
> - cpufreq_add_update_util_hook(cpu_num, &cpu->update_util,
> - intel_pstate_update_util);
> + if (hwp_active)
> + cpufreq_add_update_util_hook(cpu_num, &cpu->update_util,
> + intel_pstate_update_util_hwp);
> + else
> + cpufreq_add_update_util_hook(cpu_num, &cpu->update_util,
> + intel_pstate_update_util);
> cpu->update_util_set = true;
> }
>
> @@ -1885,8 +1895,11 @@ static int intel_pstate_set_policy(struct cpufreq_policy *policy)
> intel_pstate_set_update_util_hook(policy->cpu);
> }
>
> - if (hwp_active)
> + if (hwp_active) {
> + if (!hwp_boost)
> + intel_pstate_clear_update_util_hook(policy->cpu);
> intel_pstate_hwp_set(policy->cpu);
> + }
>
> mutex_unlock(&intel_pstate_limits_lock);
>
> --
> 2.13.6
>
^ permalink raw reply
* Re: [PATCH] cpuidle/powernv : Add Description for cpuidle state
From: Stewart Smith @ 2018-05-29 1:39 UTC (permalink / raw)
To: rjw, daniel.lezcano, benh, paulus, mpe, linux-pm, linuxppc-dev,
linux-kernel
Cc: Abhishek Goel
In-Reply-To: <20180528173442.100642-1-huntbag@linux.vnet.ibm.com>
Abhishek Goel <huntbag@linux.vnet.ibm.com> writes:
> @@ -215,7 +216,7 @@ static inline void add_powernv_state(int index, const char *name,
> u64 psscr_val, u64 psscr_mask)
> {
> strlcpy(powernv_states[index].name, name, CPUIDLE_NAME_LEN);
> - strlcpy(powernv_states[index].desc, name, CPUIDLE_NAME_LEN);
> + strlcpy(powernv_states[index].desc, desc, CPUIDLE_DESC_LEN);
We should still fall back to using name in the event of desc being null,
as not all firmware will expose the descriptions.
> @@ -311,6 +313,11 @@ static int powernv_add_idle_states(void)
> pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-names in DT\n");
> goto out;
> }
> + if (of_property_read_string_array(power_mgt,
> + "ibm,cpu-idle-state-descs", descs, dt_idle_states) < 0) {
> + pr_warn("cpuidle-powernv: missing ibm,cpu-idle-state-descs in DT\n");
> + goto out;
> + }
I don't think pr_warn is appropriate here, as for all current released
firmware we don't have that property. I think perhaps just silently
continuing on is okay, as we have to keep compatibility with that firmware.
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -17,7 +17,7 @@
>
> #define CPUIDLE_STATE_MAX 10
> #define CPUIDLE_NAME_LEN 16
> -#define CPUIDLE_DESC_LEN 32
> +#define CPUIDLE_DESC_LEN 60
Do we really get that long?
--
Stewart Smith
OPAL Architect, IBM.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox