On 2/25/2024 9:19 PM, Jason Wang wrote:
On Tue, Feb 20, 2024 at 9:11 AM Shannon Nelson <shannon.nelson@xxxxxxx> wrote:This addresses a couple of things found while testing the FLR and AER handling with the VFs. - release irqs before calling vp_modern_remove() - make sure we have a valid struct pointer before using it to release irqs - make sure the FW is alive before trying to add a new device Signed-off-by: Shannon Nelson <shannon.nelson@xxxxxxx> --- drivers/vdpa/pds/aux_drv.c | 2 +- drivers/vdpa/pds/vdpa_dev.c | 20 +++++++++++++++++--- drivers/vdpa/pds/vdpa_dev.h | 1 + 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/drivers/vdpa/pds/aux_drv.c b/drivers/vdpa/pds/aux_drv.c index 186e9ee22eb1..f57330cf9024 100644 --- a/drivers/vdpa/pds/aux_drv.c +++ b/drivers/vdpa/pds/aux_drv.c @@ -93,8 +93,8 @@ static void pds_vdpa_remove(struct auxiliary_device *aux_dev) struct device *dev = &aux_dev->dev; vdpa_mgmtdev_unregister(&vdpa_aux->vdpa_mdev); + pds_vdpa_release_irqs(vdpa_aux->pdsv); vp_modern_remove(&vdpa_aux->vd_mdev); - pci_free_irq_vectors(vdpa_aux->padev->vf_pdev); pds_vdpa_debugfs_del_vdpadev(vdpa_aux); kfree(vdpa_aux); diff --git a/drivers/vdpa/pds/vdpa_dev.c b/drivers/vdpa/pds/vdpa_dev.c index 25c0fe5ec3d5..301d95e08596 100644 --- a/drivers/vdpa/pds/vdpa_dev.c +++ b/drivers/vdpa/pds/vdpa_dev.c @@ -426,12 +426,18 @@ static int pds_vdpa_request_irqs(struct pds_vdpa_device *pdsv) return err; } -static void pds_vdpa_release_irqs(struct pds_vdpa_device *pdsv) +void pds_vdpa_release_irqs(struct pds_vdpa_device *pdsv) { - struct pci_dev *pdev = pdsv->vdpa_aux->padev->vf_pdev; - struct pds_vdpa_aux *vdpa_aux = pdsv->vdpa_aux; + struct pds_vdpa_aux *vdpa_aux; + struct pci_dev *pdev; int qid; + if (!pdsv) + return; + + pdev = pdsv->vdpa_aux->padev->vf_pdev; + vdpa_aux = pdsv->vdpa_aux; + if (!vdpa_aux->nintrs) return; @@ -612,6 +618,7 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, struct device *dma_dev; struct pci_dev *pdev; struct device *dev; + u8 status; int err; int i; @@ -638,6 +645,13 @@ static int pds_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, dma_dev = &pdev->dev; pdsv->vdpa_dev.dma_dev = dma_dev; + status = pds_vdpa_get_status(&pdsv->vdpa_dev); + if (status == 0xff) { + dev_err(dev, "Broken PCI - status %#x\n", status); + err = -ENXIO; + goto err_unmap; + }This seems a violation of the virtio spec. Note that spec has a DEVICE_NEEDS_RESET(64) or do we have a vendor specific way to detect this instead? Thanks
I think that we need a virtio device before we can say the device needs a reset, but there's no device here yet.
In this case, we haven't even been able to set up the device because, for whatever reason, the PCI connection is broken and we can't talk to the actual hardware that supports that device. Here we are throwing an error on the device creation before we can figure out any status for the device itself and there's no place to set a DEVICE_NEEDS_RESET flag.
sln
+ pdsv->supported_features = mgmt->supported_features; if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) { diff --git a/drivers/vdpa/pds/vdpa_dev.h b/drivers/vdpa/pds/vdpa_dev.h index d984ba24a7da..84bdb45871ff 100644 --- a/drivers/vdpa/pds/vdpa_dev.h +++ b/drivers/vdpa/pds/vdpa_dev.h @@ -46,5 +46,6 @@ struct pds_vdpa_device { #define PDS_VDPA_PACKED_INVERT_IDX 0x8000 +void pds_vdpa_release_irqs(struct pds_vdpa_device *pdsv); int pds_vdpa_get_mgmt_info(struct pds_vdpa_aux *vdpa_aux); #endif /* _VDPA_DEV_H_ */ -- 2.17.1
