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

[Stefan Kerkmann <s.kerkmann@pengutronix.de>]

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.

Regards,
Stefan



-- 
Pengutronix e.K.                       | Stefan Kerkmann             |
Steuerwalder Str. 21                   | https://www.pengutronix.de/ |
31137 Hildesheim, Germany              | Phone: +49-5121-206917-128  |
Amtsgericht Hildesheim, HRA 2686       | Fax:   +49-5121-206917-9    |