Attention is currently required from: Julius Werner, Arthur Heymans, ron minnich.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68768 )
Change subject: lib/coreboot_table: Simplify API to set up lb_serial ......................................................................
Patch Set 7:
(1 comment)
File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/68768/comment/24bc7a87_da897af4 PS7, Line 102: struct lb_serial new_serial = get_lb_serial(); : /* Use the base address as a proxy for an invalid struct */ : if (new_serial.baseaddr == 0) : return; : : serial = (struct lb_serial *)lb_new_record(header); : memcpy(serial, &new_serial, sizeof(*serial)); : serial->tag = LB_TAG_SERIAL; : serial->size = sizeof(*serial); : if (serial->type == LB_SERIAL_TYPE_IO_MAPPED) : lb_add_console(LB_TAG_CONSOLE_SERIAL8250, header); : else : lb_add_console(LB_TAG_CONSOLE_SERIAL8250MEM, header); It's true we can return a struct, but the implementation above is - using a hint baseaddr=0 implies invalid - assuming the get_lb_serial will only fill partial data - overwriting the size
If we're going to change the APIs, I'd rather redefine it as: int get_lb_serial(struct lb_serial *serial) { serial->type = ... return 0; }
And then you can do here:
struct lb_serial new_serial = {0}; new_serial.tag = LB_TAG_SERIAL; new_serial.size = sizeof(new_serial); if (!get_lb_serial(&new_serial)) return; // allocate the new record, copy serial etc