Re: [PATCH v3 1/3] platform/x86: apple-gmux: add MacBookPro15,1 dGPU power sequence

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The discrete GPU's PCI device pointer (`discrete_pdev`) fails to initialize if the `apple-gmux` module is reloaded, breaking the discrete GPU power-on sequence on MacBookPro15,1.
--

--- Patch [1]: [PATCH v3 1/3] platform/x86: apple-gmux: add MacBookPro15,1 dGPU power sequence ---
commit 82f7efe17d405f437532909cef38c118380b2e3f
Author: Atharva Tiwari <[email protected]>
Subject: platform/x86: apple-gmux: add MacBookPro15,1 dGPU power sequence

This commit adds a specific power-on sequence using PWG1 and PWG3 ACPI
methods for the discrete GPU on MacBookPro15,1. It evaluates these link
methods around the GMUX transition and waits for PCI configuration space
before completing power-on.

> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index fbc30f1f8abdf..2b6363aab8d15 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -510,14 +541,49 @@ static int gmux_switch_ddc(enum vga_switcheroo_client_id id)
>  static int gmux_set_discrete_state(struct apple_gmux_data *gmux_data,
>  				   enum vga_switcheroo_state state)
>  {
> +	int ret;
> +
>  	reinit_completion(&gmux_data->powerchange_done);
>  
>  	if (state == VGA_SWITCHEROO_ON) {
> -		gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 1);
> -		gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 3);
> +		if (gmux_data->use_pwg_power_sequence &&
> +		    gmux_data->discrete_pdev) {

[Severity: High]
If discrete_pdev fails to initialize during module reload, won't this check
silently fall back to the legacy power sequence?

If the legacy sequence is used on this model, could the discrete GPU fail to
power on properly, causing subsequent PCI accesses to read 0xFFFFFFFF and
potentially crash the system?

[ ... ]

> @@ -549,11 +615,16 @@ static enum vga_switcheroo_client_id gmux_get_client_id(struct pci_dev *pdev)
>  	 */
>  	if (pdev->vendor == PCI_VENDOR_ID_INTEL)
>  		return VGA_SWITCHEROO_IGD;
> -	else if (pdev->vendor == PCI_VENDOR_ID_NVIDIA &&
> -		 pdev->device == 0x0863)
> +	if (pdev->vendor == PCI_VENDOR_ID_NVIDIA && pdev->device == 0x0863)
>  		return VGA_SWITCHEROO_IGD;
> -	else
> -		return VGA_SWITCHEROO_DIS;
> +
> +	if (apple_gmux_data->use_pwg_power_sequence &&
> +	    apple_gmux_data->discrete_pdev != pdev) {
> +		pci_dev_put(apple_gmux_data->discrete_pdev);
> +		apple_gmux_data->discrete_pdev = pci_dev_get(pdev);
> +	}
> +
> +	return VGA_SWITCHEROO_DIS;
>  }

[Severity: High]
If the apple-gmux module is unloaded and reloaded, will this initialization
actually happen?

Looking at vga_switcheroo_enable() in the vgasr core, it intentionally skips
get_client_id() for clients that already have a known ID:

vga_switcheroo.c:vga_switcheroo_enable() {
    ...
    if (!client_is_vga(client) ||
         client_id(client) != VGA_SWITCHEROO_UNKNOWN_ID)
        continue;

    ret = vgasr_priv.handler->get_client_id(client->pdev);
    ...
}

Since clients retain their IDs from the first load, won't get_client_id() be
skipped entirely on module reload, leaving discrete_pdev as NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1