[Skiboot] [PATCH v2 3/9] opal-msg: Enhance opal-get-msg API

[Jeremy Kerr]

Hi Vasant,

> -int _opal_queue_msg(enum opal_msg_type msg_type, void *data,
> -		    void (*consumed)(void *data, int status),
> -		    size_t num_params, const u64 *params)
> +static int __opal_queue_msg(enum opal_msg_type msg_type, void *data,
> +			    void (*consumed)(void *data, int status),
> +			    size_t params_size, const void *params_data)

Why add the new __opal_queue_msg(), when we have _opal_queue_msg()?
Can't we just remove the "/sizeof(u64)" from the opal_queue_msg macro
instead, and use _opal_queue_msg() for the low-level function?

> @@ -56,12 +62,14 @@ int _opal_queue_msg(enum opal_msg_type msg_type,
> void *data,
>  	entry->consumed = consumed;
>  	entry->data = data;
>  	entry->msg.msg_type = cpu_to_be32(msg_type);
> +	entry->msg.size = cpu_to_be32(params_size);
>  
> -	if (num_params > ARRAY_SIZE(entry->msg.params)) {
> -		prerror("Discarding extra parameters\n");
> -		num_params = ARRAY_SIZE(entry->msg.params);
> +	if (params_size > OPAL_MSG_FIXED_PARAMS_SIZE) {
> +		/* msg.params[0] contains address of data */
> +		entry->msg.params[0] = (uint64_t)params_data;
> +	} else {
> +		memcpy(entry->msg.params, params_data, params_size);
>  	}

This concerns me a bit - now we have two different "formats" of message
on the msg_pending_list, and it looks like there are now two different
lifetime requirements on the params_data argument, depending on the
size.

Could we unify this?

> -	memcpy(entry->msg.params, params, num_params*sizeof(u64));
>  
>  	list_add_tail(&msg_pending_list, &entry->link);
>  	opal_update_pending_evt(OPAL_EVENT_MSG_PENDING,
> @@ -72,8 +80,29 @@ int _opal_queue_msg(enum opal_msg_type msg_type,
> void *data,
>  	return 0;
>  }
>  
> +int _opal_queue_msg(enum opal_msg_type msg_type, void *data,
> +		    void (*consumed)(void *data, int status),
> +		    size_t num_params, const u64 *params)
> +{
> +	/* Convert num_params to actual size */
> +	return __opal_queue_msg(msg_type, data, consumed,
> +				(num_params * sizeof(u64)), params);
> +}
> +
> +int opal_queue_msg_buf(enum opal_msg_type msg_type, void *data,
> +		       void (*consumed)(void *data, int status),
> +		       size_t params_size, const void *params_data)
> +{
> +	return __opal_queue_msg(msg_type, data,
> +				consumed, params_size, params_data);
> +}
> +
>  static int64_t opal_get_msg(uint64_t *buffer, uint64_t size)
>  {
> +	int rc = OPAL_SUCCESS;
> +	int params_size;
> +	uint64_t msg_size;
> +	struct opal_msg *msg = (struct opal_msg *)buffer;

Minor (but also on other patches in this series): reverse christmas-
tree?

Cheers,


Jeremy