[Skiboot] [RFC PATCH 1/6] ipmi: Introduce registration for SEL command handlers

[Andrew Jeffery]

On Wed, 19 Sep 2018, at 14:40, Stewart Smith wrote:
> Andrew Jeffery <andrew at aj.id.au> writes:
> > Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> > ---
> >  hw/ipmi/ipmi-sel.c | 110 +++++++++++++++++++++++++++++++++------------
> >  include/ipmi.h     |   5 +++
> >  2 files changed, 86 insertions(+), 29 deletions(-)
> >
> > diff --git a/hw/ipmi/ipmi-sel.c b/hw/ipmi/ipmi-sel.c
> > index eb63147bdc52..c85cd6fc781b 100644
> > --- a/hw/ipmi/ipmi-sel.c
> > +++ b/hw/ipmi/ipmi-sel.c
> > @@ -15,6 +15,10 @@
> >   */
> >  
> >  #define pr_fmt(fmt) "IPMI: " fmt
> > +#include <ccan/list/list.h>
> > +#include <ccan/str/str.h>
> > +#include <compiler.h>
> > +#include <errno.h>
> >  #include <skiboot.h>
> >  #include <stdlib.h>
> >  #include <string.h>
> > @@ -37,10 +41,14 @@
> >  #define SEL_NETFN_IBM		0x3a
> >  
> >  /* OEM SEL Commands */
> > +/* TODO: Move these to their respective source files */
> 
> FWIW I think it's okay to leave that to the future.

Right, that's why I put the TODO there - not so much for me to
address as for someone else to address at some point :)

> 
> > +int ipmi_sel_register(uint8_t oem_cmd,
> > +		      void (*fn)(uint8_t data, void *context),
> > +		      void *context)
> > +{
> > +	struct ipmi_sel_handler *handler;
> > +
> > +	handler = malloc(sizeof(*handler));
> > +	if (!handler)
> > +		return -ENOMEM;
> > +
> > +	handler->oem_cmd = oem_cmd;
> > +	handler->fn = fn;
> > +	handler->context = context;
> > +
> > +	list_add(&sel_handlers, &handler->node);
> 
> I'm tempted to say we should check that nothing in the list has the
> same oem_cmd as one already there to prevent subtle bugs.

Fair point. I don't think there's going to be any performance concern
iterating the list each time, so I'll just do that for the moment as it's
dumb and easy.

> 
> otherwise looks ok.

Great.

> 
> -- 
> Stewart Smith
> OPAL Architect, IBM.
>