Re: [PATCH v11 1/6] net: wwan: t9xx: Add PCIe core

[Wu. JackBB (GSM)]

Hi Brian,

Thanks for the review.

On Fri, Nov 13, 2026 at 6:07 AM Brian Norris <briannorris@xxxxxxxxxxxx> wrote:
> On Thu, Nov 05, 2026 at 03:56:27PM +0800, Jack Wu wrote:
[...]
> > +     ret = pci_save_state(pdev);
> > +     if (ret) {
> > +             dev_err(mdev->dev, "Failed to save PCI state: %d\n", ret);
> > +             goto free_irq;
> > +     }
>
> I'm a bit skeptical of drivers that stash an extra copy of PCI state.
> There are times where it's legitimate, but often it means you'll
> introduce bugs of your own.

Agreed, v12 will drop the copy. remove() will call pci_save_state() right
before the modem reset and pci_restore_state() right after it, the way
t7xx_reset_device() does, and probe() will no longer save anything.

The restore itself is still needed: the reset returns the endpoint to
its power-on defaults, with the BARs and the command register cleared,
so config space has to be written back for the next driver that binds
to the device.

[...]
> > +     /* Without this a runtime-PM suspend after unbind would snapshot the
> > +      * reset state over our copy, and whoever binds next finds BARs the
> > +      * device does not decode.
> > +      */
>
> I'm confused about this explanation. You don't ever call
> pm_runtime_put() or similar, so this device can never actually
> runtime-suspend. Are you talking about some code that's not included in
> this patch series?
>
> https://docs.kernel.org/power/pci.html#device-runtime-power-management

You're right, the comment is wrong. The driver does not use runtime PM,
and the save and restore are needed only because of the reset above.
The comment will be removed in v12.

> Anyway, if you *did* have a problem here ... I expect the answer is to
> properly get() a reference on the way out, so that runtime_suspend is no
> longer possible after a reset, until the next probe().

With both the save and the restore inside remove(), the reference the
PCI core already holds across ->remove() keeps the device active between
them, and config space is back in place before remove() returns. So v12
will not need a reference of its own.

> You also have a slightly different description from the cover letter:
>
> """
>   - mtk_pci_remove() calls pci_restore_state() and pci_clear_master()
>     after pci_load_and_free_saved_state(): the reset leaves the device
>     at power-on defaults and the state was saved after pci_set_master()
> """

You're right, the code no longer matches that description. In v12 the
save will happen after pci_clear_master() and pci_disable_device(), right
before the reset, so the restored command register has bus mastering
off, and the v12 changelog will describe it that way.

> In any case, something seems fishy here.

Agreed. In v12 it will be a single save and restore around the reset.

Thanks,
Jack