On Tue, 5 Dec 2017 19:57:10 +0800 weiping zhang <zhangweiping@xxxxxxxxxxxxxxx> wrote: > As mentioned at drivers/base/core.c: > /* > * NOTE: _Never_ directly free @dev after calling this function, even > * if it returned an error! Always use put_device() to give up the > * reference initialized in this function instead. > */ > > Normal we do cleanup for @vm_dev by contianer_of(@dev), but in this case > we need release @mem resource from @pdev and vm_dev->base. It make > @pdev->vm_dev.dev.release() too complicated, so put_device just put the > reference of register_virtio_device->device_register->device_initialize > and release all resource in virtio_mmio_probe. Releasing the resources when unwinding on error can work, but I think there still are some issues (more below). This is all very tangly code :( > > Signed-off-by: weiping zhang <zhangweiping@xxxxxxxxxxxxxxx> > --- > drivers/virtio/virtio_mmio.c | 36 ++++++++++++++++++++++++++++-------- > 1 file changed, 28 insertions(+), 8 deletions(-) > > @@ -573,7 +580,20 @@ static int virtio_mmio_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, vm_dev); > > - return register_virtio_device(&vm_dev->vdev); > + rc = register_virtio_device(&vm_dev->vdev); > + if (rc) > + goto put_dev; > + return 0; > +put_dev: > + put_device(&vm_dev->vdev.dev); Here you give up the extra reference from device_initialize(), which may or may not be the last reference (since you don't know if device_add() had already exposed the struct device to other code that might have acquired a reference). As the device has an empty release function, touching the device structure after that is not a real problem, but... > +unmap: > + iounmap(vm_dev->base); > +free_mem: > + devm_release_mem_region(&pdev->dev, mem->start, > + resource_size(mem)); > +free_vmdev: > + devm_kfree(&pdev->dev, vm_dev); ...unconditionally freeing the device here would be a problem if other code had acquired a reference above. (Unlikely, but we should try to get this right.) > + return rc; > } > > static int virtio_mmio_remove(struct platform_device *pdev) So, I think there are basically two ways of doing that: - Move the cleanup into the currently empty release callback. Then, you won't need to touch the remove function. The problem with that is that you can't trigger a cleanup via put_device() if you did not call register_virtio_device() yet. - Move just devm_kfree() into the release function. Cleanup the resources here, do the put_device() last thing if had you called register_virtio_device() before and devm_kfree() if you didn't. [Of course, I still might be missing some devm subtility, so other comments are welcome.] _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
