[Skiboot] [PATCH v3 04/10] opal-msg: Enhance opal-get-msg API

[Vasant Hegde]

On 05/16/2019 12:21 PM, Jeremy Kerr wrote:
> Hi Vasant,
> 
>> - If buffer size < actual message size, then opal_get_msg will copy
>> partial data and return OPAL_PARTIAL to Linux.
> 
> This sounds a little dangerous, but I'm sure you had a good reason for
> that (rather than dropping the message). Could you remind me of that?

Ideally its not nice.. But we don't wanted to break the API. Hence we retained 
OPAL_PARTIAL.

Also with this patch, we are passing message size to kernel and returning 
OPAL_PARTIAL. So
its not that bad. Kernel will know that data is partial and it has actual size.. 
so if it wants to
use partial data, it can still do it.



> 
>> - Add new variable "extended" to "opal_msg_entry" structure to keep
>> track of messages that has more than 64byte data. We will allocate
>> separate memory for these messages and once kernel consumes message we
>> will release that memory.
> 
> OK, that scheme sounds good.
> 
>> +	if (params_size > OPAL_MSG_FIXED_PARAMS_SIZE) {
>> +		entry_size = sizeof(struct opal_msg_entry) + params_size;
>> +		entry_size -= OPAL_MSG_FIXED_PARAMS_SIZE;
>> +		entry = zalloc(entry_size);
>>
> 
> Can we set entry->extended here ...

Sure.

> 
>> +	} else {
>> +		entry = list_pop(&msg_free_list, struct opal_msg_entry, link);
>>   		if (!entry) {
>> -			prerror("Allocation failed\n");
>> -			unlock(&opal_msg_lock);
>> -			return OPAL_RESOURCE;
>> +			prerror("No available node in the free list, allocating\n");
>> +			entry = zalloc(sizeof(struct opal_msg_entry));
>>   		}
>>
>>   	}
>> +	if (!entry) {
>> +		prerror("Allocation failed\n");
>> +		unlock(&opal_msg_lock);
>> +		return OPAL_RESOURCE;
>> +	}
>>   
>>   	entry->consumed = consumed;
>>   	entry->data = data;
>>   	entry->msg.msg_type = cpu_to_be32(msg_type);
>> -
>> -	if (params_size > OPAL_MSG_FIXED_PARAMS_SIZE) {
>> -		prerror("Discarding extra parameters\n");
>> -		params_size = OPAL_MSG_FIXED_PARAMS_SIZE;
>> -	}
>> +	entry->msg.size = cpu_to_be32(params_size);
>>   	memcpy(entry->msg.params, params, params_size);
>>   
>> +	if (params_size > OPAL_MSG_FIXED_PARAMS_SIZE)
>> +		entry->extended = true;
> 
> ... rather than here, so that it's more obvious that ->extended is
> correlated to the allocation we've just used. I don't want us to miss
> that in a future change, as it'll result in a silent memory leak.

Agree. Fixed in v4.
> 
> With that changed:
> 
> Acked-by: Jeremy Kerr <jk at ozlabs.org>

Thanks!

-Vasant