This series implements vDPA management netlink for ifcvf.
Please help review
Thanks!
Zhu Lingshan (3):
vDPA/ifcvf: introduce get_dev_type() which returns virtio dev id
vDPA/ifcvf: implement management netlink framework for ifcvf
vDPA/ifcvf: set_status() should get a adapter from the mgmt dev
drivers/vdpa/ifcvf/ifcvf_base.h | 6 +
drivers/vdpa/ifcvf/ifcvf_main.c | 192 ++++++++++++++++++++++++--------
2 files changed, 149 insertions(+), 49 deletions(-)
--
2.27.0
This commit introduces a new function get_dev_type() which returns
the virtio device id of a device, to avoid duplicated code.
Signed-off-by: Zhu Lingshan <redacted>
---
drivers/vdpa/ifcvf/ifcvf_main.c | 34 ++++++++++++++++++++-------------
1 file changed, 21 insertions(+), 13 deletions(-)
@@ -442,6 +442,26 @@ static const struct vdpa_config_ops ifc_vdpa_ops = {.set_config_cb=ifcvf_vdpa_set_config_cb,};+staticu32get_dev_type(structpci_dev*pdev)+{+u32dev_type;++/* This drirver drives both modern virtio devices and transitional+*devicesinmodernmode.+*vDPArequiresfeaturebitVIRTIO_F_ACCESS_PLATFORM,+*solegacydevicesandtransitionaldevicesinlegacy+*modewillnotworkforvDPA,thisdriverwillnot+*drivedeviceswithlegacyinterface.+*/++if(pdev->device<0x1040)+dev_type=pdev->subsystem_device;+else+dev_type=pdev->device-0x1040;++returndev_type;+}+staticintifcvf_probe(structpci_dev*pdev,conststructpci_device_id*id){structdevice*dev=&pdev->dev;
@@ -486,19 +506,7 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)pci_set_drvdata(pdev,adapter);vf=&adapter->vf;--/* This drirver drives both modern virtio devices and transitional-*devicesinmodernmode.-*vDPArequiresfeaturebitVIRTIO_F_ACCESS_PLATFORM,-*solegacydevicesandtransitionaldevicesinlegacy-*modewillnotworkforvDPA,thisdriverwillnot-*drivedeviceswithlegacyinterface.-*/-if(pdev->device<0x1040)-vf->dev_type=pdev->subsystem_device;-else-vf->dev_type=pdev->device-0x1040;-+vf->dev_type=get_dev_type(pdev);vf->base=pcim_iomap_table(pdev);adapter->pdev=pdev;
@@ -462,48 +472,30 @@ static u32 get_dev_type(struct pci_dev *pdev)returndev_type;}-staticintifcvf_probe(structpci_dev*pdev,conststructpci_device_id*id)+staticintifcvf_vdpa_dev_add(structvdpa_mgmt_dev*mdev,constchar*name){-structdevice*dev=&pdev->dev;+structifcvf_vdpa_mgmt_dev*ifcvf_mgmt_dev;structifcvf_adapter*adapter;+structpci_dev*pdev;structifcvf_hw*vf;+structdevice*dev;intret,i;-ret=pcim_enable_device(pdev);-if(ret){-IFCVF_ERR(pdev,"Failed to enable device\n");-returnret;-}--ret=pcim_iomap_regions(pdev,BIT(0)|BIT(2)|BIT(4),-IFCVF_DRIVER_NAME);-if(ret){-IFCVF_ERR(pdev,"Failed to request MMIO region\n");-returnret;-}--ret=dma_set_mask_and_coherent(dev,DMA_BIT_MASK(64));-if(ret){-IFCVF_ERR(pdev,"No usable DMA configuration\n");-returnret;-}--ret=devm_add_action_or_reset(dev,ifcvf_free_irq_vectors,pdev);-if(ret){-IFCVF_ERR(pdev,-"Failed for adding devres for freeing irq vectors\n");-returnret;-}+ifcvf_mgmt_dev=container_of(mdev,structifcvf_vdpa_mgmt_dev,mdev);+if(ifcvf_mgmt_dev->adapter)+return-EOPNOTSUPP;+pdev=ifcvf_mgmt_dev->pdev;+dev=&pdev->dev;adapter=vdpa_alloc_device(structifcvf_adapter,vdpa,-dev,&ifc_vdpa_ops,NULL);-if(adapter==NULL){+dev,&ifc_vdpa_ops,name);+if(!adapter){IFCVF_ERR(pdev,"Failed to allocate vDPA structure");return-ENOMEM;}-pci_set_master(pdev);-pci_set_drvdata(pdev,adapter);+ifcvf_mgmt_dev->adapter=adapter;+pci_set_drvdata(pdev,ifcvf_mgmt_dev);vf=&adapter->vf;vf->dev_type=get_dev_type(pdev);
@@ -515,7 +507,7 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)ret=ifcvf_init_hw(vf,pdev);if(ret){IFCVF_ERR(pdev,"Failed to init IFCVF hw\n");-gotoerr;+returnret;}for(i=0;i<IFCVF_MAX_QUEUE_PAIRS*2;i++)
@@ -523,9 +515,94 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)vf->hw_features=ifcvf_get_hw_features(vf);-ret=vdpa_register_device(&adapter->vdpa,IFCVF_MAX_QUEUE_PAIRS*2);+adapter->vdpa.mdev=&ifcvf_mgmt_dev->mdev;+ret=_vdpa_register_device(&adapter->vdpa,IFCVF_MAX_QUEUE_PAIRS*2);if(ret){-IFCVF_ERR(pdev,"Failed to register ifcvf to vdpa bus");+IFCVF_ERR(pdev,"Failed to register to vDPA bus");+returnret;+}++return0;+}++staticvoidifcvf_vdpa_dev_del(structvdpa_mgmt_dev*mdev,structvdpa_device*dev)+{+structifcvf_vdpa_mgmt_dev*ifcvf_mgmt_dev;++ifcvf_mgmt_dev=container_of(mdev,structifcvf_vdpa_mgmt_dev,mdev);+_vdpa_unregister_device(dev);+ifcvf_mgmt_dev->adapter=NULL;+}++staticconststructvdpa_mgmtdev_opsifcvf_vdpa_mgmt_dev_ops={+.dev_add=ifcvf_vdpa_dev_add,+.dev_del=ifcvf_vdpa_dev_del+};++staticintifcvf_probe(structpci_dev*pdev,conststructpci_device_id*id)+{+structifcvf_vdpa_mgmt_dev*ifcvf_mgmt_dev;+structdevice*dev=&pdev->dev;+structifcvf_adapter*adapter;+u32dev_type;+intret;++ifcvf_mgmt_dev=kzalloc(sizeof(structifcvf_vdpa_mgmt_dev),GFP_KERNEL);+if(!ifcvf_mgmt_dev){+IFCVF_ERR(pdev,"Failed to alloc memory for the vDPA management device\n");+return-ENOMEM;+}++dev_type=get_dev_type(pdev);+switch(dev_type){+caseVIRTIO_ID_NET:+ifcvf_mgmt_dev->mdev.id_table=id_table_net;+break;+caseVIRTIO_ID_BLOCK:+ifcvf_mgmt_dev->mdev.id_table=id_table_blk;+break;+default:+IFCVF_ERR(pdev,"VIRTIO ID %u not supported\n",dev_type);+ret=-EOPNOTSUPP;+gotoerr;+}++ifcvf_mgmt_dev->mdev.ops=&ifcvf_vdpa_mgmt_dev_ops;+ifcvf_mgmt_dev->mdev.device=dev;+ifcvf_mgmt_dev->pdev=pdev;++ret=pcim_enable_device(pdev);+if(ret){+IFCVF_ERR(pdev,"Failed to enable device\n");+gotoerr;+}++ret=pcim_iomap_regions(pdev,BIT(0)|BIT(2)|BIT(4),+IFCVF_DRIVER_NAME);+if(ret){+IFCVF_ERR(pdev,"Failed to request MMIO region\n");+gotoerr;+}++ret=dma_set_mask_and_coherent(dev,DMA_BIT_MASK(64));+if(ret){+IFCVF_ERR(pdev,"No usable DMA configuration\n");+gotoerr;+}++ret=devm_add_action_or_reset(dev,ifcvf_free_irq_vectors,pdev);+if(ret){+IFCVF_ERR(pdev,+"Failed for adding devres for freeing irq vectors\n");+gotoerr;+}++pci_set_master(pdev);++ret=vdpa_mgmtdev_register(&ifcvf_mgmt_dev->mdev);+if(ret){+IFCVF_ERR(pdev,+"Failed to initialize the management interfaces\n");gotoerr;}
ifcvf_vdpa_set_status() should get a adapter from the
management device
Signed-off-by: Zhu Lingshan <redacted>
---
drivers/vdpa/ifcvf/ifcvf_main.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
drivers/vdpa/ifcvf/ifcvf_main.c:612:14: warning: variable 'adapter' is uninitialized when used here [-Wuninitialized]
put_device(&adapter->vdpa.dev);
^~~~~~~
drivers/vdpa/ifcvf/ifcvf_main.c:546:31: note: initialize the variable 'adapter' to silence this warning
struct ifcvf_adapter *adapter;
^
= NULL
1 warning generated.
Actually the problem is real and this is almost surely the wrong fix.
We need an extra label to skip using put_device when adapter was not
yet initialized.
@@ -462,48 +472,30 @@ static u32 get_dev_type(struct pci_dev *pdev)returndev_type;}-staticintifcvf_probe(structpci_dev*pdev,conststructpci_device_id*id)+staticintifcvf_vdpa_dev_add(structvdpa_mgmt_dev*mdev,constchar*name){-structdevice*dev=&pdev->dev;+structifcvf_vdpa_mgmt_dev*ifcvf_mgmt_dev;structifcvf_adapter*adapter;+structpci_dev*pdev;structifcvf_hw*vf;+structdevice*dev;intret,i;-ret=pcim_enable_device(pdev);-if(ret){-IFCVF_ERR(pdev,"Failed to enable device\n");-returnret;-}--ret=pcim_iomap_regions(pdev,BIT(0)|BIT(2)|BIT(4),-IFCVF_DRIVER_NAME);-if(ret){-IFCVF_ERR(pdev,"Failed to request MMIO region\n");-returnret;-}--ret=dma_set_mask_and_coherent(dev,DMA_BIT_MASK(64));-if(ret){-IFCVF_ERR(pdev,"No usable DMA configuration\n");-returnret;-}--ret=devm_add_action_or_reset(dev,ifcvf_free_irq_vectors,pdev);-if(ret){-IFCVF_ERR(pdev,-"Failed for adding devres for freeing irq vectors\n");-returnret;-}+ifcvf_mgmt_dev=container_of(mdev,structifcvf_vdpa_mgmt_dev,mdev);+if(ifcvf_mgmt_dev->adapter)+return-EOPNOTSUPP;+pdev=ifcvf_mgmt_dev->pdev;+dev=&pdev->dev;adapter=vdpa_alloc_device(structifcvf_adapter,vdpa,-dev,&ifc_vdpa_ops,NULL);-if(adapter==NULL){+dev,&ifc_vdpa_ops,name);+if(!adapter){IFCVF_ERR(pdev,"Failed to allocate vDPA structure");return-ENOMEM;}-pci_set_master(pdev);-pci_set_drvdata(pdev,adapter);+ifcvf_mgmt_dev->adapter=adapter;+pci_set_drvdata(pdev,ifcvf_mgmt_dev);vf=&adapter->vf;vf->dev_type=get_dev_type(pdev);
@@ -515,7 +507,7 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)ret=ifcvf_init_hw(vf,pdev);if(ret){IFCVF_ERR(pdev,"Failed to init IFCVF hw\n");-gotoerr;+returnret;}for(i=0;i<IFCVF_MAX_QUEUE_PAIRS*2;i++)
@@ -523,9 +515,94 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)vf->hw_features=ifcvf_get_hw_features(vf);-ret=vdpa_register_device(&adapter->vdpa,IFCVF_MAX_QUEUE_PAIRS*2);+adapter->vdpa.mdev=&ifcvf_mgmt_dev->mdev;+ret=_vdpa_register_device(&adapter->vdpa,IFCVF_MAX_QUEUE_PAIRS*2);if(ret){-IFCVF_ERR(pdev,"Failed to register ifcvf to vdpa bus");+IFCVF_ERR(pdev,"Failed to register to vDPA bus");+returnret;+}++return0;+}++staticvoidifcvf_vdpa_dev_del(structvdpa_mgmt_dev*mdev,structvdpa_device*dev)+{+structifcvf_vdpa_mgmt_dev*ifcvf_mgmt_dev;++ifcvf_mgmt_dev=container_of(mdev,structifcvf_vdpa_mgmt_dev,mdev);+_vdpa_unregister_device(dev);+ifcvf_mgmt_dev->adapter=NULL;+}++staticconststructvdpa_mgmtdev_opsifcvf_vdpa_mgmt_dev_ops={+.dev_add=ifcvf_vdpa_dev_add,+.dev_del=ifcvf_vdpa_dev_del+};++staticintifcvf_probe(structpci_dev*pdev,conststructpci_device_id*id)+{+structifcvf_vdpa_mgmt_dev*ifcvf_mgmt_dev;+structdevice*dev=&pdev->dev;+structifcvf_adapter*adapter;
adapter is not used.
quoted hunk
+ u32 dev_type;
+ int ret;
+
+ ifcvf_mgmt_dev = kzalloc(sizeof(struct ifcvf_vdpa_mgmt_dev), GFP_KERNEL);
+ if (!ifcvf_mgmt_dev) {
+ IFCVF_ERR(pdev, "Failed to alloc memory for the vDPA management device\n");
+ return -ENOMEM;
+ }
+
+ dev_type = get_dev_type(pdev);
+ switch (dev_type) {
+ case VIRTIO_ID_NET:
+ ifcvf_mgmt_dev->mdev.id_table = id_table_net;
+ break;
+ case VIRTIO_ID_BLOCK:
+ ifcvf_mgmt_dev->mdev.id_table = id_table_blk;
+ break;
+ default:
+ IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", dev_type);
+ ret = -EOPNOTSUPP;
+ goto err;
+ }
+
+ ifcvf_mgmt_dev->mdev.ops = &ifcvf_vdpa_mgmt_dev_ops;
+ ifcvf_mgmt_dev->mdev.device = dev;
+ ifcvf_mgmt_dev->pdev = pdev;
+
+ ret = pcim_enable_device(pdev);
+ if (ret) {
+ IFCVF_ERR(pdev, "Failed to enable device\n");
+ goto err;
+ }
+
+ ret = pcim_iomap_regions(pdev, BIT(0) | BIT(2) | BIT(4),
+ IFCVF_DRIVER_NAME);
+ if (ret) {
+ IFCVF_ERR(pdev, "Failed to request MMIO region\n");
+ goto err;
+ }
+
+ ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
+ if (ret) {
+ IFCVF_ERR(pdev, "No usable DMA configuration\n");
+ goto err;
+ }
+
+ ret = devm_add_action_or_reset(dev, ifcvf_free_irq_vectors, pdev);
+ if (ret) {
+ IFCVF_ERR(pdev,
+ "Failed for adding devres for freeing irq vectors\n");
+ goto err;
+ }
+
+ pci_set_master(pdev);
+
+ ret = vdpa_mgmtdev_register(&ifcvf_mgmt_dev->mdev);
+ if (ret) {
+ IFCVF_ERR(pdev,
+ "Failed to initialize the management interfaces\n");
goto err;
}
From: Jason Wang <hidden> Date: 2021-07-05 05:09:35
在 2021/6/30 下午4:21, Zhu Lingshan 写道:
quoted hunk
ifcvf_vdpa_set_status() should get a adapter from the
management device
Signed-off-by: Zhu Lingshan <redacted>
---
drivers/vdpa/ifcvf/ifcvf_main.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
If this is a fix for patch 2, you need to squash this into that one.
Any reason that vdpa_to_adapter() can't work?
And I see:
+struct ifcvf_vdpa_mgmt_dev {
+ struct vdpa_mgmt_dev mdev;
+ struct ifcvf_adapter *adapter;
+ struct pci_dev *pdev;
+};
What's the reason for having a adapter pointer here?
Thanks
It's used in error handling below. It's not *initialized*.
sorry, my bad. I think I should move adapter related error handling code
into ifcvf_vdpa_dev_add(),
probe() does not see adapter anymore, only struct ifcvf_vdpa_mgmt_dev.
Thanks
quoted
quoted
+ u32 dev_type;
+ int ret;
+
+ ifcvf_mgmt_dev = kzalloc(sizeof(struct ifcvf_vdpa_mgmt_dev), GFP_KERNEL);
+ if (!ifcvf_mgmt_dev) {
+ IFCVF_ERR(pdev, "Failed to alloc memory for the vDPA management device\n");
+ return -ENOMEM;
+ }
+
+ dev_type = get_dev_type(pdev);
+ switch (dev_type) {
+ case VIRTIO_ID_NET:
+ ifcvf_mgmt_dev->mdev.id_table = id_table_net;
+ break;
+ case VIRTIO_ID_BLOCK:
+ ifcvf_mgmt_dev->mdev.id_table = id_table_blk;
+ break;
+ default:
+ IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", dev_type);
+ ret = -EOPNOTSUPP;
+ goto err;
+ }
+
+ ifcvf_mgmt_dev->mdev.ops = &ifcvf_vdpa_mgmt_dev_ops;
+ ifcvf_mgmt_dev->mdev.device = dev;
+ ifcvf_mgmt_dev->pdev = pdev;
+
+ ret = pcim_enable_device(pdev);
+ if (ret) {
+ IFCVF_ERR(pdev, "Failed to enable device\n");
+ goto err;
+ }
+
+ ret = pcim_iomap_regions(pdev, BIT(0) | BIT(2) | BIT(4),
+ IFCVF_DRIVER_NAME);
+ if (ret) {
+ IFCVF_ERR(pdev, "Failed to request MMIO region\n");
+ goto err;
+ }
+
+ ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
+ if (ret) {
+ IFCVF_ERR(pdev, "No usable DMA configuration\n");
+ goto err;
+ }
+
+ ret = devm_add_action_or_reset(dev, ifcvf_free_irq_vectors, pdev);
+ if (ret) {
+ IFCVF_ERR(pdev,
+ "Failed for adding devres for freeing irq vectors\n");
+ goto err;
+ }
+
+ pci_set_master(pdev);
+
+ ret = vdpa_mgmtdev_register(&ifcvf_mgmt_dev->mdev);
+ if (ret) {
+ IFCVF_ERR(pdev,
+ "Failed to initialize the management interfaces\n");
goto err;
}
ifcvf_vdpa_set_status() should get a adapter from the
management device
Signed-off-by: Zhu Lingshan <redacted>
---
drivers/vdpa/ifcvf/ifcvf_main.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c
b/drivers/vdpa/ifcvf/ifcvf_main.c
index 7c2f64ca2163..28c71eef1d2b 100644
If this is a fix for patch 2, you need to squash this into that one.
sure will squash it to patch 2
Any reason that vdpa_to_adapter() can't work?
will use it in V2.
And I see:
+struct ifcvf_vdpa_mgmt_dev {
+ struct vdpa_mgmt_dev mdev;
+ struct ifcvf_adapter *adapter;
+ struct pci_dev *pdev;
+};
What's the reason for having a adapter pointer here?
because in ifcvf_remove(), we need to get the management device from
pdev struct, so need to set the management device pointor
to the pdev drvdata, then need this *adapter pointor to address the adapter.
Thanks