Re: [PATCH 1/2] habv4: correct habv4 rom vector table

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 12.01.24 16:26, Stefan Kerkmann wrote:
> Hello Ahmad,
> 
> On 11.01.24 16:57, Ahmad Fatoum wrote:
>> Hello Stefan,
>>
>>> diff --git a/drivers/hab/habv4.c b/drivers/hab/habv4.c
>>> index ed6d4db77c..dfa0207435 100644
>>> --- a/drivers/hab/habv4.c
>>> +++ b/drivers/hab/habv4.c
>>> @@ -144,31 +144,41 @@ struct hab_header {
>>>       uint8_t par;
>>>   } __packed;
>>>   -typedef enum hab_status hab_loader_callback_fn(void **start, uint32_t *bytes, const void *boot_data);
>>> +typedef enum hab_status hab_loader_callback_fn(void **start, size_t *bytes, const void *boot_data);
>>> +typedef void (*hab_image_entry_fn)(void);
>>
>> While it only matter for forward declaration, perhaps, change this
>> to be a non-pointer like hab_loader_callback_fn above?
> 
> Done in v2.
> 
>>> +    enum hab_status (*run_dcd)(const uint8_t *dcd);
>>> +    enum hab_status (*run_csf)(const uint8_t *csf, uint8_t cid, uint32_t srkmask);
>>>       enum hab_status (*assert)(enum hab_assertion assertion, const void *data, uint32_t count);
>>> -    enum hab_status (*report_event)(enum hab_status status, uint32_t index, void *event, uint32_t *bytes);
>>> +    enum hab_status (*report_event)(enum hab_status status, uint32_t index, uint8_t *event, size_t *bytes);
>>>       enum hab_status (*report_status)(enum hab_config *config, enum habv4_state *state);
>>>       void (*failsafe)(void);
>>> +    hab_image_entry_fn(* authenticate_image_no_dcd)(uint8_t cid, ptrdiff_t ivt_offset, void **start, size_t *bytes, hab_loader_callback_fn loader);
>>
>> And here you rely on a function pointer being automatically derived.
>> While that's ok from a correctness point of view, for symmetry, it
>> would be better to stick to one type.
> 
> Done in v2.
> 
>>> +    uint32_t(* get_version)(void);
>>
>> Nitpick: space after uint32_t and not before get_version.
>>
> 
> Done in v2.
> 
>>> +    enum hab_status (*authenticate_container)(uint8_t cid, ptrdiff_t ivt_offset, void **start, size_t *bytes, hab_loader_callback_fn loader, uint32_t srkmask, int skip_dcd);
>>>   } __packed;
>>>   -#define FSL_SIP_HAB             0xC2000007
>>
>> This is removed without replacement?
>>
> 
> No it is still there, the diff is a bit misleading.

Ah, its whitespace was changed..

> 
> Regards,
> Stefan
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |