Re: [PATCH 1/1] drm/i915: handle failure from vga_get

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 18 Feb 2026, Simon Richter <Simon.Richter@hogyros.de> wrote:
> This function returns an error code.

Which function? Please make commit messages independent of the Subject
line.

Also, the function we call is vga_get_uninterruptible().

> If that code is non-zero, VGA decoding
> is undefined, and the lock counter has not been increased, so it is not valid
> to access registers or call vga_put afterwards.
>
> Signed-off-by: Simon Richter <Simon.Richter@hogyros.de>
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1824
> ---
>  drivers/gpu/drm/i915/display/intel_vga.c | 29 ++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vga.c b/drivers/gpu/drm/i915/display/intel_vga.c
> index 6fc3e3702cb8..4118c451d53c 100644
> --- a/drivers/gpu/drm/i915/display/intel_vga.c
> +++ b/drivers/gpu/drm/i915/display/intel_vga.c
> @@ -112,12 +112,17 @@ static bool intel_pci_bridge_set_vga(struct pci_dev *pdev, bool enable)
>  	return old & PCI_BRIDGE_CTL_VGA;
>  }
>  
> -static bool intel_vga_get(struct intel_display *display, bool mmio)
> +static int __must_check intel_vga_get(struct intel_display *display,

I'm not convinced by __must_check annotations for static
functions. Global stuff, sure, but we'd be adding tons and tons of these
annotations if we decided to go this path.

> +	bool mmio, bool *old_io_decode)
>  {
>  	struct pci_dev *pdev = to_pci_dev(display->drm->dev);
> +	int err;
>  
>  	if (mmio)
> -		return false;
> +	{

Brace location, please see checkpatch.

> +		*old_io_decode = false;
> +		return 0;
> +	}
>  
>  	/*
>  	 * Bypass the VGA arbiter on the iGPU and just enable
> @@ -131,9 +136,14 @@ static bool intel_vga_get(struct intel_display *display, bool mmio)
>  	 * of how any other VGA routing bits are configured.
>  	 */
>  	if (display->platform.dgfx)
> -		vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
> +	{

Brace location.

> +		err = vga_get_uninterruptible(pdev, VGA_RSRC_LEGACY_IO);
> +		if (unlikely(err))

I'm not convinced by unlikely annotations in cases like this. It's just
more stuff to read, for no clear benefit. Again, if we set the example,
we'll be adding these absolutely everywhere.

> +			return err;
> +	}
>  
> -	return intel_pci_set_io_decode(pdev, true);
> +	*old_io_decode = intel_pci_set_io_decode(pdev, true);

Blank line before return.

> +	return 0;
>  }
>  
>  static void intel_vga_put(struct intel_display *display, bool io_decode, bool mmio)
> @@ -175,6 +185,7 @@ void intel_vga_disable(struct intel_display *display)
>  	bool io_decode;
>  	u8 msr, sr1;
>  	u32 tmp;
> +	int err;
>  
>  	if (!intel_vga_decode_is_enabled(display)) {
>  		drm_dbg_kms(display->drm, "VGA decode is disabled\n");
> @@ -216,7 +227,15 @@ void intel_vga_disable(struct intel_display *display)
>  			goto reset_vgacntr;
>  	}
>  
> -	io_decode = intel_vga_get(display, mmio);
> +	/* This should not fail, because vga_get will only report errors for

/* on its own line please.

> +	 * dGPUs that are unreachable via the bridge, and cannot be made
> +	 * reachable either. We shouldn't even get here for this case, but if
> +	 * we do, we assume that the bridge will also refuse future requests
> +	 * to forward VGA accesses.
> +	 */
> +	err = intel_vga_get(display, mmio, &io_decode);
> +	if (unlikely(err))

Please no unlikely.

> +		goto reset_vgacntr;
>  
>  	drm_WARN_ON(display->drm, !mmio && !intel_pci_has_vga_io_decode(pdev));

-- 
Jani Nikula, Intel