Re: [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe

[Cornelia Huck <cohuck@redhat.com>]

On Tue, 5 Dec 2017 19:57:10 +0800
weiping zhang <zhangweiping@didichuxing.com> 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@didichuxing.com>
> ---
>  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.]