[08/13] OMAP2+: powerdomain: control power domains next state

Commit Message

Jean Pihet July 29, 2011, 8:47 a.m. UTC

Todd,

On Fri, Jul 29, 2011 at 9:59 AM, Todd Poynor <[e-mail]> wrote:
> On Thu, Jul 28, 2011 at 10:30:15AM +0200, [e-mail] wrote:
> ...
>> +int pwrdm_set_wkup_lat_constraint(struct powerdomain *pwrdm, void *cookie,
>> +                               long min_latency)
>> +{
>> +     struct pwrdm_wkup_constraints_entry *user = NULL;
>> +     struct pwrdm_wkup_constraints_entry *tmp_user, *new_user;
>> +     int ret = 0, free_new_user = 0, free_node = 0;
>> +     long value = 0;
>> +     unsigned long flags;
>> +
>> +     pr_debug("powerdomain: %s: pwrdm %s, cookie=0x%p, min_latency=%ld\n",
>> +              __func__, pwrdm->name, cookie, min_latency);
>> +
>> +     new_user = kzalloc(sizeof(struct pwrdm_wkup_constraints_entry),
>> +                        GFP_KERNEL);
>> +     if (!new_user) {
>> +             pr_err("%s: FATAL ERROR: kzalloc failed\n", __func__);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     spin_lock_irqsave(&pwrdm->wkup_lat_plist_lock, flags);
>> +
>> +     /* Check if there already is a constraint for cookie */
>> +     plist_for_each_entry(tmp_user, &pwrdm->wkup_lat_plist_head, node) {
>> +             if (tmp_user->cookie == cookie) {
>> +                     user = tmp_user;
>> +                     free_new_user = 1;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE) {
>> +             /* If nothing to update, job done */
>> +             if (user && (user->node.prio == min_latency))
>> +                     goto exit_ok;
>> +
>> +             if (!user) {
>> +                     /* Add new entry to the list */
>> +                     user = new_user;
>> +                     user->cookie = cookie;
>> +             } else {
>> +                     /* Update existing entry */
>> +                     plist_del(&user->node, &pwrdm->wkup_lat_plist_head);
>> +             }
>> +
>> +             plist_node_init(&user->node, min_latency);
>> +             plist_add(&user->node, &pwrdm->wkup_lat_plist_head);
>> +     } else {
>> +             /* Remove the constraint from the list */
>> +             if (!user) {
>> +                     pr_err("%s: Error: no prior constraint to release\n",
>> +                            __func__);
>> +                     ret = -EINVAL;
>> +                     goto exit_error;
>> +             }
>> +
>> +             plist_del(&user->node, &pwrdm->wkup_lat_plist_head);
>> +             free_node = 1;
>
> All min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE paths need
> free_new_user = 1.
free_new_user = 1 is only needed if no existing constraint has been
found, i.e. user stays at NULL. This is implemented in the check for
an existing constraint (plist_for_each_entry(...)).

>(Or maybe change the logic to check user !=
> new_user and free new_user if so.)
That could be done as well.

>
>> +     }
>> +
>> +exit_ok:
>> +     /* Find the strongest constraint from the list */
>> +     if (!plist_head_empty(&pwrdm->wkup_lat_plist_head))
>> +             value = plist_first(&pwrdm->wkup_lat_plist_head)->prio;
>> +
>> +     spin_unlock_irqrestore(&pwrdm->wkup_lat_plist_lock, flags);
>> +
>> +     if (free_node)
>> +             kfree(user);
>> +
>> +     if (free_new_user)
>> +             kfree(new_user);
>> +
>> +     /* Apply the constraint to the pwrdm */
>> +     pr_debug("powerdomain: %s: pwrdm %s, value=%ld\n",
>> +              __func__, pwrdm->name, value);
>> +     pwrdm_wakeuplat_update_pwrst(pwrdm, value);
>> +
>> +     return 0;
>> +
>> +exit_error:
>> +     spin_unlock_irqrestore(&pwrdm->wkup_lat_plist_lock, flags);
>
> Need:
>        kfree(new_user);
Correct!
BTW I have a new version (patch here below) that improves the cases
with latency = PM_QOS_DEV_LAT_DEFAULT_VALUE. This will come in v4
after testing.

>
>> +     return ret;
>> +}
>
>
> Todd
>

Thanks for reviewing!

Regards,
Jean

---
Patch for cases with latency = PM_QOS_DEV_LAT_DEFAULT_VALUE:

  * Programs a new target state if it is different from current power state.
@@ -232,7 +232,7 @@ static int pwrdm_wakeuplat_update_pwrst(struct
powerdomain *pwrdm,

 	/* Find power state with wakeup latency < minimum constraint */
 	for (new_state = 0x0; new_state < PWRDM_MAX_PWRSTS; new_state++) {
-		if (min_latency == 0 ||
+		if (min_latency == PM_QOS_DEV_LAT_DEFAULT_VALUE ||
 		    pwrdm->wakeup_lat[new_state] <= min_latency)
 			break;
 	}
@@ -1018,8 +1018,8 @@ int pwrdm_post_transition(void)
  * @pwrdm: struct powerdomain * which the constraint applies to
  * @cookie: constraint identifier, used for tracking.
  * @min_latency: minimum wakeup latency constraint (in microseconds) for
- *  the given pwrdm. The value of PM_QOS_DEV_WAKEUP_LAT_DEFAULT_VALUE
- *  removes the constraint.
+ *  the given pwrdm. The value of PM_QOS_DEV_LAT_DEFAULT_VALUE removes
+ *  the constraint.
  *
  * Tracks the constraints by @cookie.
  * Constraint set/update: Adds a new entry to powerdomain's wake-up latency
@@ -1042,7 +1042,7 @@ int pwrdm_set_wkup_lat_constraint(struct
powerdomain *pwrdm, void *cookie,
 	struct pwrdm_wkup_constraints_entry *user = NULL;
 	struct pwrdm_wkup_constraints_entry *tmp_user, *new_user;
 	int ret = 0, free_new_user = 0, free_node = 0;
-	long value = 0;
+	long value = PM_QOS_DEV_LAT_DEFAULT_VALUE;
 	unsigned long flags;

 	pr_debug("powerdomain: %s: pwrdm %s, cookie=0x%p, min_latency=%ld\n",
@@ -1083,16 +1083,17 @@ int pwrdm_set_wkup_lat_constraint(struct
powerdomain *pwrdm, void *cookie,
 		plist_node_init(&user->node, min_latency);
 		plist_add(&user->node, &pwrdm->wkup_lat_plist_head);
 	} else {
-		/* Remove the constraint from the list */
-		if (!user) {
-			pr_err("%s: Error: no prior constraint to release\n",
-			       __func__);
+		if (user) {
+			/* Remove the constraint from the list */
+			plist_del(&user->node, &pwrdm->wkup_lat_plist_head);
+			free_node = 1;
+		} else {
+			/* Constraint not existing or list empty, do nothing */
+			free_new_user = 1;
 			ret = -EINVAL;
 			goto exit_error;
 		}

-		plist_del(&user->node, &pwrdm->wkup_lat_plist_head);
-		free_node = 1;
 	}

 exit_ok:
@@ -1117,6 +1118,10 @@ exit_ok:

 exit_error:
 	spin_unlock_irqrestore(&pwrdm->wkup_lat_plist_lock, flags);
+
+	if (free_new_user)
+		kfree(new_user);
+
 	return ret;
 }

Comments

Todd Poynor July 29, 2011, 6 p.m. UTC | #1

On Fri, Jul 29, 2011 at 10:47:43AM +0200, Jean Pihet wrote:
> On Fri, Jul 29, 2011 at 9:59 AM, Todd Poynor <[e-mail]> wrote:
...
> > All min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE paths need
> > free_new_user = 1.
> free_new_user = 1 is only needed if no existing constraint has been
> found, i.e. user stays at NULL. This is implemented in the check for
> an existing constraint (plist_for_each_entry(...)).

Oops, I meant to say it applies in all cases where min_latency ==
PM_QOS_DEV_LAT_DEFAULT_VALUE, since the new node is only used for
adding a constraint (and no existing constraint found).  I'd suggest
something like:

        if (min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE) {
                new_user = kzalloc(sizeof(struct pwrdm_wkup_constraints_entry),
                        GFP_KERNEL);
                <check NULL return>
                free_new_user = 1;
        }

and then set free_new_user = 0 only if no existing constraint is found
for the add case.  Because it's easy to miss cases where the
allocated memory needs to be freed when that's not the default, and you
might as well skip the allocate on a constraint removal.  Pretty minor
point, though.


Todd

Jean Pihet Aug. 11, 2011, 3:09 p.m. UTC | #2

Hi Todd,

On Fri, Jul 29, 2011 at 8:00 PM, Todd Poynor <[e-mail]> wrote:
> On Fri, Jul 29, 2011 at 10:47:43AM +0200, Jean Pihet wrote:
>> On Fri, Jul 29, 2011 at 9:59 AM, Todd Poynor <[e-mail]> wrote:
> ...
>> > All min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE paths need
>> > free_new_user = 1.
>> free_new_user = 1 is only needed if no existing constraint has been
>> found, i.e. user stays at NULL. This is implemented in the check for
>> an existing constraint (plist_for_each_entry(...)).
>
> Oops, I meant to say it applies in all cases where min_latency ==
> PM_QOS_DEV_LAT_DEFAULT_VALUE, since the new node is only used for
> adding a constraint (and no existing constraint found).  I'd suggest
> something like:
>
>        if (min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE) {
>                new_user = kzalloc(sizeof(struct pwrdm_wkup_constraints_entry),
>                        GFP_KERNEL);
>                <check NULL return>
>                free_new_user = 1;
>        }
>
> and then set free_new_user = 0 only if no existing constraint is found
> for the add case.  Because it's easy to miss cases where the
> allocated memory needs to be freed when that's not the default, and you
> might as well skip the allocate on a constraint removal.  Pretty minor
> point, though.

This has been addressed in the latest patch set (v4).

Regards,
Jean

>
>
> Todd
>

Patch

diff --git a/arch/arm/mach-omap2/powerdomain.c
b/arch/arm/mach-omap2/powerdomain.c
index c0f48ab..2e9379b 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -206,7 +206,7 @@  static int _pwrdm_post_transition_cb(struct
powerdomain *pwrdm, void *unused)
  * pwrdm_wakeuplat_update_pwrst - Update power domain power state if needed
  * @pwrdm: struct powerdomain * to which requesting device belongs to.
  * @min_latency: the allowed wake-up latency for the given power domain. A
- *  value of 0 means 'no constraint' on the pwrdm.
+ *  value of PM_QOS_DEV_LAT_DEFAULT_VALUE means 'no constraint' on the pwrdm.
  *
  * Finds the power domain next power state that fulfills the constraint.