Re: [PATCH v4 3/9] ipack: tpci200: don't store pci_device_id

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] MMIO configuration registers are mapped and accessed before calling pci_enable_device(), which can lead to dropped writes or master aborts.
--

--- Patch [3]: [PATCH v4 3/9] ipack: tpci200: don't store pci_device_id ---
commit eed0a798b7390c20259769af7ca3dfe410d112f0
Author: Gary Guo <[email protected]>
Subject: ipack: tpci200: don't store pci_device_id

This commit removes the stored pci_device_id from the tpci200_infos structure.
Since dynamic IDs are not guaranteed to outlive the probe function, and the
stored ID is unused, removing it prevents potential issues.

> diff --git a/drivers/ipack/carriers/tpci200.c b/drivers/ipack/carriers/tpci200.c
> index 05dcb6675cd66..1cf51f7632932 100644
> --- a/drivers/ipack/carriers/tpci200.c
> +++ b/drivers/ipack/carriers/tpci200.c
> @@ -562,7 +562,6 @@ static int tpci200_pci_probe(struct pci_dev *pdev,
>  
>  	/* Save struct pci_dev pointer */
>  	tpci200->info->pdev = pdev;
> -	tpci200->info->id_table = (struct pci_device_id *)id;
>  
>  	/* register the device and initialize it */
>  	ret = tpci200_install(tpci200);

[Severity: High]
This is a pre-existing issue, but I noticed that MMIO configuration registers
are mapped and accessed before pci_enable_device() is called.

Earlier in tpci200_pci_probe(), the driver maps the configuration memory bar
and immediately accesses the registers to configure byte swapping:

    reg32 = ioread32(tpci200->info->cfg_regs + LAS1_DESC);
    reg32 |= 1 << LAS_BIT_BIGENDIAN;
    iowrite32(reg32, tpci200->info->cfg_regs + LAS1_DESC);

However, pci_enable_device() doesn't seem to be called until later in the
probe sequence, when tpci200_install() eventually calls tpci200_register():

    if (pci_enable_device(tpci200->info->pdev) < 0)
        return -ENODEV;

Since the memory enable bit in the PCI command register is not guaranteed to
be set by the firmware before probe, accessing MMIO regions before
pci_enable_device() can result in reads returning 0xFFFFFFFF and writes being
dropped. On some architectures, this might even trigger a system-crashing
PCI master abort.

Should pci_enable_device() be called earlier in tpci200_pci_probe() before
any MMIO access occurs?

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