On 09/13/2016 05:14 AM, Jeremy Kerr wrote: > 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.
The values are used at startup time, but after that it's just for querying. >> 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? The device id, manufacturer id, and product id should never change and should be atomic, anyway. The firmware revision is two separate bytes, which possibly might be used along with the device revision. aux_firmware_version is an array and there's a separate aux_firmware_revision_set bit for it, and it may be used along with the other version bytes. It's not that any single field is an issue, really, it's that something reading it might get an inconsistent view of the different data items together. But a mutex won't help there, with the possible exception of aux_firmware_revision. So never mind, it's probably not worth it. > @@ -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. The values from those fields are used in detecting that the same BMC is on different IPMI interfaces. So it does need to have valid data from this to go on. Thanks, -corey > Cheers, > > > Jeremy > > ------------------------------------------------------------------------------ > _______________________________________________ > Openipmi-developer mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/openipmi-developer ------------------------------------------------------------------------------ _______________________________________________ Openipmi-developer mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openipmi-developer
