Re: [PATCH v2 1/2] hisi_acc_vfio_pci: fix live migration enable conditions for PF passthrough

On 2026/8/24 16:42, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The `set_reset_flag` lock tracker is never reset to false after a successful lock acquisition, allowing subsequent failed resets to erroneously clear the global `QM_RESETTING` lock.
> --
> 
> --- Patch [1]: [PATCH v2 1/2] hisi_acc_vfio_pci: fix live migration enable conditions for PF passthrough ---
> commit e8e63b8a17edd44cc9b69e8ca3472ee6c656873c
> Author: Longfang Liu <[email protected]>
> 
> hisi_acc_vfio_pci: fix live migration enable conditions for PF passthrough
> 
> This commit fixes a crash in PF passthrough mode by adding validity
> checks for uninitialized PF pointers during device probe, error
> handling, and migration initialization.
> 
>> diff --git a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>> index 86362ec424a50..e95d0ab0f11ab 100644
>> --- a/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
>> +++ b/drivers/vfio/pci/hisilicon/hisi_acc_vfio_pci.c
> [ ... ]
>> @@ -1174,8 +1179,12 @@ static void hisi_acc_vf_pci_aer_reset_done(struct pci_dev *pdev)
>>  	struct hisi_acc_vf_core_device *hisi_acc_vdev = hisi_acc_drvdata(pdev);
>>  	struct hisi_qm *qm = hisi_acc_vdev->pf_qm;
>>  
>> -	if (hisi_acc_vdev->set_reset_flag)
>> -		clear_bit(QM_RESETTING, &qm->misc_ctl);
>> +	if (hisi_acc_vdev->set_reset_flag) {
>> +		if (qm && qm->io_base)
>> +			clear_bit(QM_RESETTING, &qm->misc_ctl);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but is it possible that
> hisi_acc_vdev->set_reset_flag is never reset to false after a successful
> lock acquisition?
> 
> If a prior reset succeeds, hisi_acc_vdev->set_reset_flag is left as true.
> 
> If a subsequent reset attempt times out in hisi_acc_vf_pci_reset_prepare()
> because another agent already holds the QM_RESETTING lock:
> 
> hisi_acc_vf_pci_reset_prepare() {
>     ...
>     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;
>         }
>     }
>     ...
> }
> 
Your analysis makes sense. After `reset_done`, if subsequent reset processing occurs,
`set_reset_flag` remains set to true. Therefore, the reset flag handling will be
executed regardless of whether `reset_prepare` succeeds or not,
which is unreasonable.

Thanks.
Longfang.

> The code returns early and leaves set_reset_flag as true.
> 
> When hisi_acc_vf_pci_aer_reset_done() is subsequently invoked by the PCI
> error recovery flow, it observes the stale true flag and unconditionally
> clears QM_RESETTING.
> 
> Does this prematurely clear the lock currently owned by the other agent?
> 
>> +		else
>> +			dev_err(&pdev->dev, "PF QM not available for reset done\n");
>> +	}
>>  
>>  	if (!hisi_acc_vdev->core_device.vdev.mig_ops)
>>  		return;
>