Re: [PATCH v2 07/14] vfio/pci: Move VGA and VF initialization to functions

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Mar 15, 2021 at 09:45:34AM +0100, Christoph Hellwig wrote:
> > +static int vfio_pci_vf_init(struct vfio_pci_device *vdev)
> > +{
> > +	struct pci_dev *pdev = vdev->pdev;
> > +	int ret;
> > +
> > +	if (!pdev->is_physfn)
> > +		return 0;
> > +
> > +	vdev->vf_token = kzalloc(sizeof(*vdev->vf_token), GFP_KERNEL);
> > +	if (!vdev->vf_token)
> > +		return -ENOMEM;
> 
> > +static void vfio_pci_vf_uninit(struct vfio_pci_device *vdev)
> > +{
> > +	if (!vdev->vf_token)
> > +		return;
> 
> I'd really prefer to keep these checks in the callers, as it makes the
> intent of the code much more clear.  Same for the VGA side.
> 
> But in general I like these helpers.

I'm here because I needed to make the error unwind tidy before I could
re-order everything in the next patch, as re-ordering with the
existing unwind quickly became a mess.

It ends up like this:

out_power:
	if (!disable_idle_d3)
		vfio_pci_set_power_state(vdev, PCI_D0);
out_vf:
	vfio_pci_vf_uninit(vdev);
out_reflck:
	vfio_pci_reflck_put(vdev->reflck);
out_free:
	kfree(vdev->pm_save);
	kfree(vdev);

I'm always leery about adding conditionals to these unwinds, it is
easy to make a mistake.

Particularly in this case the init/uninit checks are not symmetric:

 +	if (!pdev->is_physfn)
 +		return 0;

vs

 +	if (!vdev->vf_token)
 +		return;

So the goto unwind looks quite odd when this is open coded. At least
with the helpers you can read the init then uninit and go 'yah, OK,
this makes sense'

Thanks,
Jason