Hello Dmitry, First of all, thanks for your comments.
On 02/26/2013 11:28 PM, Dmitry Torokhov wrote: > On Tue, Feb 26, 2013 at 10:03:15AM +0100, Samuel Iglesias Gonsalvez wrote: >> put_device() must be called after device_register() fails, >> since device_register() always initializes the refcount >> on the device structure to one. >> >> dev->id is free'd inside of ipack_device_release function. >> So, it's not needed to do it here. >> >> Signed-off-by: Samuel Iglesias Gonsalvez <sigles...@igalia.com> >> --- >> drivers/ipack/ipack.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/ipack/ipack.c b/drivers/ipack/ipack.c >> index 7ec6b20..3588ccf 100644 >> --- a/drivers/ipack/ipack.c >> +++ b/drivers/ipack/ipack.c >> @@ -449,7 +449,7 @@ int ipack_device_register(struct ipack_device *dev) >> >> ret = device_register(&dev->dev); >> if (ret < 0) >> - kfree(dev->id); >> + put_device(&dev->dev); > > Now callers have no idea if they have to free (put_device) it in case of > failure or it is already done for them, because a few lines earlier, if > pack_device_read_id() fails, it simply returns error code. > > IMO if a function did not allocate object it should not try to free it, > callers should dispose of the device as they see fit. > > What is missing, however, is dev->id = NULL assignment so that if caller > does put_device() kit won't double-free the memory. OK. I will write a patch. > Also it would make > sense to split device_register into device_init() and device_add() so > that device_init() is very first thing you do and in case of all > failures callers should use put_device(). > It makes sense. I will write a patch for it. > Also tpci200.c need to learn to clean up after itself and I also not > sure why it tries to provide its own release function. > tpci200 driver is the one who allocated the struct ipack_device and it should free it. The path to release it is the following: * The kernel will call the corresponding release() function of the struct device. This callback is linked to ipack_release_device() in ipack.c. In that function, there is a kfree(dev->id) as it was previously allocated by ipack bus driver. * Then, ipack_release_device() calls the carrier's release function to indicate that the device is being released and the carrier driver needs to take care of its own resources. In summary, each driver allocate and free its own resources when unregistering a device. Thanks, Sam -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/