Re: context_loss_count error value

[Kevin Hilman <khilman@ti.com>]

Tomi Valkeinen <tomi.valkeinen@ti.com> writes:

[...]

>> 
>> You're right, the code is just wrong here and would lead to strange
>> return value checking in the callers to be correct.
>> 
>> I think the best fix for this problem is to use a signed return value
>> which can wrap as expected, and then use return negative error codes
>> (e.g. -ENODEV).
>> 
>> Care to send a patch?  or do you have any other suggestions for a fix?
>
> Here's a patch. 

Thanks!

> I'm not quite sure in what situations the functions should return an
> error, but the code now returns -ENODEV if:
>
> - device pointer given by the driver to
> omap_pm_get_dev_context_loss_count() is NULL
>
> - pwrdomain pointer given to pwrdm_get_context_loss_count() is NULL,
> which should never happen, as the caller of that function already checks
> the pwrdomain pointer

Looks right.

Some comments below about wrapping.  Looks like your doing wrapping
manually?  Why is that necessary?

>  Tomi
>
>
> From 11ce3b3bd1f5aac44aae7ab05725d77bc9ca3b42 Mon Sep 17 00:00:00 2001
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date: Wed, 25 May 2011 11:06:41 +0300
> Subject: [PATCH] OMAP: change get_context_loss_count ret value to int
>
> get_context_loss_count functions return context loss count as u32, and
> zero means an error. However, zero is also returned when context has
> never been lost and could also be returned when the context loss count
> has wrapped and goes to zero.
>
> Change the functions to return an int, with negative value meaning an
> error.
>
> OMAP HSMMC code uses omap_pm_get_dev_context_loss_count(), but as the
> hsmmc code handles the returned value as an int, with negative value
> meaning an error, this patch actually fixes hsmmc code also.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>

[...]

> @@ -953,7 +953,9 @@ u32 pwrdm_get_context_loss_count(struct powerdomain *pwrdm)
>  	for (i = 0; i < pwrdm->banks; i++)
>  		count += pwrdm->ret_mem_off_counter[i];
>  
> -	pr_debug("powerdomain: %s: context loss count = %u\n",
> +	count &= 0x7fffffff;

What's this for?

> +	pr_debug("powerdomain: %s: context loss count = %d\n",
>  		 pwrdm->name, count);
>  
>  	return count;

[...]

> @@ -311,22 +311,26 @@ void omap_pm_disable_off_mode(void)
>  
>  #ifdef CONFIG_ARCH_OMAP2PLUS
>  
> -u32 omap_pm_get_dev_context_loss_count(struct device *dev)
> +int omap_pm_get_dev_context_loss_count(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> -	u32 count;
> +	int count;
>  
>  	if (WARN_ON(!dev))
> -		return 0;
> +		return -ENODEV;
>  
>  	if (dev->parent == &omap_device_parent) {
>  		count = omap_device_get_context_loss_count(pdev);
>  	} else {
>  		WARN_ONCE(off_mode_enabled, "omap_pm: using dummy context loss counter; device %s should be converted to omap_device",
>  			  dev_name(dev));
> -		if (off_mode_enabled)
> -			dummy_context_loss_counter++;
> +
>  		count = dummy_context_loss_counter;
> +
> +		if (off_mode_enabled) {
> +			count = (count + 1) & 0x7fffffff;
> +			dummy_context_loss_counter = count;
> +		}

Again, I don't think this masking is needed.   count is already an
'int', so when it gets bigger than INT_MAX, it will wrap.

Kevin