On Saturday 02 May 2009, Jean Delvare wrote: > 1* In i2c_device_uevent() there is the following piece of code: > > /* by definition, legacy drivers can't hotplug */ > if (dev->driver) > return 0; > > David, I presume this can go away? Legacy drivers are going away, so ... yes. :) > 2* Core locking should be checked. I am not completely sure what > core_lock was supposed to protect, nor what it should protect now. I > think it should protect at least i2c_adapter_idr, driver->detect() > calls (which may add a device to the driver->clients list) and > driver->attach_adapter() calls (for the same reason), although for the > latter two, a per-driver lock may be more appropriate. Protecting the IDR seems right to me. I'd have to look at the post-patches code to say more ... but for now I'll just say that locking driver->*() calls seems less desirable than just making sure the updates to clients_list are protected. You should work to minimize lock scope, so in general *drop* locks before calling out from one module to another. (Lots of nastiness comes from the other module making calls that involve more locking...) > At the moment, we also hold the lock for all of i2c_register_adapter(), > i2c_del_adapter() and i2c_del_driver(), but not i2c_register_driver(), > which seems a little inconsistent. David, do you know if we need to > serialize the calls to device_register() and device_unregister(), or if > the driver core takes care of this for us? Driver core locking should eventually suffice for everything except data structures directly managed by the I2C framework. - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
