[Cbe-oss-dev] [PATCH 10/10 V2] MARS: workload type move from context to queue block

[Yuji Mano]

Geoff Levand wrote:
> Yuji Mano wrote:
>> Make the workload context a completely abstract container.
>> Previously the workload type was part of the workload context, but this moves
>> the type into the inherited workload context (task context).
>> 
>> Signed-off-by: Yuji Mano <yuji.mano at am.sony.com>
>> 
>> ---
>> Changes since V1:
>>  - remove unnecessary pad in task context
>>  - rename pad to context in workload context
> 
> No mention of my comments on workload_queue_add() and
> mars_workload_to_task()...
> 
>> @@ -47,7 +47,7 @@ extern "C" {
>>  int workload_queue_initialize(struct mars_workload_queue *queue);
>>  int workload_queue_finalize(struct mars_workload_queue *queue);
>>  int workload_queue_add(struct mars_workload_queue *queue, uint16_t *id,
>> -			struct mars_workload_context *workload);
>> +			struct mars_workload_context *workload, uint8_t type);
> 
> The workload arg to workload_queue_add() will always need to be cast, so
> is meaningless, and so should just be a void* type.

Why make it void when the function is expecting a pointer to a mars workload
context or some derived structure? Even though the caller needs to cast it,
at least it's more obvious what they need to pass in. I am thinking similar
to C++ derived classes where struct mars_workload_context is the base class.
It makes perfect sense in C++ to pass the base class pointer...why not here?

>>  int workload_queue_remove(struct mars_workload_queue *queue, uint16_t id,
>>  			struct mars_workload_context *workload);
>>  int workload_queue_context(struct mars_workload_queue *queue, uint16_t id,
>> --- a/include/mpu/mars/mars_kernel.h
>> +++ b/include/mpu/mars/mars_kernel.h
> 
> 
>> @@ -65,9 +66,9 @@ void workload_run(void)
>>  	workload_state = MARS_WORKLOAD_STATE_RUNNING;
>>  
>>  	/* workload type specific handling */
>> -	switch(workload.type) {
>> +	switch(workload_type) {
>>  	case MARS_WORKLOAD_TYPE_TASK:
>> -		task_run(&workload.type_context.task);
>> +		task_run((struct mars_task_context *)&workload);
> 
> In general, casts in open code like this are undesirable.  Make a
> mars_workload_to_task() or some such routine.

I'm not sure if having an inline function that does the same thing is any
better. The mars_workload_context is a purely abstract container and should
never change or need to change. The workload context structure and task
context structure should be referring to the same thing, the only difference
being that the task context has member definitions. 

Is it also undesirable to cast a uint64_t ea to a struct pointer?

Either way I think if your recommended changes are to be made they are
better off in a separate patch as they may span out to other files and is
part of a more general abstraction clean up.

Regards,
Yuji