Messages in this thread | | | Date | Tue, 1 Dec 2020 10:36:14 +0000 | From | Ionela Voinescu <> | Subject | Re: [PATCH 2/5] thermal: devfreq_cooling: get a copy of device status |
| |
Hi,
Sorry for the delay and for the noise on this older version. I first want to understand the code better.
On Thursday 22 Oct 2020 at 11:55:28 (+0100), Lukasz Luba wrote: [..] > > > > > > +{ > > > + /* Make some space if needed */ > > > + if (status->busy_time > 0xffff) { > > > + status->busy_time >>= 10; > > > + status->total_time >>= 10; > > > + } > > > > How about removing the above code and adding here: > > > > status->busy_time = status->busy_time ? : 1; > > It's not equivalent. The code operates on raw device values, which > might be big (e.g. read from counters). If it's lager than the 0xffff, > it is going to be shifted to get smaller. >
Yes, the big values are handled below through the division and by making total_time = 1024. These two initial checks are only to cover the possibility for busy_time and total_time being 0, or busy_time > total_time.
> > > > > + > > > + if (status->busy_time > status->total_time) > > > > This check would then cover the possibility that total_time is 0. > > > > > + status->busy_time = status->total_time; > > > > But a reversal is needed here: > > status->total_time = status->busy_time; > > No, I want to clamp the busy_time, which should not be bigger that > total time. It could happen when we deal with 'raw' values from device > counters. >
Yes, I understand. But isn't making total_time = busy_time accomplishing the same thing?
> > > > > + > > > + status->busy_time *= 100; > > > + status->busy_time /= status->total_time ? : 1; > > > + > > > + /* Avoid division by 0 */ > > > + status->busy_time = status->busy_time ? : 1; > > > + status->total_time = 100; > > > > Then all of this code can be replaced by: > > > > status->busy_time = (unsigned long)div64_u64((u64)status->busy_time << 10, > > status->total_time); > > status->total_time = 1 << 10; > > No, the total_time closed to 'unsigned long' would overflow. >
I'm not sure I understand. total_time gets a value of 1024, it's not itself shifted by 10.
> > > > This way you gain some resolution to busy_time and the divisions in the > > callers would just become shifts by 10. > > > I don't want to gain more resolution here. I want to be prepare for raw > (not processed yet) big values coming from driver. >
Agreed! The higher resolution is an extra benefit. The more important benefit is that, through my suggestion, you'd be replacing all future divisions by shifts.
Thanks, Ionela.
> Regards, > Lukasz > > > > > Hope it helps, > > Ionela. > >
| |