[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?
> 
> Yeah we can do that.. But _opal_queue_msg() is used in many places
> directly.
> So we have to change all places to pass actual size.. something like
> below :
> 
> diff --git a/core/hmi.c b/core/hmi.c
> index e81328600..ab079a95a 100644
> --- a/core/hmi.c
> +++ b/core/hmi.c
> @@ -337,7 +337,7 @@ static int queue_hmi_event(struct OpalHMIEvent
> *hmi_evt, int 
> recover, uint64_t *
> 
>          /* queue up for delivery to host. */
>          return _opal_queue_msg(OPAL_MSG_HMI_EVT, NULL, NULL,
> -                               num_params, (uint64_t *)hmi_evt);
> +                               num_params * sizeof(u64), (uint64_t
> *)hmi_evt);
>   }
> 
> To avoid these changes I created added function to pass buffer 
> (opal_queue_msg_buf()).
> 
> If you still want I can change this. Please let me know.

I think it'd be simpler to reduce the number of functions here,
especially as the difference between them is so subtle.

But as you say, it's not super important.

> > > @@ -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?
> 
> This is bit tricky one. In many places its just return code -OR-  some
> values queued using opal_queue_msg.

I'm not sure I understand? Do you mean that not all callsites (ie, in
the "just return code" case) are guaranteeing that their buffers remain
valid until the callback?

This sounds like a source of future bugs. For example, if we ever change
the threshold between the two methods of message queuing.

Can we just use the memcpy() implementation and only have the one type
of params storage instead?

> > Minor (but also on other patches in this series): reverse christmas-
> > tree?
> 
> Not sure what you meant here. you mean variable order ?

Yeah, this seems to be an ad-hoc convention that we've adopted, where
the longer declarations go first:

I'm not sure if anyone else is particularly fussed about it.

Cheers,


Jeremy