weiping zhang
2017-Dec-11 15:53 UTC
[PATCH 0/3] fix cleanup for fail to register_virtio_device
This series fix the cleanup for the caller of register_virtio_device, the main work is use put_device instead of kfree. weiping zhang (3): virtio_pci: use put_device instead of kfree virtio: use put_device instead of kfree virtio: put reference count of virtio_device.dev drivers/misc/mic/vop/vop_main.c | 16 +++++++++------- drivers/remoteproc/remoteproc_virtio.c | 2 +- drivers/virtio/virtio_pci_common.c | 17 +++++++++-------- 3 files changed, 19 insertions(+), 16 deletions(-) -- 2.9.4
weiping zhang
2017-Dec-11 15:55 UTC
[PATCH 1/3] virtio_pci: use put_device instead of kfree
don't free vp_dev until vp_dev->vdev.dev.release be called. Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> --- drivers/virtio/virtio_pci_common.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c index 1c4797e..91d20f7 100644 --- a/drivers/virtio/virtio_pci_common.c +++ b/drivers/virtio/virtio_pci_common.c @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev, pci_set_master(pci_dev); rc = register_virtio_device(&vp_dev->vdev); - if (rc) - goto err_register; + if (rc) { + if (vp_dev->ioaddr) + virtio_pci_legacy_remove(vp_dev); + else + virtio_pci_modern_remove(vp_dev); + pci_disable_device(pci_dev); + put_device(&vp_dev->vdev.dev); + } - return 0; + return rc; -err_register: - if (vp_dev->ioaddr) - virtio_pci_legacy_remove(vp_dev); - else - virtio_pci_modern_remove(vp_dev); err_probe: pci_disable_device(pci_dev); err_enable_device: -- 2.9.4
don't free vp_vdev until vp_vdev.dev.release be called. Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> --- drivers/misc/mic/vop/vop_main.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c index a341938..8c716a0 100644 --- a/drivers/misc/mic/vop/vop_main.c +++ b/drivers/misc/mic/vop/vop_main.c @@ -452,10 +452,12 @@ static irqreturn_t vop_virtio_intr_handler(int irq, void *data) static void vop_virtio_release_dev(struct device *_d) { - /* - * No need for a release method similar to virtio PCI. - * Provide an empty one to avoid getting a warning from core. - */ + struct virtio_device *vdev + container_of(_d, struct virtio_device, dev); + struct _vop_vdev *vop_vdev + container_of(vdev, struct _vop_vdev, vdev); + + kfree(vop_vdev); } /* @@ -501,7 +503,9 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, dev_err(_vop_dev(vdev), "Failed to register vop device %u type %u\n", offset, type); - goto free_irq; + vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev); + put_device(&vdev->vdev.dev); + return ret; } writeq((u64)vdev, &vdev->dc->vdev); dev_dbg(_vop_dev(vdev), "%s: registered vop device %u type %u vdev %p\n", @@ -509,8 +513,6 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, return 0; -free_irq: - vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev); kfree: kfree(vdev); return ret; -- 2.9.4
weiping zhang
2017-Dec-11 15:55 UTC
[PATCH 3/3] virtio: put reference count of virtio_device.dev
rproc_virtio_dev_release will be called iff virtio_device.dev's refer count became to 0. Here we should put vdev.dev, and then rproc->dev's cleanup will be done in rproc_virtio_dev_release. Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> --- drivers/remoteproc/remoteproc_virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c index 2946348..b0633fd 100644 --- a/drivers/remoteproc/remoteproc_virtio.c +++ b/drivers/remoteproc/remoteproc_virtio.c @@ -327,7 +327,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id) ret = register_virtio_device(vdev); if (ret) { - put_device(&rproc->dev); + put_device(&vdev->dev); dev_err(dev, "failed to register vdev: %d\n", ret); goto out; } -- 2.9.4
Cornelia Huck
2017-Dec-12 10:00 UTC
[PATCH 1/3] virtio_pci: use put_device instead of kfree
On Mon, 11 Dec 2017 23:55:16 +0800 weiping zhang <zhangweiping at didichuxing.com> wrote:> don't free vp_dev until vp_dev->vdev.dev.release be called.Maybe add the same description as in your virtio_mmio patch so that it is clear why the kfree() is not ok?> > Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> > --- > drivers/virtio/virtio_pci_common.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > index 1c4797e..91d20f7 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev, > pci_set_master(pci_dev); > > rc = register_virtio_device(&vp_dev->vdev); > - if (rc) > - goto err_register; > + if (rc) { > + if (vp_dev->ioaddr) > + virtio_pci_legacy_remove(vp_dev); > + else > + virtio_pci_modern_remove(vp_dev); > + pci_disable_device(pci_dev); > + put_device(&vp_dev->vdev.dev); > + } > > - return 0; > + return rc; > > -err_register: > - if (vp_dev->ioaddr) > - virtio_pci_legacy_remove(vp_dev); > - else > - virtio_pci_modern_remove(vp_dev); > err_probe: > pci_disable_device(pci_dev); > err_enable_device:Otherwise, looks good. Reviewed-by: Cornelia Huck <cohuck at redhat.com>
On Mon, 11 Dec 2017 23:55:26 +0800 weiping zhang <zhangweiping at didichuxing.com> wrote:> don't free vp_vdev until vp_vdev.dev.release be called.Same comment as for the virtio_pci patch.> > Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> > --- > drivers/misc/mic/vop/vop_main.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c > index a341938..8c716a0 100644 > --- a/drivers/misc/mic/vop/vop_main.c > +++ b/drivers/misc/mic/vop/vop_main.c > @@ -452,10 +452,12 @@ static irqreturn_t vop_virtio_intr_handler(int irq, void *data) > > static void vop_virtio_release_dev(struct device *_d) > { > - /* > - * No need for a release method similar to virtio PCI. > - * Provide an empty one to avoid getting a warning from core. > - */ > + struct virtio_device *vdev > + container_of(_d, struct virtio_device, dev); > + struct _vop_vdev *vop_vdev > + container_of(vdev, struct _vop_vdev, vdev); > + > + kfree(vop_vdev); > } > > /* > @@ -501,7 +503,9 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, > dev_err(_vop_dev(vdev), > "Failed to register vop device %u type %u\n", > offset, type); > - goto free_irq; > + vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev); > + put_device(&vdev->vdev.dev); > + return ret; > } > writeq((u64)vdev, &vdev->dc->vdev); > dev_dbg(_vop_dev(vdev), "%s: registered vop device %u type %u vdev %p\n", > @@ -509,8 +513,6 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, > > return 0; > > -free_irq: > - vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev); > kfree: > kfree(vdev); > return ret;BTW, the kfree(vdev) in _vop_remove_device() is also wrong (needs to be a put_device() as well).
Cornelia Huck
2017-Dec-12 10:33 UTC
[PATCH 3/3] virtio: put reference count of virtio_device.dev
On Mon, 11 Dec 2017 23:55:38 +0800 weiping zhang <zhangweiping at didichuxing.com> wrote:> rproc_virtio_dev_release will be called iff virtio_device.dev's > refer count became to 0. Here we should put vdev.dev, and then > rproc->dev's cleanup will be done in rproc_virtio_dev_release. > > Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> > --- > drivers/remoteproc/remoteproc_virtio.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c > index 2946348..b0633fd 100644 > --- a/drivers/remoteproc/remoteproc_virtio.c > +++ b/drivers/remoteproc/remoteproc_virtio.c > @@ -327,7 +327,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id) > > ret = register_virtio_device(vdev); > if (ret) { > - put_device(&rproc->dev); > + put_device(&vdev->dev); > dev_err(dev, "failed to register vdev: %d\n", ret); > goto out; > }Uh, rproc_vdev is really weird. It embeds a struct virtio_device, and in addition a struct kref. At a glance, it seems to be correct, though. Your change is more obviously sane. Reviewed-by: Cornelia Huck <cohuck at redhat.com>
Maybe Matching Threads
- [PATCH 2/3] virtio: use put_device instead of kfree
- [PATCH 2/3] virtio: use put_device instead of kfree
- [PATCH v2 2/3] virtio: use put_device instead of kfree
- [PATCH 0/3] fix cleanup for fail to register_virtio_device
- [PATCH 0/3] fix cleanup for fail to register_virtio_device