cpufreq: Allow light-weight tear down and bring up of CPUs

PATCH Superseded Viresh Kumar 3 participants 10 messages Feb 12, 2019 – Feb 19, 2019 CPU frequency scaling framework

Share & tools

Viresh Kumar

author maintainer

2019-02-12 07:06 UTC #0 lore raw

v2 cover cpufreq: Allow light-weight tear down and bring up of CPUs

The cpufreq core doesn't remove the cpufreq policy anymore on CPU offline operation, rather that happens when the CPU device gets unregistered from the kernel. This allows faster recovery when the CPU comes back online. This is also very useful during system wide suspend/resume where we offline all non-boot CPUs during suspend and then bring them back on resume.

This patcset takes the same idea a step ahead to allow drivers to do light weight tear-down and bring up during CPU offline/online operations and updates the cpufreq-dt driver to implement the new helpers.

V1->V2:
- s/light_weight_exit()/offline()
- Also introduce the online() counterpart

Viresh Kumar (2):
cpufreq: Allow light-weight tear down and bring up of CPUs
cpufreq: dt: Implement online/offline() callbacks

 drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++
 drivers/cpufreq/cpufreq.c    | 55 +++++++++++++++++++++++-------------
 include/linux/cpufreq.h      |  2 ++
 3 files changed, 55 insertions(+), 19 deletions(-)

Signature · 1 line

2.20.1.321.g9e740568ce00

Viresh Kumar

author maintainer

2019-02-12 07:06 UTC #1 lore raw

PATCH v2 1/2 cpufreq: Allow light-weight tear down and bring up of CPUs

This commit takes the same idea a step ahead to allow drivers to do light weight tear-down and bring-up during CPU offline and online operations.

A new set of callbacks is introduced, online/offline(). online() gets called when the first CPU of an inactive policy is brought up and offline() gets called when all the CPUs of a policy are offlined.

The existing init/exit() callback get called on policy creation/destruction. They also get called instead of online/offline() callbacks if the online/offline() callbacks aren't provided.

This also moves around some code to get executed only for the new-policy case going forward.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq.c | 55 +++++++++++++++++++++++++--------------
 include/linux/cpufreq.h   |  2 ++
 2 files changed, 38 insertions(+), 19 deletions(-)

drivers/cpufreq/cpufreq.c +36 ~~-19~~

index 96a69c67a545..2b3e9d501f49 100644
@@ -1201,28 +1201,39 @@ static int cpufreq_online(unsigned int cpu)
 			return -ENOMEM;
 	}
 
-	cpumask_copy(policy->cpus, cpumask_of(cpu));
+	if (!new_policy && cpufreq_driver->online) {
+		ret = cpufreq_driver->online(policy);
+		if (ret) {
+			pr_debug("%s: %d: initialization failed\n", __func__,
+				 __LINE__);
+			goto out_exit_policy;
+		}
 
-	/* call driver. From then on the cpufreq must be able
-	 * to accept all calls to ->verify and ->setpolicy for this CPU
-	 */
-	ret = cpufreq_driver->init(policy);
-	if (ret) {
-		pr_debug("initialization failed\n");
-		goto out_free_policy;
-	}
+		/* Recover policy->cpus using related_cpus */
+		cpumask_copy(policy->cpus, policy->related_cpus);
+	} else {
+		cpumask_copy(policy->cpus, cpumask_of(cpu));
 
-	ret = cpufreq_table_validate_and_sort(policy);
-	if (ret)
-		goto out_exit_policy;
+		/*
+		 * Call driver. From then on the cpufreq must be able
+		 * to accept all calls to ->verify and ->setpolicy for this CPU.
+		 */
+		ret = cpufreq_driver->init(policy);
+		if (ret) {
+			pr_debug("%s: %d: initialization failed\n", __func__,
+				 __LINE__);
+			goto out_free_policy;
+		}
 
-	down_write(&policy->rwsem);
+		ret = cpufreq_table_validate_and_sort(policy);
+		if (ret)
+			goto out_exit_policy;
 
-	if (new_policy) {
 		/* related_cpus should at least include policy->cpus. */
 		cpumask_copy(policy->related_cpus, policy->cpus);
 	}
 
+	down_write(&policy->rwsem);
 	/*
 	 * affected cpus must always be the one, which are online. We aren't
 	 * managing offline cpus here.

@@ -1421,11 +1432,12 @@ static int cpufreq_offline(unsigned int cpu)
 		cpufreq_exit_governor(policy);
 
 	/*
-	 * Perform the ->exit() even during light-weight tear-down,
-	 * since this is a core component, and is essential for the
-	 * subsequent light-weight ->init() to succeed.
+	 * Perform the ->offline() during light-weight tear-down, as
+	 * that allows fast recovery when the CPU comes back.
 	 */
-	if (cpufreq_driver->exit) {
+	if (cpufreq_driver->offline) {
+		cpufreq_driver->offline(policy);
+	} else if (cpufreq_driver->exit) {
 		cpufreq_driver->exit(policy);
 		policy->freq_table = NULL;
 	}

@@ -1454,8 +1466,13 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	cpumask_clear_cpu(cpu, policy->real_cpus);
 	remove_cpu_dev_symlink(policy, dev);
 
-	if (cpumask_empty(policy->real_cpus))
+	if (cpumask_empty(policy->real_cpus)) {
+		/* We did light-weight exit earlier, do full tear down now */
+		if (cpufreq_driver->offline)
+			cpufreq_driver->exit(policy);
+
 		cpufreq_policy_free(policy);
+	}
 }
 
 /**

include/linux/cpufreq.h +2 -0

index 9db074ecbbd7..b160e98076e3 100644
@@ -325,6 +325,8 @@ struct cpufreq_driver {
 	/* optional */
 	int		(*bios_limit)(int cpu, unsigned int *limit);
 
+	int		(*online)(struct cpufreq_policy *policy);
+	int		(*offline)(struct cpufreq_policy *policy);
 	int		(*exit)(struct cpufreq_policy *policy);
 	void		(*stop_cpu)(struct cpufreq_policy *policy);
 	int		(*suspend)(struct cpufreq_policy *policy);

Signature · 1 line

2.20.1.321.g9e740568ce00

Viresh Kumar

author maintainer

2019-02-12 10:47 UTC #2 lore raw

PATCH v3 1/2 cpufreq: Allow light-weight tear down and bring up of CPUs

This commit takes the same idea a step ahead to allow drivers to do light weight tear-down and bring-up during CPU offline and online operations.

The existing init/exit() callback get called on policy creation/destruction. They also get called instead of online/offline() callbacks if the online/offline() callbacks aren't provided.

This also moves around some code to get executed only for the new-policy case going forward.

Signed-off-by Viresh Kumar <[email protected]>

---
V2->V3:
- Additional check in cpufreq_register_driver().

 drivers/cpufreq/cpufreq.c | 58 +++++++++++++++++++++++++--------------
 include/linux/cpufreq.h   |  2 ++
 2 files changed, 40 insertions(+), 20 deletions(-)

drivers/cpufreq/cpufreq.c +38 ~~-20~~

index 96a69c67a545..dd7626c5eb85 100644
@@ -1201,28 +1201,39 @@ static int cpufreq_online(unsigned int cpu)
 			return -ENOMEM;
 	}
 
-	cpumask_copy(policy->cpus, cpumask_of(cpu));
+	if (!new_policy && cpufreq_driver->online) {
+		ret = cpufreq_driver->online(policy);
+		if (ret) {
+			pr_debug("%s: %d: initialization failed\n", __func__,
+				 __LINE__);
+			goto out_exit_policy;
+		}
 
-	/* call driver. From then on the cpufreq must be able
-	 * to accept all calls to ->verify and ->setpolicy for this CPU
-	 */
-	ret = cpufreq_driver->init(policy);
-	if (ret) {
-		pr_debug("initialization failed\n");
-		goto out_free_policy;
-	}
+		/* Recover policy->cpus using related_cpus */
+		cpumask_copy(policy->cpus, policy->related_cpus);
+	} else {
+		cpumask_copy(policy->cpus, cpumask_of(cpu));
 
-	ret = cpufreq_table_validate_and_sort(policy);
-	if (ret)
-		goto out_exit_policy;
+		/*
+		 * Call driver. From then on the cpufreq must be able
+		 * to accept all calls to ->verify and ->setpolicy for this CPU.
+		 */
+		ret = cpufreq_driver->init(policy);
+		if (ret) {
+			pr_debug("%s: %d: initialization failed\n", __func__,
+				 __LINE__);
+			goto out_free_policy;
+		}
 
-	down_write(&policy->rwsem);
+		ret = cpufreq_table_validate_and_sort(policy);
+		if (ret)
+			goto out_exit_policy;
 
-	if (new_policy) {
 		/* related_cpus should at least include policy->cpus. */
 		cpumask_copy(policy->related_cpus, policy->cpus);
 	}
 
+	down_write(&policy->rwsem);
 	/*
 	 * affected cpus must always be the one, which are online. We aren't
 	 * managing offline cpus here.

@@ -1421,11 +1432,12 @@ static int cpufreq_offline(unsigned int cpu)
 		cpufreq_exit_governor(policy);
 
 	/*
-	 * Perform the ->exit() even during light-weight tear-down,
-	 * since this is a core component, and is essential for the
-	 * subsequent light-weight ->init() to succeed.
+	 * Perform the ->offline() during light-weight tear-down, as
+	 * that allows fast recovery when the CPU comes back.
 	 */
-	if (cpufreq_driver->exit) {
+	if (cpufreq_driver->offline) {
+		cpufreq_driver->offline(policy);
+	} else if (cpufreq_driver->exit) {
 		cpufreq_driver->exit(policy);
 		policy->freq_table = NULL;
 	}

@@ -1454,8 +1466,13 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	cpumask_clear_cpu(cpu, policy->real_cpus);
 	remove_cpu_dev_symlink(policy, dev);
 
-	if (cpumask_empty(policy->real_cpus))
+	if (cpumask_empty(policy->real_cpus)) {
+		/* We did light-weight exit earlier, do full tear down now */
+		if (cpufreq_driver->offline)
+			cpufreq_driver->exit(policy);
+
 		cpufreq_policy_free(policy);
+	}
 }
 
 /**

@@ -2488,7 +2505,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 		    driver_data->target) ||
 	     (driver_data->setpolicy && (driver_data->target_index ||
 		    driver_data->target)) ||
-	     (!!driver_data->get_intermediate != !!driver_data->target_intermediate))
+	     (!!driver_data->get_intermediate != !!driver_data->target_intermediate) ||
+	     (!driver_data->online ^ !driver_data->offline))
 		return -EINVAL;
 
 	pr_debug("trying to register driver %s\n", driver_data->name);

include/linux/cpufreq.h +2 -0

index 9db074ecbbd7..b160e98076e3 100644
@@ -325,6 +325,8 @@ struct cpufreq_driver {
 	/* optional */
 	int		(*bios_limit)(int cpu, unsigned int *limit);
 
+	int		(*online)(struct cpufreq_policy *policy);
+	int		(*offline)(struct cpufreq_policy *policy);
 	int		(*exit)(struct cpufreq_policy *policy);
 	void		(*stop_cpu)(struct cpufreq_policy *policy);
 	int		(*suspend)(struct cpufreq_policy *policy);

Signature · 1 line

2.20.1.321.g9e740568ce00

Rafael J. Wysocki

maintainer

2019-02-12 10:54 UTC #3 lore raw

Quoted 1 line · Viresh Kumar

[cut]

drivers/cpufreq/cpufreq.c int cpufreq_register_driver(struct cpufreq_driver *driver_data) · line 2509

Quoted 7 lines

drivers/cpufreq/cpufreq.c +2 -1

@@ -2488,7 +2505,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
                    driver_data->target) ||
             (driver_data->setpolicy && (driver_data->target_index ||
                    driver_data->target)) ||
-            (!!driver_data->get_intermediate != !!driver_data->target_intermediate))
+            (!!driver_data->get_intermediate != !!driver_data->target_intermediate) ||
+            (!driver_data->online ^ !driver_data->offline))

Why do you need the ^ here? != should be sufficient:

!driver_data->online != !driver_data->offline

Quoted 3 lines

drivers/cpufreq/cpufreq.c +0 -0

                return -EINVAL;
 
        pr_debug("trying to register driver %s\n", driver_data->name);

Viresh Kumar

author maintainer

2019-02-12 11:06 UTC #4 lore raw

PATCH v4 1/2 cpufreq: Allow light-weight tear down and bring up of CPUs

In reply to Viresh Kumar

This commit takes the same idea a step ahead to allow drivers to do light weight tear-down and bring-up during CPU offline and online operations.

The existing init/exit() callback get called on policy creation/destruction. They also get called instead of online/offline() callbacks if the online/offline() callbacks aren't provided.

This also moves around some code to get executed only for the new-policy case going forward.

Signed-off-by Viresh Kumar <[email protected]>

---
V3->V4:
- Use != instead of ^

 drivers/cpufreq/cpufreq.c | 58 +++++++++++++++++++++++++--------------
 include/linux/cpufreq.h   |  2 ++
 2 files changed, 40 insertions(+), 20 deletions(-)

drivers/cpufreq/cpufreq.c +38 ~~-20~~

index 96a69c67a545..55e9795801a4 100644
@@ -1201,28 +1201,39 @@ static int cpufreq_online(unsigned int cpu)
 			return -ENOMEM;
 	}
 
-	cpumask_copy(policy->cpus, cpumask_of(cpu));
+	if (!new_policy && cpufreq_driver->online) {
+		ret = cpufreq_driver->online(policy);
+		if (ret) {
+			pr_debug("%s: %d: initialization failed\n", __func__,
+				 __LINE__);
+			goto out_exit_policy;
+		}
 
-	/* call driver. From then on the cpufreq must be able
-	 * to accept all calls to ->verify and ->setpolicy for this CPU
-	 */
-	ret = cpufreq_driver->init(policy);
-	if (ret) {
-		pr_debug("initialization failed\n");
-		goto out_free_policy;
-	}
+		/* Recover policy->cpus using related_cpus */
+		cpumask_copy(policy->cpus, policy->related_cpus);
+	} else {
+		cpumask_copy(policy->cpus, cpumask_of(cpu));
 
-	ret = cpufreq_table_validate_and_sort(policy);
-	if (ret)
-		goto out_exit_policy;
+		/*
+		 * Call driver. From then on the cpufreq must be able
+		 * to accept all calls to ->verify and ->setpolicy for this CPU.
+		 */
+		ret = cpufreq_driver->init(policy);
+		if (ret) {
+			pr_debug("%s: %d: initialization failed\n", __func__,
+				 __LINE__);
+			goto out_free_policy;
+		}
 
-	down_write(&policy->rwsem);
+		ret = cpufreq_table_validate_and_sort(policy);
+		if (ret)
+			goto out_exit_policy;
 
-	if (new_policy) {
 		/* related_cpus should at least include policy->cpus. */
 		cpumask_copy(policy->related_cpus, policy->cpus);
 	}
 
+	down_write(&policy->rwsem);
 	/*
 	 * affected cpus must always be the one, which are online. We aren't
 	 * managing offline cpus here.

@@ -1421,11 +1432,12 @@ static int cpufreq_offline(unsigned int cpu)
 		cpufreq_exit_governor(policy);
 
 	/*
-	 * Perform the ->exit() even during light-weight tear-down,
-	 * since this is a core component, and is essential for the
-	 * subsequent light-weight ->init() to succeed.
+	 * Perform the ->offline() during light-weight tear-down, as
+	 * that allows fast recovery when the CPU comes back.
 	 */
-	if (cpufreq_driver->exit) {
+	if (cpufreq_driver->offline) {
+		cpufreq_driver->offline(policy);
+	} else if (cpufreq_driver->exit) {
 		cpufreq_driver->exit(policy);
 		policy->freq_table = NULL;
 	}

@@ -1454,8 +1466,13 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	cpumask_clear_cpu(cpu, policy->real_cpus);
 	remove_cpu_dev_symlink(policy, dev);
 
-	if (cpumask_empty(policy->real_cpus))
+	if (cpumask_empty(policy->real_cpus)) {
+		/* We did light-weight exit earlier, do full tear down now */
+		if (cpufreq_driver->offline)
+			cpufreq_driver->exit(policy);
+
 		cpufreq_policy_free(policy);
+	}
 }
 
 /**

@@ -2488,7 +2505,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 		    driver_data->target) ||
 	     (driver_data->setpolicy && (driver_data->target_index ||
 		    driver_data->target)) ||
-	     (!!driver_data->get_intermediate != !!driver_data->target_intermediate))
+	     (!!driver_data->get_intermediate != !!driver_data->target_intermediate) ||
+	     (!driver_data->online != !driver_data->offline))
 		return -EINVAL;
 
 	pr_debug("trying to register driver %s\n", driver_data->name);

include/linux/cpufreq.h +2 -0

index 9db074ecbbd7..b160e98076e3 100644
@@ -325,6 +325,8 @@ struct cpufreq_driver {
 	/* optional */
 	int		(*bios_limit)(int cpu, unsigned int *limit);
 
+	int		(*online)(struct cpufreq_policy *policy);
+	int		(*offline)(struct cpufreq_policy *policy);
 	int		(*exit)(struct cpufreq_policy *policy);
 	void		(*stop_cpu)(struct cpufreq_policy *policy);
 	int		(*suspend)(struct cpufreq_policy *policy);

Signature · 1 line

2.20.1.321.g9e740568ce00

Rafael J. Wysocki

2019-02-19 10:25 UTC #5 lore raw

Quoted 137 lines · CET Viresh Kumar

This commit takes the same idea a step ahead to allow drivers to do light weight tear-down and bring-up during CPU offline and online operations.

The existing init/exit() callback get called on policy creation/destruction. They also get called instead of online/offline() callbacks if the online/offline() callbacks aren't provided.

This also moves around some code to get executed only for the new-policy case going forward.

Signed-off-by Viresh Kumar <[email protected]>

---
V3->V4:
- Use != instead of ^

 drivers/cpufreq/cpufreq.c | 58 +++++++++++++++++++++++++--------------
 include/linux/cpufreq.h   |  2 ++
 2 files changed, 40 insertions(+), 20 deletions(-)

drivers/cpufreq/cpufreq.c +38 ~~-20~~

index 96a69c67a545..55e9795801a4 100644
@@ -1201,28 +1201,39 @@ static int cpufreq_online(unsigned int cpu)
 			return -ENOMEM;
 	}
 
-	cpumask_copy(policy->cpus, cpumask_of(cpu));
+	if (!new_policy && cpufreq_driver->online) {
+		ret = cpufreq_driver->online(policy);
+		if (ret) {
+			pr_debug("%s: %d: initialization failed\n", __func__,
+				 __LINE__);
+			goto out_exit_policy;
+		}
 
-	/* call driver. From then on the cpufreq must be able
-	 * to accept all calls to ->verify and ->setpolicy for this CPU
-	 */
-	ret = cpufreq_driver->init(policy);
-	if (ret) {
-		pr_debug("initialization failed\n");
-		goto out_free_policy;
-	}
+		/* Recover policy->cpus using related_cpus */
+		cpumask_copy(policy->cpus, policy->related_cpus);
+	} else {
+		cpumask_copy(policy->cpus, cpumask_of(cpu));
 
-	ret = cpufreq_table_validate_and_sort(policy);
-	if (ret)
-		goto out_exit_policy;
+		/*
+		 * Call driver. From then on the cpufreq must be able
+		 * to accept all calls to ->verify and ->setpolicy for this CPU.
+		 */
+		ret = cpufreq_driver->init(policy);
+		if (ret) {
+			pr_debug("%s: %d: initialization failed\n", __func__,
+				 __LINE__);
+			goto out_free_policy;
+		}
 
-	down_write(&policy->rwsem);
+		ret = cpufreq_table_validate_and_sort(policy);
+		if (ret)
+			goto out_exit_policy;
 
-	if (new_policy) {
 		/* related_cpus should at least include policy->cpus. */
 		cpumask_copy(policy->related_cpus, policy->cpus);
 	}
 
+	down_write(&policy->rwsem);
 	/*
 	 * affected cpus must always be the one, which are online. We aren't
 	 * managing offline cpus here.

@@ -1421,11 +1432,12 @@ static int cpufreq_offline(unsigned int cpu)
 		cpufreq_exit_governor(policy);
 
 	/*
-	 * Perform the ->exit() even during light-weight tear-down,
-	 * since this is a core component, and is essential for the
-	 * subsequent light-weight ->init() to succeed.
+	 * Perform the ->offline() during light-weight tear-down, as
+	 * that allows fast recovery when the CPU comes back.
 	 */
-	if (cpufreq_driver->exit) {
+	if (cpufreq_driver->offline) {
+		cpufreq_driver->offline(policy);
+	} else if (cpufreq_driver->exit) {
 		cpufreq_driver->exit(policy);
 		policy->freq_table = NULL;
 	}

@@ -1454,8 +1466,13 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	cpumask_clear_cpu(cpu, policy->real_cpus);
 	remove_cpu_dev_symlink(policy, dev);
 
-	if (cpumask_empty(policy->real_cpus))
+	if (cpumask_empty(policy->real_cpus)) {
+		/* We did light-weight exit earlier, do full tear down now */
+		if (cpufreq_driver->offline)
+			cpufreq_driver->exit(policy);
+
 		cpufreq_policy_free(policy);
+	}
 }
 
 /**

@@ -2488,7 +2505,8 @@ int cpufreq_register_driver(struct cpufreq_driver *driver_data)
 		    driver_data->target) ||
 	     (driver_data->setpolicy && (driver_data->target_index ||
 		    driver_data->target)) ||
-	     (!!driver_data->get_intermediate != !!driver_data->target_intermediate))
+	     (!!driver_data->get_intermediate != !!driver_data->target_intermediate) ||
+	     (!driver_data->online != !driver_data->offline))
 		return -EINVAL;
 
 	pr_debug("trying to register driver %s\n", driver_data->name);

include/linux/cpufreq.h +0 -0

index 9db074ecbbd7..b160e98076e3 100644

include/linux/cpufreq.h struct cpufreq_driver { · line 332

Quoted 8 lines

include/linux/cpufreq.h +2 -0

@@ -325,6 +325,8 @@ struct cpufreq_driver {
 	/* optional */
 	int		(*bios_limit)(int cpu, unsigned int *limit);
 
+	int		(*online)(struct cpufreq_policy *policy);
+	int		(*offline)(struct cpufreq_policy *policy);
 	int		(*exit)(struct cpufreq_policy *policy);
 	void		(*stop_cpu)(struct cpufreq_policy *policy);
 	int		(*suspend)(struct cpufreq_policy *policy);

Applied along with the [2/2], thanks!

Viresh Kumar

author maintainer

2019-02-12 07:06 UTC #6 lore raw

PATCH v2 2/2 cpufreq: dt: Implement online/offline() callbacks

In reply to Viresh Kumar

Implement the light-weight tear down and bring up helpers to reduce the amount of work to do on CPU offline/online operation.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

drivers/cpufreq/cpufreq-dt.c +17 -0

index 7ba392911cd0..1aefaa1b0ca2 100644
@@ -295,6 +295,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 	return ret;
 }
 
+static int cpufreq_online(struct cpufreq_policy *policy)
+{
+	/* We did light-weight tear down earlier, nothing to do here */
+	return 0;
+}
+
+static int cpufreq_offline(struct cpufreq_policy *policy)
+{
+	/*
+	 * Preserve policy->driver_data and don't free resources on light-weight
+	 * tear down.
+	 */
+	return 0;
+}
+
 static int cpufreq_exit(struct cpufreq_policy *policy)
 {
 	struct private_data *priv = policy->driver_data;

@@ -319,6 +334,8 @@ static struct cpufreq_driver dt_cpufreq_driver = {
 	.get = cpufreq_generic_get,
 	.init = cpufreq_init,
 	.exit = cpufreq_exit,
+	.online = cpufreq_online,
+	.offline = cpufreq_offline,
 	.name = "cpufreq-dt",
 	.attr = cpufreq_dt_attr,
 	.suspend = cpufreq_generic_suspend,

Signature · 1 line

2.20.1.321.g9e740568ce00

Rafael J. Wysocki

maintainer

2019-02-12 09:43 UTC #7 lore raw

Re: [PATCH V2 2/2] cpufreq: dt: Implement online/offline() callbacks

Quoted 14 lines · Viresh Kumar

Implement the light-weight tear down and bring up helpers to reduce the amount of work to do on CPU offline/online operation.
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
drivers/cpufreq/cpufreq-dt.c +0 -0
index 7ba392911cd0..1aefaa1b0ca2 100644

drivers/cpufreq/cpufreq-dt.c static int cpufreq_init(struct cpufreq_policy *policy) · line 302

Quoted 8 lines

drivers/cpufreq/cpufreq-dt.c +5 -0

@@ -295,6 +295,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
        return ret;
 }
 
+static int cpufreq_online(struct cpufreq_policy *policy)
+{
+       /* We did light-weight tear down earlier, nothing to do here */
+       return 0;
+}

I think you could avoid having to add this empty stub if the core checked for both online and offline, that is

if (driver->offline || driver->online) {
    ret = driver->online ? driver->online(policy) : 0;
} else {
    ret = driver->init(policy);
}

or similar.

Quoted 25 lines

drivers/cpufreq/cpufreq-dt.c +12 -1

+
+static int cpufreq_offline(struct cpufreq_policy *policy)
+{
+       /*
+        * Preserve policy->driver_data and don't free resources on light-weight
+        * tear down.
+        */
+       return 0;
+}
+
 static int cpufreq_exit(struct cpufreq_policy *policy)
 {
        struct private_data *priv = policy->driver_data;

@@ -319,6 +334,8 @@ static struct cpufreq_driver dt_cpufreq_driver = {
        .get = cpufreq_generic_get,
        .init = cpufreq_init,
        .exit = cpufreq_exit,
+       .online = cpufreq_online,
+       .offline = cpufreq_offline,
        .name = "cpufreq-dt",
        .attr = cpufreq_dt_attr,
        .suspend = cpufreq_generic_suspend,
--

2.20.1.321.g9e740568ce00

Viresh Kumar

author maintainer

2019-02-12 09:49 UTC #8 lore raw

Re: [PATCH V2 2/2] cpufreq: dt: Implement online/offline() callbacks

Quoted 27 lines · Rafael J. Wysocki

Quoted 14 lines · Viresh Kumar
Implement the light-weight tear down and bring up helpers to reduce the amount of work to do on CPU offline/online operation.
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
drivers/cpufreq/cpufreq-dt.c +0 -0
index 7ba392911cd0..1aefaa1b0ca2 100644
drivers/cpufreq/cpufreq-dt.c static int cpufreq_init(struct cpufreq_policy *policy) · line 302
Quoted 8 lines
drivers/cpufreq/cpufreq-dt.c +5 -0
@@ -295,6 +295,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
        return ret;
 }
 
+static int cpufreq_online(struct cpufreq_policy *policy)
+{
+       /* We did light-weight tear down earlier, nothing to do here */
+       return 0;
+}
I think you could avoid having to add this empty stub if the core checked for both online and offline, that is

Quoted 1 line

if (driver->offline || driver->online) {

This doesn't look great as all we should care about here is ->online() and checking for offline as well looks a bit hacky.

Quoted 6 lines

    ret = driver->online ? driver->online(policy) : 0;
} else {
    ret = driver->init(policy);
}

or similar.

I also thought of a new flag like: CPUFREQ_LIGHT_WEIGHT_OFFLINE and then we can get rid of both online/offline dummies but then thought of starting with the dummy routines to begin with :)

Signature · 1 line

viresh

Rafael J. Wysocki

maintainer

2019-02-12 10:26 UTC #9 lore raw

Re: [PATCH V2 2/2] cpufreq: dt: Implement online/offline() callbacks

Quoted 29 lines · Viresh Kumar

Quoted 27 lines · Rafael J. Wysocki
Quoted 14 lines · Viresh Kumar
Implement the light-weight tear down and bring up helpers to reduce the amount of work to do on CPU offline/online operation.
Signed-off-by Viresh Kumar <[email protected]>
---
 drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
drivers/cpufreq/cpufreq-dt.c +0 -0
index 7ba392911cd0..1aefaa1b0ca2 100644
drivers/cpufreq/cpufreq-dt.c static int cpufreq_init(struct cpufreq_policy *policy) · line 302
Quoted 8 lines
drivers/cpufreq/cpufreq-dt.c +5 -0
@@ -295,6 +295,21 @@ static int cpufreq_init(struct cpufreq_policy *policy)
        return ret;
 }
 
+static int cpufreq_online(struct cpufreq_policy *policy)
+{
+       /* We did light-weight tear down earlier, nothing to do here */
+       return 0;
+}
I think you could avoid having to add this empty stub if the core checked for both online and offline, that is

Quoted 4 lines

Quoted 1 line
if (driver->offline || driver->online) {
This doesn't look great as all we should care about here is ->online() and checking for offline as well looks a bit hacky.

That's a matter of the driver I/F definition.

You can require online to be there if offline is set and the other way around (but then you should check if that's the case while registering the driver) or you can allow just one of them to be provided for the lightweight online/offline to be done. In the latter case the presence of the other callback plays the role of a flag.

Both are basically fine by me, but if you go for the former, please add the checks to cpufreq_register_driver().