Re: [PATCH v2 07/12] platform/apple-gmux: Add documentation about iMacs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] platform/x86: apple-gmux: premature teardown and EFI write in
  gmux_shutdown()
- [Medium] platform/x86: apple-gmux: uninitialized work and race in
  gmux_probe()
- [Medium] platform/x86: apple-gmux: state save bugs in
  gmux_set_discrete_state()
- [Medium] platform/x86: apple-gmux: lost irq bits and loop in
  gmux_notify_work()
- [Low] platform/x86: apple-gmux: unused variable result in
  gmux_set_discrete_state()
--

commit 765faab19bedc6dfe41f21d3ab1e32b30a85f20e
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

platform/apple-gmux: Add documentation about iMacs

This series updates the apple-gmux driver to support power control,
interrupt handling, EFI switch state persistence, and documentation for
MMIO/T2 Macs.

> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 9c728ac4e045a..4f5c984450580 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c

[ ... ]

> @@ -502,23 +557,158 @@ 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)
>  {
>  	reinit_completion(&gmux_data->powerchange_done);
> +	acpi_handle discrete_handle;
> +	acpi_status status;
> +	unsigned long long result;

[Severity: Low]
result is never used in gmux_set_discrete_state(), and these declarations
are placed after a statement, triggering compiler warnings.

> +
> +	if (!gmux_data->discrete_pdev)
> +		return -ENODEV;
> +
> +	discrete_handle = ACPI_HANDLE(&gmux_data->discrete_pdev->dev);
> +	if (!discrete_handle)
> +		return -ENODEV;
>  
>  	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->type == APPLE_GMUX_TYPE_MMIO) {
> +			acpi_handle handle;
> +			u16 vendor;
> +			int i;
> +
> +			gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 2);
> +			msleep(100);
> +			gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 3);
> +
> +			status = acpi_execute_simple_method(discrete_handle, "PWRD", 0);
> +			if (ACPI_FAILURE(status))
> +				return -EIO;
> +			msleep(20);
> +
> +			if (gmux_save_restore_pci(NULL, "\\_SB_.PCI0.PEG0", 0, &handle))
> +				return -EIO;
> +
> +			if (gmux_save_restore_pci(handle, "EGP0", 0, &handle))
> +				return -EIO;
> +
> +			if (gmux_save_restore_pci(handle, "EGP1", 0, &handle))
> +				return -EIO;
> +
> +			for (i = 0; i < 1000; i++) {
> +				pci_read_config_word(gmux_data->discrete_pdev,
> +							PCI_VENDOR_ID, &vendor);
> +				if (vendor != 0xffff)
> +					break;
> +				msleep(1);
> +			}
> +			if (vendor == 0xffff)
> +				return -ETIMEDOUT;
> +
> +			if (gmux_save_restore_pci(handle, "GFX0", 0, NULL))
> +				return -EIO;
> +
> +			if (gmux_save_restore_pci(handle, "HDAU", 0, NULL))
> +				return -EIO;
> +		} else {
> +			gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 1);
> +			gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 3);
> +		}
>  		pr_debug("Discrete card powered up\n");
>  	} else {
> -		gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 1);
> -		gmux_write8(gmux_data, GMUX_PORT_DISCRETE_POWER, 0);
> +		if (gmux_data->type == APPLE_GMUX_TYPE_MMIO) {
> +			acpi_handle parent_handle = discrete_handle;
> +			bool saved_hda = false;
> +
> +			/* If the power state was OFF, then we only do
> +			 * PWRD and we do not save registers, as the
> +			 * registers are already saved
> +			 */
> +			if (gmux_data->power_state == VGA_SWITCHEROO_OFF)
> +				goto do_pwrd;
> +
> +			while (ACPI_SUCCESS(acpi_get_parent(parent_handle,
> +							&parent_handle))) {
> +				if (!saved_hda) {
> +					if (!gmux_save_restore_pci(parent_handle,
> +								"HDAU", true, NULL)) {
> +						saved_hda = true;
> +						pci_save_state(gmux_data->discrete_pdev);
> +						gmux_data->discrete_pdev->current_state =
> +								PCI_D3cold;
> +					}
> +				}
> +				if (gmux_save_restore_pci(parent_handle, NULL, true, NULL))
> +					break;
> +			}

[Severity: Medium]
Does requiring gmux_data->discrete_pdev and discrete_handle at the start of
gmux_set_discrete_state() break power control on non-MMIO Macs when
discrete_pdev has not been populated yet or lacks an ACPI companion handle,
even though only the APPLE_GMUX_TYPE_MMIO paths use them?

Also, in the MMIO power-off loop, pci_save_state(gmux_data->discrete_pdev)
is gated on gmux_save_restore_pci(parent_handle, "HDAU", true, NULL)
succeeding, so the discrete GPU's PCI state is not saved before cutting
power if HDAU is absent or fails to save.  Finally, if an MMIO power-on
fails after gmux_save_restore_pci() restores PEG0, EGP0, and EGP1 (which
clears pdev->state_saved), returning early leaves gmux_data->power_state as
VGA_SWITCHEROO_OFF, causing the next power-off to jump to do_pwrd without
saving the bridges' PCI state again.

[ ... ]

> @@ -655,23 +853,78 @@ static void gmux_clear_interrupts(struct apple_gmux_data *gmux_data)
>  	/* to clear interrupts write back current status */
>  	status = gmux_interrupt_get_status(gmux_data);
>  	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> -	/* Prevent flood of status=0 interrupts */
> -	if (gmux_data->config == &apple_gmux_mmio)
> -		acpi_execute_simple_method(gmux_data->dhandle, "GMSP", 0);
>  }
>  
> +static void gmux_notify_work(struct work_struct *work)
> +{
> +	struct apple_gmux_data *gmux_data =
> +		container_of(work, struct apple_gmux_data, interrupt_work);
> +	u8 status;
> +	bool turn_on_gpu = false;
> +
> +	status = gmux_interrupt_get_status(gmux_data);
> +	if (!status)
> +		return;
> +
> +	gmux_disable_interrupts(gmux_data);
> +	pr_debug("Notify handler called: status %d\n", status);
> +	acpi_execute_simple_method(gmux_data->dhandle, "GMSP", 0);
> +
> +	status = gmux_interrupt_get_status(gmux_data);
> +	while (status) {
> +		gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> +		if (status & GMUX_INTERRUPT_STATUS_FAILED) {
> +			pr_warn_ratelimited("interrupt failed: status: 0x%x\n", status);
> +			status = gmux_interrupt_get_status(gmux_data);
> +			continue;
> +		}
> +		if (status & GMUX_INTERRUPT_STATUS_POWER)
> +			complete(&gmux_data->powerchange_done);

[Severity: Medium]
Does writing status to GMUX_PORT_INTERRUPT_STATUS before the continue on
GMUX_INTERRUPT_STATUS_FAILED drop GMUX_INTERRUPT_STATUS_POWER or
GMUX_INTERRUPT_STATUS_HOTPLUG events that were set in the same status read?
Clearing all asserted bits in hardware and skipping the remaining checks
can cause powerchange_done timeouts or missed hotplug wakeups.  Also,
while (status) has no bound, so a stuck status bit will spin indefinitely on
the system workqueue.

[ ... ]

> @@ -920,6 +1174,8 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  			goto err_notify;
>  		}
>  
> +		if (type == APPLE_GMUX_TYPE_MMIO)
> +			INIT_WORK(&gmux_data->interrupt_work, gmux_notify_work);
>  		status = acpi_enable_gpe(NULL, gmux_data->gpe);

[ ... ]

> @@ -972,6 +1230,9 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  		acpi_remove_notify_handler(gmux_data->dhandle,
>  					   ACPI_DEVICE_NOTIFY,
>  					   &gmux_notify_handler);
> +
> +	if (gmux_data->type == APPLE_GMUX_TYPE_MMIO)
> +		cancel_work_sync(&gmux_data->interrupt_work);
>  err_notify:
>  	backlight_device_unregister(bdev);
>  err_unmap:
> @@ -1001,6 +1262,9 @@ static void gmux_remove(struct pnp_dev *pnp)
>  					   &gmux_notify_handler);
>  	}
>  
> +	if (gmux_data->type == APPLE_GMUX_TYPE_MMIO)
> +		cancel_work_sync(&gmux_data->interrupt_work);

[Severity: Medium]
Can gmux_data->interrupt_work be scheduled or canceled before INIT_WORK()
runs?

INIT_WORK(&gmux_data->interrupt_work, gmux_notify_work) is only called when
evaluating GMGP succeeds, and after acpi_install_notify_handler() is
registered.  If GMGP fails on an MMIO system, err_enable_gpe and
gmux_remove() call cancel_work_sync(&gmux_data->interrupt_work) on an
uninitialized work_struct, and an ACPI notification arriving between
acpi_install_notify_handler() and INIT_WORK() (or before
init_completion(&gmux_data->powerchange_done)) will schedule uninitialized
work.  Also, gmux_suspend() disables interrupts without canceling
interrupt_work, allowing an in-flight gmux_notify_work() to call
gmux_enable_interrupts() and re-enable interrupts while suspended.

>  	pci_dev_put(gmux_data->discrete_pdev);
>  	backlight_device_unregister(gmux_data->bdev);
>  
>  	if (gmux_data->iomem_base) {
> @@ -1012,6 +1276,15 @@ static void gmux_remove(struct pnp_dev *pnp)
>  	kfree(gmux_data);
>  }
>  
> +static void gmux_shutdown(struct pnp_dev *pnp)
> +{
> +	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
> +
> +	gmux_write_switch_state_efi(gmux_data);
> +
> +	gmux_remove(pnp);
> +}

[Severity: High]
Does calling gmux_remove() from gmux_shutdown() cause a NULL pointer
dereference when pci_device_shutdown() runs afterward?

gmux_remove() calls vga_switcheroo_unregister_handler() (setting
vgasr_priv.handler to NULL), unmaps MMIO, and frees gmux_data.  Because PNP
devices are registered after PCI devices, device_shutdown() runs
gmux_shutdown() before pci_device_shutdown() on the discrete GPU.  If the
discrete GPU is runtime-suspended at shutdown, pci_device_shutdown() calls
pm_runtime_resume() -> vga_switcheroo_runtime_resume() ->
vga_switcheroo_power_switch(), which dereferences vgasr_priv.handler:

drivers/gpu/vga/vga_switcheroo.c:vga_switcheroo_power_switch() {
	struct vga_switcheroo_client *client;

	if (!vgasr_priv.handler->power_state)
		return;
    ...
}

Also, gmux_shutdown() calls gmux_write_switch_state_efi() without checking
gmux_data->type == APPLE_GMUX_TYPE_MMIO (unlike gmux_switchto()), writing
the T2 gpu-active EFI variable on classic and indexed gmux Macs as well.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114160731.3756-1-atharvatiwarilinuxdev@gmail.com?part=7