wchfixes

[robertlipe]

• Specify full paramater definition in decl. C89's 'implicit int' is an error here.
• Allow a clean build on systems with warnings.

[karlp]

Given that the entire structure of the WCH support is not upstreamable, I'm not really interested in taking a bunch of whitespace cleanups, when it will make it harder to track merges from newer MRS releases.

[robertlipe]

OK. Cherry pick out the first couple of commits. They're layered/ordered that way for a reason.

…

On Sat, Aug 6, 2022 at 4:50 AM Karl Palsson ***@***.***> wrote: Given that the entire structure of the WCH support is not upstreamable, I'm not *really* interested in taking a bunch of whitespace cleanups, when it will make it harder to track merges from newer MRS releases. — Reply to this email directly, view it on GitHub <#3 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD37O2YE3K5LPLIY4VXTVXYYNLANCNFSM55XYZDIQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[karlp]

Without debug docs from WCH, we don't know if this is wrong or right. It might only allow a 32bit read, even if they are only interested in the lower 16bits. Is the code gross? yes! is this code actually wrong? we have no idea!

[robertlipe]

The existing code is definitely wrong. It is writing 32 bits into a pointer holding a 16 bit variable. That's definitely an out-of-bounds write. This is another case where the warnings highlighted an actual bug. If you want to preserve the possibility that they have a target_read_u16 that doesn't actually work, you can do something like: uint32_t temp; int retval = target_read_u32(target, 0x1ffff7e0, &temp); *flash_size_in_kb = temp; That preserves existing behaviour and doesn't scribble on random RAM. Upon reflection, it's not a synchronization problem because target_read_u32/u16 are seekable; it's not like there's a file pointer that's now being advanced less far in a file being read or something. And who designs something in 2022 that'll silently overflow if ever there's a device with 64MB of flash? It's not like that's a crazy large amount of flash... But again, that's beyond what we're here to fix. :-)

…

On Sat, Aug 6, 2022 at 1:37 PM Karl Palsson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/flash/nor/wcharm.c <#3 (comment)>: > @@ -510,9 +513,13 @@ static int ch32x_get_device_id(struct flash_bank *bank, uint32_t *device_id) static int ch32x_get_flash_size(struct flash_bank *bank, uint16_t *flash_size_in_kb) { struct target *target = bank->target; - uint32_t cpuid, flash_size_reg; - uint32_t temp; - int retval = target_read_u32(target, 0x1ffff7e0, flash_size_in_kb); + // FIXME: this is almost certainly an actual bug and I'm applying pain killer + // to it. + // target_read_u32 reads 32 bits and stores it in the pointee of the final arg. + // The final argument here is a uint16_t as it is in the caller. I'm going to + // change this to read 16 bits, but this could cause synchronization issues. + // Orig: int retval = target_read_u32(target, 0x1ffff7e0, flash_size_in_kb); + int retval = target_read_u16(target, 0x1ffff7e0, flash_size_in_kb); Without debug docs from WCH, we don't know if this is wrong or right. It might only allow a 32bit read, even if they are only interested in the lower 16bits. Is the code gross? yes! is this code actually wrong? we have no idea! — Reply to this email directly, view it on GitHub <#3 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD374EZ6JB6JUP2B6DSLVX2WHXANCNFSM55XYZDIQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[karlp]

right, yes, I missed that the u16 pointer got converted to a u32 pointer first. I thought it was just writing a 32bit value to a 16bit pointer, and it was just discarding the top16 bits, which is dumb, but fine, and would be valid if (for instance) their target_read_16 was busted.