Re: [PATCH v2 04/13] PCI/IOV: Allow VF drivers to handle resets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] PCI/IOV: calling VF reset_prepare() before PF breaks hisi_qm PF FLR
--

commit b82e20ef0c7a057bbb242268e11b3dd9218fd7d8
Author: Michał Winiarski <michal.winiarski@intel.com>

PCI/IOV: Allow VF drivers to handle resets

This commit updates PCI function, bus, and slot reset handling to lock
enabled VFs, notify VF drivers via .reset_prepare() and .reset_done(), and
save and restore VF configuration state across PF resets.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d8e7c7d8e4021..d088302547c34 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

[ ... ]

> @@ -5171,6 +5202,17 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
>  		return;
>  
>  	pci_save_state(dev);
> +}
> +
> +static void pci_dev_save_and_disable(struct pci_dev *dev)
> +{
> +	struct pci_dev *vf;
> +
> +	for_each_pci_vf(dev, vf)
> +		pci_dev_reset_prepare_and_save(vf);
> +
> +	pci_dev_reset_prepare_and_save(dev);

[Severity: High]
Does calling .reset_prepare() on each VF before the PF here cause a
self-blocking timeout and reset failure on HiSilicon QM devices
(hisi_hpre, hisi_sec2, hisi_zip, and hisi_acc_vfio_pci) when resetting a PF
with enabled VFs?

In pci_dev_save_and_disable(), pci_dev_reset_prepare_and_save(vf) runs for
every VF and then pci_dev_reset_prepare_and_save(dev) runs for the PF in the
same thread before any .reset_done() callback is invoked in
pci_dev_restore().

When VFs are bound to hisi_acc_vfio_pci (where hisi_acc_vdev->pf_qm points
to the PF's struct hisi_qm), the first VF's .reset_prepare() sets the reset
bit on the shared PF qm->misc_ctl and holds it until .reset_done():

drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c:
hisi_acc_vf_pci_reset_prepare() {
	struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_drvdata(pdev);
	struct hisi_qm *qm = hisi_acc_vdev->pf_qm;
	...
	while (test_and_set_bit(QM_RESETTING, &qm->misc_ctl)) {
		msleep(1);
		if (++delay > QM_RESET_WAIT_TIMEOUT) {
			dev_err(dev, "reset prepare failed\n");
			return;
		}
	}
}

Similarly, when VFs are bound to hisi_hpre, hisi_sec2, or hisi_zip on
Kunpeng920, hisi_qm_reset_prepare() -> qm_reset_prepare_ready() acquires the
same bit on pf_qm->misc_ctl via qm_wait_reset_finish(pf_qm) and only clears
it in hisi_qm_reset_done() -> qm_reset_bit_clear():

drivers/crypto/hisilicon/qm.c:
qm_reset_prepare_ready() {
	struct pci_dev *pdev = qm->pdev;
	struct hisi_qm *pf_qm = pci_get_drvdata(pci_physfn(pdev));

	if (qm->ver < QM_HW_V3)
		return qm_wait_reset_finish(pf_qm);

	return qm_wait_reset_finish(qm);
}

Because the first VF's .reset_done() has not run yet, every subsequent VF's
.reset_prepare() and the PF's own hisi_qm_reset_prepare() will spin waiting
for that bit on pf_qm->misc_ctl until the timeout expires, log an error, and
return early without stopping the remaining VFs or the PF prior to the
hardware reset.

On Kunpeng930 (where qm->ver >= QM_HW_V3) with host-bound VFs, each VF's
hisi_qm_reset_prepare() sets the bit on its own vf_qm->misc_ctl. When the PF
subsequently runs hisi_qm_reset_prepare(), it notifies VFs via mailbox and
waits for them:

drivers/crypto/hisilicon/qm.c:
hisi_qm_reset_prepare()
  -> qm_try_stop_vfs()
       -> qm_ping_all_vfs()
       -> qm_wait_vf_prepare_finish()

Each VF's mailbox work handler (qm_pf_reset_vf_process() ->
qm_pf_reset_vf_prepare() -> qm_reset_prepare_ready(vf_qm)) then tries to
acquire the bit on vf_qm->misc_ctl that is already held by that VF's own
.reset_prepare(), timing out and causing the PF reset preparation to fail as
well.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027160446.858300-1-lukasz.laguna@intel.com?part=4