[PATCH v3 4/5] discover/platform-powerpc: add mailbox message structure
Maxim Polyakov
m.polyakov at yadro.com
Thu Oct 17 21:14:59 AEDT 2019
08.10.2019 12:30, Jeremy Kerr:
> Hi Maxim,
>
> Thanks for your patience while reviewing your patches, I've had a bit of
> time to take a look now :)
Hi, Jeremy. Thanks for review
>
> This series looks good - just one thing to raise with this patch, but
> it's pretty minor:
>
>> +typedef struct __attribute__((packed)) {
>> + uint8_t iana[CHASSIS_BOOT_MBOX_IANA_SZ];
>> + uint8_t data[CHASSIS_BOOT_MBOX_BLOCK0_DATA_SZ];
>> +} mbox_block0_t;
>
> We generally don't define new typedefs in the petitboot codebase. Can we
> just use the struct names instead?
Yes, I removed all types for my structures.
>
> And do they need to be separate structs, or shoud we just have the top-
> level 'struct ipmi_mbox_response' ?
@@ -538,15 +537,15 @@ static int get_ipmi_boot_mailbox(struct platform_powerpc *platform,
* on higher numbers.
*/
for (i = 0; i < UCHAR_MAX; i++) {
- rc = get_ipmi_boot_mailbox_block(platform, block_buffer, i);
- if (rc < 3 && i == 0) {
+ int block_size = get_ipmi_boot_mailbox_block(platform, &mailbox, i);
It would be better to split the ipmi message into separate structures, since
outside the get_ipmi_boot_mailbox_block() function we use only the ipmi message
payload - mailbox data (or 'union mbox' in the code), without msg header and so on.
I would not like to change anything in the structures.
So the code is cleaner and more understandable for me.
>
> [Also, I'm fine with your original patch of keeping these in platform-
> powerpc.c, as there's no current users outside of that file. ipmi.h is
> fine though, no need to change it if you prefer it as-is].
Ok, I moved these structures back to the platform-powerpc.c file.
>
> Cheers,
>
>
> Jeremy
>
>
--
Regards,
Maxim Polyakov
Software Engineer, YADRO.
More information about the Petitboot
mailing list