Re: context_loss_count error value

[Kevin Hilman <khilman@ti.com>]

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

> On Wed, 2011-05-25 at 11:34 -0700, Kevin Hilman wrote:
>> 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!
>
> <snip>
>
>> > @@ -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.
>
> When count is INT_MAX and one is added to it, it'll wrap to INT_MIN,
> i.e. maximum negative value, which would be an error value. So by
> masking out the highest bit we'll get nonnegative count range from 0 to
> INT_MAX.
>
> Perhaps a comment would be justified here =).

Indeed, and using INT_MAX instead of the hard-coded constants would help
readability also.

Thanks,

Kevin