[Openipmi-developer] [PATCH 3/4] ipmi: allow dynamic BMC version information

[Jeremy Kerr]

Hi Corey,

> In all, this looks good.  I have two minor nits inline below, and
> two more major comments here:
> 
> I would prefer if it always queried the data, even if the device id
> information is provided by the lower level driver.

OK, can do. I was concerned that the SMI driver may want to provide its
own device identification details (and not want to override those with
the ones provided by Get Device Id), but if that's not the case then it
makes sense to store the original values but allow requery.

> Since the values can change while you are querying them, do
> we need some sort of mutex on them?

Which values are you referring to here? The device ID, or the members of
struct bmc_device?

If it's the latter, I'd be happy to take your guidance on the locking
protocol there. Is there an existing mutex that would be suitable for
that?

>From the comments inline:

> > +    /* Do we have enough data to parse the device ID details? This doesn't
> > +     * inclde the optional auxilliary version data. */
> 
> Minor nit: include is misspelled. 

D'oh, will fix in v2.

> > @@ -2450,6 +2590,8 @@ static int ipmi_bmc_register(ipmi_smi_t intf, int ifnum)
> >   
> >   	mutex_lock(&ipmidriver_mutex);
> >   
> > +	bmc_update_device_id(bmc);
> > +
> 
> What happens if this fails?  You can return an error from this function.
> It's also probably not necessary to have this inside the mutex.

I didn't think we'd want to fail registration on query failure there; it
could be a transient error.

Cheers,


Jeremy