Re: [PATCH] drm/i915: Add vgpu.entry list initialization

[Andi Shyti <andi.shyti@linux.intel.com>]

Hi Jani,

> >> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
> >> index d29a06ea51a5..362282b20f7b 100644
> >> --- a/drivers/gpu/drm/i915/i915_vgpu.c
> >> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
> >> @@ -67,6 +67,7 @@ void intel_vgpu_detect(struct drm_i915_private *dev_priv)
> >>  	u64 magic;
> >>  	u16 version_major;
> >>  	void __iomem *shared_area;
> >> +	INIT_LIST_HEAD(&dev_priv->vgpu.entry);
> >
> > Despite what Sebastian is suggesting, I think the place is right
> > (or almost right).
> >
> > But I wouldn't fix it this way. Initializing virtual GPUs is not
> > mandatory, indeed it fails only during delete that is the only
> > function that doesn't check whether the list is initialized.
> >
> > I would rather check whether the list is initialized before
> > trying to delete it, with a nice comment saying that the list
> > might not have been initialized and we want to avoid accessing an
> > invalid list.
> 
> Nope, none of this.
> 
> The problem is intel_gvt_init() and intel_gvt_driver_remove() happening
> at different abstraction levels in i915_driver.c, with the calls also
> happening at different abstraction levels in the error path.
> 
> Basically when i915_driver_hw_probe() returns with an error, the caller
> has no way of knowing whether intel_gvt_init() succeeded or not, and any
> call to intel_gvt_driver_remove() is bound to be wrong.
> 
> The fix is not to make intel_gvt_driver_remove() "gracefully" handle
> broken probe/cleanup calls, but to fix the probe/cleanup calls.

Yes, agree, the initialization of the vgpu is a little fancy (I
was about to re-reply right after my reply with a better
suggestion, but I didn't want to spam)

But as a fast one- liner and correct cleanup we have the choice
to gracefully remove the driver or freak out because we are
trying to delete a list that is not initialized.

[*] https://intel-gfx-ci.01.org/tree/drm-tip/IGT_8711/shard-dg2-5/igt@i915_module_load@fault-injection.html#dmesg-warnings385

> Michał is actually looking into this, Cc'd.

Good that Michal is looking into this.

Thank you,
Andi