Tomi Valkeinen <tomi.valkeinen@xxxxxx> writes:
> On Wed, 2011-05-25 at 11:34 -0700, Kevin Hilman wrote:
>> Tomi Valkeinen <tomi.valkeinen@xxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
