Re: [PATCH 08/13] platform/apple-gmux: Add documentation about iMacs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] platform/x86: apple-gmux: lock leak and uninit return in gmux_mmio_read8/32()
- [High] platform/x86: apple-gmux: NULL discrete_pdev deref in gmux_set_discrete_state()
- [High] platform/x86: apple-gmux: workqueue teardown bugs in gmux_probe/remove()
- [High] PCI: uninitialized output in pci_read_config_*() when access is ignored
- [High] platform/x86: apple-gmux: PCI state corruption in gmux_set_discrete_state()
--

commit 60c5ee5c904bb8e5a78842b541c5dc5de1769e8f
Author: Atharva Tiwari <atharvatiwarilinuxdev@gmail.com>

platform/apple-gmux: Add documentation about iMacs

This series updates the apple-gmux driver and PCI access layer to support MMIO
gmux power management, interrupts, boot GPU selection, FSS detection, and iMac
documentation.

> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index b123da16b63ba..5e95130671767 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -559,6 +559,9 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
>  
>  int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev)) {
>  		PCI_SET_ERROR_RESPONSE(val);
>  		return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -569,6 +572,9 @@ EXPORT_SYMBOL(pci_read_config_byte);
>  
>  int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;
> +
>  	if (pci_dev_is_disconnected(dev)) {
>  		PCI_SET_ERROR_RESPONSE(val);
>  		return PCIBIOS_DEVICE_NOT_FOUND;
> @@ -580,6 +586,9 @@ EXPORT_SYMBOL(pci_read_config_word);
>  int pci_read_config_dword(const struct pci_dev *dev, int where,
>  					u32 *val)
>  {
> +	if (pci_dev_ignore_access(dev))
> +		return PCIBIOS_SUCCESSFUL;

[Severity: High]
Does returning PCIBIOS_SUCCESSFUL without setting *val in
pci_read_config_byte(), pci_read_config_word(), and pci_read_config_dword()
leave caller stack variables uninitialized when pci_dev_ignore_access(dev) is
true?

Callers such as pci_update_current_state() and pci_clear_and_set_config_dword()
pass an uninitialized stack variable to pci_read_config_word() or
pci_read_config_dword() expecting *val to be populated on return:

pci_update_current_state()
    u16 pmcsr;
    pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
    dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);

When dev->ignore_access_d3 is set and dev->current_state is PCI_D3cold, leaving
pmcsr uninitialized can overwrite dev->current_state and clear PCI_D3cold while
the upstream bridge is still powered down by PWRD(1), allowing subsequent
config accesses to hit the powered-off bridge.

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

[ ... ]

> @@ -244,12 +264,14 @@ static u8 gmux_mmio_read8(struct apple_gmux_data *gmux_data, int port)
>  	u8 val;
>  
>  	mutex_lock(&gmux_data->index_lock);
> -	gmux_mmio_wait(gmux_data);
> -	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> -	iowrite8(GMUX_MMIO_READ | sizeof(val),
> -		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> -	gmux_mmio_wait(gmux_data);
> -	val = ioread8(gmux_data->iomem_base);
> +	if (gmux_mmio_wait(gmux_data)) {
> +		iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +		iowrite8(GMUX_MMIO_READ | sizeof(val),
> +			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +		if (!gmux_mmio_wait(gmux_data))
> +			return 0;
> +		val = ioread8(gmux_data->iomem_base);
> +	}
>  	mutex_unlock(&gmux_data->index_lock);
>  
>  	return val;

[ ... ]

> @@ -275,12 +300,16 @@ static u32 gmux_mmio_read32(struct apple_gmux_data *gmux_data, int port)
>  	u32 val;
>  
>  	mutex_lock(&gmux_data->index_lock);
> -	gmux_mmio_wait(gmux_data);
> -	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> -	iowrite8(GMUX_MMIO_READ | sizeof(val),
> -		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> -	gmux_mmio_wait(gmux_data);
> -	val = ioread32be(gmux_data->iomem_base);
> +
> +	if (gmux_mmio_wait(gmux_data)) {
> +		iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
> +		iowrite8(GMUX_MMIO_READ | sizeof(val),
> +			gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
> +		if (!gmux_mmio_wait(gmux_data))
> +			return 0;
> +
> +		val = ioread32be(gmux_data->iomem_base);
> +	}
>  	mutex_unlock(&gmux_data->index_lock);
>  
>  	return val;

[Severity: High]
In gmux_mmio_read8() and gmux_mmio_read32(), if the second gmux_mmio_wait()
times out and returns 0, won't returning 0 directly leak gmux_data->index_lock
and deadlock subsequent gmux MMIO accesses?

Also, if the first gmux_mmio_wait() call returns 0, the if block is skipped and
both functions return the uninitialized local variable val (which can also be
read from userspace via debugfs in gmux_selected_port_data_read()). Should val
be initialized to 0 and the early return paths unlock gmux_data->index_lock
before returning?

[ ... ]

> @@ -502,23 +565,171 @@ 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_HANDLE(&gmux_data->discrete_pdev->dev);
> +	acpi_status status;
> +	unsigned long long result;
> +
> +	if (!discrete_handle)
> +		return -ENODEV;

[Severity: High]
Can gmux_set_discrete_state() dereference a NULL gmux_data->discrete_pdev here?

gmux_data->discrete_pdev is only assigned in gmux_get_client_id() when
vga_switcheroo_enable() runs after both GPU clients have registered. If the
discrete GPU registers and enters runtime suspend before a second client
registers:

vga_switcheroo_runtime_suspend()
  -> vga_switcheroo_power_switch()
    -> gmux_set_power_state()
      -> gmux_set_discrete_state()

gmux_data->discrete_pdev is still NULL when evaluating
ACPI_HANDLE(&gmux_data->discrete_pdev->dev). In addition, checking
!discrete_handle unconditionally at the start of gmux_set_discrete_state()
will abort with -ENODEV on non-MMIO systems (APPLE_GMUX_TYPE_PIO and
APPLE_GMUX_TYPE_INDEXED) that do not use discrete_handle.

>  	if (state == VGA_SWITCHEROO_ON) {

[ ... ]

>  		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) {
> +			if (gmux_data->uses_pwrd) {
> +				acpi_handle parent_handle = discrete_handle;
> +				bool saved_hda = false;
> +
> +				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;
> +				}
> +				status = acpi_execute_simple_method(discrete_handle, "PWRD", 1);
> +				if (ACPI_FAILURE(status))
> +					return -EIO;
> +			}

[Severity: High]
What happens on MacBookPro16,1 and MacBookPro16,4 (where gmux_data->uses_pwrd
is true) when the system suspends and resumes while gmux_data->power_state is
already VGA_SWITCHEROO_OFF?

On system resume, gmux_resume() calls:

gmux_resume()
  -> gmux_set_discrete_state(gmux_data, VGA_SWITCHEROO_OFF)

When the discrete GPU and upstream bridges are already powered off in
PCI_D3cold, calling pci_save_state() and gmux_save_restore_pci(..., true, NULL)
again will overwrite the previously saved PCI configuration state of HDAU,
gmux_data->discrete_pdev, and the upstream bridges with readings from the
powered-down devices before a subsequent VGA_SWITCHEROO_ON can restore them.

[ ... ]

> @@ -823,6 +1101,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  			pr_err("couldn't remap gmux mmio region");
>  			goto err_release;
>  		}
> +		INIT_WORK(&gmux_data->interrupt_work, gmux_notify_work);
>  		goto get_version;
>  	case APPLE_GMUX_TYPE_INDEXED:
>  		gmux_data->config = &apple_gmux_index;

[ ... ]

> @@ -977,6 +1263,8 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  err_unmap:
>  	if (gmux_data->iomem_base)
>  		iounmap(gmux_data->iomem_base);
> +	if (gmux_data->type == APPLE_GMUX_TYPE_MMIO)
> +		cancel_work_sync(&gmux_data->interrupt_work);
>  err_release:
>  	if (gmux_data->config->resource_type == IORESOURCE_MEM)
>  		release_mem_region(gmux_data->iostart, gmux_data->iolen);
> @@ -993,7 +1281,9 @@ static void gmux_remove(struct pnp_dev *pnp)
>  
>  	gmux_fini_debugfs(gmux_data);
>  	vga_switcheroo_unregister_handler();
> +	pci_dev_put(gmux_data->discrete_pdev);
>  	gmux_disable_interrupts(gmux_data);
> +	cancel_work_sync(&gmux_data->interrupt_work);
>  	if (gmux_data->gpe >= 0) {
>  		acpi_disable_gpe(NULL, gmux_data->gpe);
>  		acpi_remove_notify_handler(gmux_data->dhandle,

[Severity: High]
Are there a few teardown ordering issues with gmux_data->interrupt_work in
gmux_probe() and gmux_remove()?

First, INIT_WORK(&gmux_data->interrupt_work, gmux_notify_work) is only called
in gmux_probe() when type is APPLE_GMUX_TYPE_MMIO, but gmux_remove() calls
cancel_work_sync(&gmux_data->interrupt_work) unconditionally, which triggers
WARN_ON(!work->func) in __flush_work() on PIO and indexed gmux devices.

Second, in the err_unmap path of gmux_probe(), iounmap(gmux_data->iomem_base)
is called before cancel_work_sync(&gmux_data->interrupt_work), so a running
gmux_notify_work() can access gmux_data->iomem_base after it has been unmapped.

Third, in gmux_remove(), pci_dev_put(gmux_data->discrete_pdev) and
cancel_work_sync(&gmux_data->interrupt_work) are called before
acpi_disable_gpe() and acpi_remove_notify_handler(). Can gmux_notify_handler()
reschedule interrupt_work after cancel_work_sync(), or can a running
gmux_notify_work() dereference gmux_data->discrete_pdev after pci_dev_put()?

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