Re: [PATCH 05/15] vga_switcheroo: Drop client power state VGA_SWITCHEROO_INIT

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Sep 08, 2015 at 02:17:47PM +0200, Lukas Wunner wrote:
> hda_intel.c:azx_probe() defers initialization of an audio controller
> on the discrete GPU if the GPU is powered off. The power state of the
> GPU is determined by calling vga_switcheroo_get_client_state().
> 
> vga_switcheroo_get_client_state() returns VGA_SWITCHEROO_INIT if
> vga_switcheroo is not enabled, i.e. if no second GPU or no handler
> has registered.
> 
> This can go wrong in the following scenario:
> - Driver for the integrated GPU is not loaded.
> - Driver for the discrete GPU registers with vga_switcheroo, uses driver
>   power control to power down the GPU, handler cuts power to the GPU.
> - Driver for the audio controller gets loaded after the GPU was powered
>   down, calls vga_switcheroo_get_client_state() which returns
>   VGA_SWITCHEROO_INIT instead of VGA_SWITCHEROO_OFF.
> - Consequence: azx_probe() tries to initialize the audio controller even
>   though the GPU is powered down.
> 
> The power state VGA_SWITCHEROO_INIT was introduced by c8e9cf7bb240
> ("vga_switcheroo: Add a helper function to get the client state").
> It is not apparent what its benefit might be. The idea seems to
> be to initialize the audio controller even if the power state is
> VGA_SWITCHEROO_OFF (were vga_switcheroo enabled), but as shown
> above this can fail.
> 
> Drop VGA_SWITCHEROO_INIT to solve this.
> 
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Takashi, does this look good to you? Ack for merging through drm-misc?

I pulled in patch 4 meanwhile.
-Daniel

> ---
>  drivers/gpu/vga/vga_switcheroo.c | 2 --
>  include/linux/vga_switcheroo.h   | 5 -----
>  2 files changed, 7 deletions(-)
> 
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 2138162..845e47d 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -355,8 +355,6 @@ int vga_switcheroo_get_client_state(struct pci_dev *pdev)
>  	client = find_client_from_pci(&vgasr_priv.clients, pdev);
>  	if (!client)
>  		ret = VGA_SWITCHEROO_NOT_FOUND;
> -	else if (!vgasr_priv.active)
> -		ret = VGA_SWITCHEROO_INIT;
>  	else
>  		ret = client->pwr_state;
>  	mutex_unlock(&vgasr_mutex);
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index 3764991..95bfbeb0 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -39,10 +39,6 @@ struct pci_dev;
>   * enum vga_switcheroo_state - client power state
>   * @VGA_SWITCHEROO_OFF: off
>   * @VGA_SWITCHEROO_ON: on
> - * @VGA_SWITCHEROO_INIT: client has registered with vga_switcheroo but
> - * 	vga_switcheroo is not enabled, i.e. no second client or no handler
> - * 	has registered. Only used in vga_switcheroo_get_client_state() which
> - * 	in turn is only called from hda_intel.c
>   * @VGA_SWITCHEROO_NOT_FOUND: client has not registered with vga_switcheroo.
>   * 	Only used in vga_switcheroo_get_client_state() which in turn is only
>   * 	called from hda_intel.c
> @@ -53,7 +49,6 @@ enum vga_switcheroo_state {
>  	VGA_SWITCHEROO_OFF,
>  	VGA_SWITCHEROO_ON,
>  	/* below are referred only from vga_switcheroo_get_client_state() */
> -	VGA_SWITCHEROO_INIT,
>  	VGA_SWITCHEROO_NOT_FOUND,
>  };
>  
> -- 
> 1.8.5.2 (Apple Git-48)
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel