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 <[email protected]> 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/[email protected]?part=8
