2017-12-06 19:11 GMT+08:00 Cornelia Huck <cohuck@xxxxxxxxxx>: > 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.) > that's true, so we don't free it until it's refer count decrease to 0 and ->release called. >> + 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. > I prefer go second way. I'll send v3 later. > [Of course, I still might be missing some devm subtility, so other > comments are welcome.] -- Thanks very much weiping > _______________________________________________ > Virtualization mailing list > Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx > https://lists.linuxfoundation.org/mailman/listinfo/virtualization _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
