12:49 <qschulz> Kwiboo: do you have a Gru Kevin/Bob?

12:49 <qschulz> Because I'm not entirely sure U-Boot is supposed to be self-sufficient on the Chromebooks, I've only ever read using it as a payload of coreboot

12:50 <qschulz> so I'd assume that U-Boot SPL runs in DRAM instead of SRAM?

12:50 <qschulz> (I have a Gru Bob, but it's still running ChromeOS /me hears the boos)

13:30 <diederik> macromorgan: as the author of the original code, what do you think about this patch? https://paste.debian.net/1312806/

13:33 <diederik> I copied the last 2 lines to show that in your code you used `* 1000` in one part and `/ 1000` in another, which I assume was deliberate

13:34 <diederik> That patch is floating around for PineTab2, but maybe it ought to be upstream

15:06 <macromorgan> diederik: I believe you, but do you have an example of this occuring? I want to try and troubleshoot it a bit.

15:07 <macromorgan> basically I want to protect against 3 things... unsigned overflows, values less than logical, and values more than logical

15:07 <macromorgan> I figured that code I wrote would do all 3, but I guess it isn't

15:08 <CounterPillow> haven't read the code much but the kernel explicitly has helper macros to deal with overflow checking multiplies

15:09 <macromorgan> while somewhat arbitrary, I didn't figure we'd have batteries smaller than 500ma (I honestly don't remember but I think we can't even tell the columb counter about such batteries)

15:10 <macromorgan> right...and we're putting a 24bit unsigned int into a signed int... aren't signed ints 31?

15:10 <diederik> AFAIK the battery in the PineTab2 sometimes gave 'weird' values and that patch apparently fixed that. It was mostly the 'integer overflow' part which made me bring it to your attention as that's genrally not good

15:10 <macromorgan> yes

15:10 <macromorgan> I wonder how big the battery is in that, maybe it's bigger than I thought it could be

15:10 <CounterPillow> in this case using mult_frac from math.h would seem appropriate

15:11 <diederik> from the PineTab2 wiki page: Battery: 6000 mAh (22.2Wh)

15:12 <macromorgan> also the /1000 and *1000 is because the precision in the registers is done in mah not uah like the power infra expects

15:12 <macromorgan> the registers are actually arbitrary (we have more than enough nvram to store uah values), but BSP did it that way and I didn't want to break compatibility

15:13 <macromorgan> okay, I have an errand to run but I'll look at this a bit later today

15:17 <diederik> My remark with `/ 1000` or `* 1000` was that on line 718 you did `* 1000` but on line 722 you did `/ 1000` and the patch changed that to using `/ 1000` in both cases

15:21 <CounterPillow> If this is a case of an integer overflow, then overflows in calculations of different sized ints in a comparison go deep into the woods of the C standard integer promotion rules https://en.cppreference.com/w/c/language/conversion#Usual_arithmetic_conversions

15:21 <CounterPillow> Shuffling around which side of the equation you do multiply/divide on is kind of an icky "fix" in that case.

15:29 <diederik> good point :)

15:30 <CounterPillow> might be worth it to just do everything in 64 bit ints and search the kernel source for some overflow-checking multiply macro

15:32 <CounterPillow> heyyyyy https://www.kernel.org/doc/html/latest/core-api/kernel-api.html?highlight=overflow#c.check_mul_overflow

15:34 <qschulz> diederik: there are other places where charger->bat_charge_full_design_uah / 1000 is used

15:35 <qschulz> so i guess the division is deemed safe (or the same mistake is made in multiple places)

16:21 <diederik> qschulz: yeah, I guess that's the reason for the change. But I agree with CP that it's an icky fix

16:22 <diederik> My eye caught it as some kind of anomaly. I'd expect the same construct everywhere, unless there's a reason to divert from it

16:23 <diederik> Which I expect there is; I just didn't know the reason

16:23 <qschulz> diederik: then all the division and multiplication in that driver should be checked and use the proper function :)

16:24 <diederik> Sounds reasonable :)

16:33 <macromorgan> in theory I think we always know (given the use of u8 reges) the maximum size of stuff. My question though is if our reg we're reading is 3 bytes, how does it overflow when cast into a 4 byte signed int? I'd love to see the case that triggers the error is all, I'm having a hard time understanding it (again I trust you that there IS a problem, I just don't understand it currently)

16:34 <macromorgan> actually hang on

16:34 <qschulz> macromorgan: you get a u24 and if you multiply it by 1000, it's more than what a u32 can contain

16:34 <macromorgan> yeah

16:34 <qschulz> macromorgan: >>> (1<<32)-1

16:34 <qschulz> 4294967295

16:34 <qschulz> >>> ((1<<24)-1) * 1000

16:34 <macromorgan> I see that now... duh

16:34 <qschulz> 16777215000

16:34 <macromorgan> had to break out the calculator, thank you, I'll look

16:34 <qschulz> macromorgan: what do you think I did just now :D

16:36 <diederik> I haven't seen it myself and the author of that patch has a PineTab2 v0.1 IIRC (not v2.0 like most), so it could be a pre-production error

16:45 <macromorgan> so tl;dr a battery larger than 21Ah will cause the overflow

16:47 <macromorgan> let me confirm maximum supported in the datasheet... we'll either sanity check for the absolute maximum or instead check for overflows... both situations should prevent overflows

16:51 <macromorgan> ugg... okay, this is a bigger problem I guess I just never saw with smaller batteries

16:51 <macromorgan> who has a PineTab2 so I can ping them?

16:52 <diederik> I have one (4GB RAM/64GB eMMC)

16:52 <macromorgan> I have a few values I should probably work on as long long int

16:53 <macromorgan> okay, let me tinker with some values

16:53 <macromorgan> first I'll try to identify the potental places where an overflow can occur

17:02 <Segfault_0> hey i see there's discussion going on about the rk817 sanity check patch :P

17:03 <diederik> o/ (Segfault_0 is the author of that patch)

17:04 <Segfault_0> i don't remember much of the details and the tablet with corrupted nvram wasn't mine so i wasn't able to get much info but i do know without the change to the sanity check the driver was reporting POWER_SUPPLY_CHARGE_FULL=-424144184

17:06 <Segfault_0> i'm fairly sure there was an overflow going on but with how little info i had i wasn't able to check for sure so if there's something else happening instead that wouldn't surprise me lol

17:12 <macromorgan> yeah, that's definitely an overflow

17:13 <macromorgan> I'm trying to check if the code elsewhere does any overflows, while I see a bunch of spots where it COULD happen it looks like it takes really dumb values to get there. Like a sleep enter current of 63 amps

17:14 <macromorgan> or if the max amp hours is 10000, which also seems ridiculously high for this device

17:14 <macromorgan> so no using your RK817 to power your electric vehicles...

17:14 <macromorgan> looking at the nvram code now though, I know at one point I had a few overflows there but I thought I fixed them all

17:17 <macromorgan> maybe instead we just do (charger->fcc_mah > (charger->bat_charge_full_design_uah / 1000))

17:17 <macromorgan> basically does the same math, but gives us more room to breathe

17:18 <macromorgan> and allows us to hold 4000A, which seems like more than enough without having to do any additional changes or fancy math

17:19 <macromorgan> Segfault_0: did you already submit this patch upstream? If not I can do it.

17:20 <CounterPillow> macromorgan: please don't do this, use the checked overflow functions I linked

17:20 <CounterPillow> Or I guess if you're just dividing and everything is 32 bit it's fine

17:21 <CounterPillow> that is, if everything is unsigned

17:21 <CounterPillow> But I am uneasy about garbage in nvram producing negative numbers if these are signed ints

17:21 <diederik> Segfault didn't submit the patch upstream

17:22 <macromorgan> it only happens because we have 24 bits, which usually is perfectly fine but when we multiply by 1000 values greater than 16384mAh cause the overflow

17:23 <macromorgan> if we omit the multiplication step we don't get an overflow until we try more than 4294967mAH which is way out of spec and a condition we shouldn't encounter

17:23 <CounterPillow> where does the value come from? If from DT, okay, if from nvram, not okay

17:24 <macromorgan> checking for overflows is good... in my view making them impossible is better

17:24 <macromorgan> the 24 bit value is from nvram

17:24 <macromorgan> if we don't multiply by 1000 there's no way to get a 24 bit number to overflow

17:24 <CounterPillow> if the maximum number you can store in nvram does not cause an overflow then that's fine

17:24 <macromorgan> in a 32 bit int

17:24 <macromorgan> yes, the maximum is 24 bits

17:25 <macromorgan> we read 3 u8 values with regmap_bulk_read() to get that value

17:26 <CounterPillow> Make sure to an a -EINVAL return condition if it ever goes larger than the max int of 24 bits, because someone might change that code down the line and break the invariant :P

17:27 <macromorgan> I'm going to do one better and try to hard limit the acceptable values, plus i'll comment that function

17:27 <macromorgan> the 24bits is to preserve backwards compatibility with the BSP, otherwise I'd have done it different

17:28 <CounterPillow> Yeah that's fair

17:53 <macromorgan> okay... I'm not seeing anywhere else where an overflow can occur until you have a battery with more than 42Ah, which is about 4 times the size of a high capacity laptop battery. It will take a fair amount of work to prevent that from happening.

17:54 <macromorgan> For now it looks like the only opportunity for an overflow (aside from a fully charged 42Ah battery) is reading a value greater than 16M from the nvram

17:54 <macromorgan> I'll push a fix. Segfault_0, is that your name/email in this patch? https://paste.debian.net/1312806/

17:56 <diederik> Seems like he left already, so I'll answer it: yes

18:17 <macromorgan> okay, I see two potential fixes that are needed. One to sanity check values before we write to nvram, another to improve the sanity check of what we read back.

18:21 <macromorgan> going to have to test it out now, but I'll at least start with the obvious that was reported upstream