Hi, The main change is split device_register into 2 sperate calls: device_initalize() and device_add, and then the caller can use put_device safety when fail to register_virtio_device. v3->v4: * split device_register into device_initialize and devicea_add that the caller can always use put_device when fail to register virtio device. v2->v3: * virtio: add new helper do get device's status then determine use put_device or kfree. v1->v2: * virtio_pci: add comments in commit message for why using put_device * virtio_vop: also use put_device int _vop_remove_device weiping zhang (4): virtio: split device_register into device_initialize and device_add virtio_pci: don't kfree device on register failure virtio_vop: don't kfree device on register failure virtio_remoteproc: don't kfree device on register failure drivers/misc/mic/vop/vop_main.c | 20 +++++++++++++------- drivers/remoteproc/remoteproc_virtio.c | 13 +++++++++++-- drivers/virtio/virtio.c | 18 +++++++++++++++--- drivers/virtio/virtio_pci_common.c | 8 ++++++-- 4 files changed, 45 insertions(+), 14 deletions(-) -- 2.9.4
weiping zhang
2017-Dec-20 04:26 UTC
[PATCH v4 1/4] virtio: split device_register into device_initialize and device_add
In order to make caller do a simple cleanup, we split device_register into device_initialize and device_add. device_initialize always sucess, the caller can always use put_device when fail to register virtio_device no matter fail at ida_simple_get or at device_add. Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> Suggested-by: Cornelia Huck <cohuck at redhat.com> --- drivers/virtio/virtio.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index bf7ff39..3c9f211 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -303,11 +303,21 @@ void unregister_virtio_driver(struct virtio_driver *driver) } EXPORT_SYMBOL_GPL(unregister_virtio_driver); +/** + * register_virtio_device - register virtio device + * @dev : virtio device interested + * + * If an error occurs, the caller must use put_device, instead of kfree, because + * device_initialize and device_add will increase @dev->dev's reference count. + * + * Returns: 0 on suceess, -error on failure + */ int register_virtio_device(struct virtio_device *dev) { int err; dev->dev.bus = &virtio_bus; + device_initialize(&dev->dev); /* Assign a unique device index and hence name. */ err = ida_simple_get(&virtio_index_ida, 0, 0, GFP_KERNEL); @@ -330,9 +340,11 @@ int register_virtio_device(struct virtio_device *dev) INIT_LIST_HEAD(&dev->vqs); - /* device_register() causes the bus infrastructure to look for a - * matching driver. */ - err = device_register(&dev->dev); + /* + * device_add() causes the bus infrastructure to look for a matching + * driver. + */ + err = device_add(&dev->dev); if (err) ida_simple_remove(&virtio_index_ida, dev->index); out: -- 2.9.4
weiping zhang
2017-Dec-20 04:26 UTC
[PATCH v4 2/4] virtio_pci: don't kfree device on register failure
As mentioned at drivers/base/core.c: /* * NOTE: _Never_ directly free @dev after calling this function, even * if it returned an error! Always use put_device() to give up the * reference initialized in this function instead. */ so we 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 | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c index 1c4797e..48d4d1c 100644 --- a/drivers/virtio/virtio_pci_common.c +++ b/drivers/virtio/virtio_pci_common.c @@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d) static int virtio_pci_probe(struct pci_dev *pci_dev, const struct pci_device_id *id) { - struct virtio_pci_device *vp_dev; + struct virtio_pci_device *vp_dev, *reg_dev = NULL; int rc; /* allocate our structure and fill it out */ @@ -551,6 +551,7 @@ static int virtio_pci_probe(struct pci_dev *pci_dev, pci_set_master(pci_dev); rc = register_virtio_device(&vp_dev->vdev); + reg_dev = vp_dev; if (rc) goto err_register; @@ -564,7 +565,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev, err_probe: pci_disable_device(pci_dev); err_enable_device: - kfree(vp_dev); + if (reg_dev) + put_device(&vp_dev->vdev.dev); + else + kfree(vp_dev); return rc; } -- 2.9.4
weiping zhang
2017-Dec-20 04:27 UTC
[PATCH v4 3/4] virtio_vop: don't kfree device on register failure
As mentioned at drivers/base/core.c: /* * NOTE: _Never_ directly free @dev after calling this function, even * if it returned an error! Always use put_device() to give up the * reference initialized in this function instead. */ so we don't free vdev until vdev->vdev.dev.release be called. Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> --- drivers/misc/mic/vop/vop_main.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c index a341938..3633202 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); } /* @@ -466,7 +468,7 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, unsigned int offset, struct vop_device *vpdev, int dnode) { - struct _vop_vdev *vdev; + struct _vop_vdev *vdev, *reg_dev = NULL; int ret; u8 type = ioread8(&d->type); @@ -497,6 +499,7 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, vdev->c2h_vdev_db = ioread8(&vdev->dc->c2h_vdev_db); ret = register_virtio_device(&vdev->vdev); + reg_dev = vdev; if (ret) { dev_err(_vop_dev(vdev), "Failed to register vop device %u type %u\n", @@ -512,7 +515,10 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, free_irq: vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev); kfree: - kfree(vdev); + if (reg_dev) + put_device(&vdev->vdev.dev); + else + kfree(vdev); return ret; } @@ -568,7 +574,7 @@ static int _vop_remove_device(struct mic_device_desc __iomem *d, iowrite8(-1, &dc->h2c_vdev_db); if (status & VIRTIO_CONFIG_S_DRIVER_OK) wait_for_completion(&vdev->reset_done); - kfree(vdev); + put_device(&vdev->vdev.dev); iowrite8(1, &dc->guest_ack); dev_dbg(&vpdev->dev, "%s %d guest_ack %d\n", __func__, __LINE__, ioread8(&dc->guest_ack)); -- 2.9.4
weiping zhang
2017-Dec-20 04:27 UTC
[PATCH v4 4/4] virtio_remoteproc: don't kfree device on register failure
rproc_virtio_dev_release will be called iff virtio_device.dev's refer count became to 0. Here we should check if we call device_register or not, if called, put vdev.dev, and then rproc->dev's cleanup will be done in rproc_virtio_dev_release, otherwise we do cleanup directly. Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> --- drivers/remoteproc/remoteproc_virtio.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c index 2946348..1073ea3 100644 --- a/drivers/remoteproc/remoteproc_virtio.c +++ b/drivers/remoteproc/remoteproc_virtio.c @@ -304,7 +304,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id) { struct rproc *rproc = rvdev->rproc; struct device *dev = &rproc->dev; - struct virtio_device *vdev = &rvdev->vdev; + struct virtio_device *vdev = &rvdev->vdev, *reg_dev = NULL; int ret; vdev->id.device = id, @@ -326,15 +326,24 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id) kref_get(&rvdev->refcount); ret = register_virtio_device(vdev); + reg_dev = vdev; if (ret) { - put_device(&rproc->dev); dev_err(dev, "failed to register vdev: %d\n", ret); goto out; } dev_info(dev, "registered %s (type %d)\n", dev_name(&vdev->dev), id); + return 0; + out: + if (reg_dev) + put_device(&vdev->dev); + else { + kref_put(&rvdev->refcount, rproc_vdev_release); + put_device(&rproc->dev); + } + return ret; } -- 2.9.4
Cornelia Huck
2017-Dec-20 15:53 UTC
[PATCH v4 1/4] virtio: split device_register into device_initialize and device_add
On Wed, 20 Dec 2017 12:26:25 +0800 weiping zhang <zwp10758 at gmail.com> wrote: [you used a different mail address in your From: than in your s-o-b:; same for the other patches]> In order to make caller do a simple cleanup, we split device_register > into device_initialize and device_add. device_initialize always sucess,s/success/succeeds/> the caller can always use put_device when fail to register virtio_device"so the caller can always use put_device when register_virtio_device failed,"> no matter fail at ida_simple_get or at device_add."no matter whether it failed..."> > Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> > Suggested-by: Cornelia Huck <cohuck at redhat.com> > --- > drivers/virtio/virtio.c | 18 +++++++++++++++--- > 1 file changed, 15 insertions(+), 3 deletions(-) > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c > index bf7ff39..3c9f211 100644 > --- a/drivers/virtio/virtio.c > +++ b/drivers/virtio/virtio.c > @@ -303,11 +303,21 @@ void unregister_virtio_driver(struct virtio_driver *driver) > } > EXPORT_SYMBOL_GPL(unregister_virtio_driver); > > +/** > + * register_virtio_device - register virtio device > + * @dev : virtio device interested"virtio device to be registered"> + * > + * If an error occurs, the caller must use put_device, instead of kfree, because > + * device_initialize and device_add will increase @dev->dev's reference count.That's not correct: It's not because of device_add increasing the reference count (it releases it again on failure), but because another code path may have obtained a reference. What about: "On error, the caller must call put_device on &@dev->dev (and not kfree), as another code path may have obtained a reference to @dev."> + * > + * Returns: 0 on suceess, -error on failure > + */ > int register_virtio_device(struct virtio_device *dev) > { > int err; > > dev->dev.bus = &virtio_bus; > + device_initialize(&dev->dev); > > /* Assign a unique device index and hence name. */ > err = ida_simple_get(&virtio_index_ida, 0, 0, GFP_KERNEL); > @@ -330,9 +340,11 @@ int register_virtio_device(struct virtio_device *dev) > > INIT_LIST_HEAD(&dev->vqs); > > - /* device_register() causes the bus infrastructure to look for a > - * matching driver. */ > - err = device_register(&dev->dev); > + /* > + * device_add() causes the bus infrastructure to look for a matching > + * driver.FWIW, I would just have done s/device_register/device_add/ in the comment, but this is ok as well.> + */ > + err = device_add(&dev->dev); > if (err) > ida_simple_remove(&virtio_index_ida, dev->index); > out:Your code change is fine.
Cornelia Huck
2017-Dec-20 15:55 UTC
[PATCH v4 2/4] virtio_pci: don't kfree device on register failure
On Wed, 20 Dec 2017 12:26:43 +0800 weiping zhang <zwp10758 at gmail.com> wrote:> As mentioned at drivers/base/core.c: > /* > * NOTE: _Never_ directly free @dev after calling this function, even > * if it returned an error! Always use put_device() to give up the > * reference initialized in this function instead. > */ > so we 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 | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c > index 1c4797e..48d4d1c 100644 > --- a/drivers/virtio/virtio_pci_common.c > +++ b/drivers/virtio/virtio_pci_common.c > @@ -513,7 +513,7 @@ static void virtio_pci_release_dev(struct device *_d) > static int virtio_pci_probe(struct pci_dev *pci_dev, > const struct pci_device_id *id) > { > - struct virtio_pci_device *vp_dev; > + struct virtio_pci_device *vp_dev, *reg_dev = NULL;Not sure if I would have used a pointer for this purpose, but as that is what Michael had proposed...> int rc; > > /* allocate our structure and fill it out */ > @@ -551,6 +551,7 @@ static int virtio_pci_probe(struct pci_dev *pci_dev, > pci_set_master(pci_dev); > > rc = register_virtio_device(&vp_dev->vdev); > + reg_dev = vp_dev; > if (rc) > goto err_register; > > @@ -564,7 +565,10 @@ static int virtio_pci_probe(struct pci_dev *pci_dev, > err_probe: > pci_disable_device(pci_dev); > err_enable_device: > - kfree(vp_dev); > + if (reg_dev) > + put_device(&vp_dev->vdev.dev); > + else > + kfree(vp_dev); > return rc; > } >Reviewed-by: Cornelia Huck <cohuck at redhat.com>
Cornelia Huck
2017-Dec-20 15:57 UTC
[PATCH v4 3/4] virtio_vop: don't kfree device on register failure
On Wed, 20 Dec 2017 12:27:04 +0800 weiping zhang <zwp10758 at gmail.com> wrote:> As mentioned at drivers/base/core.c: > /* > * NOTE: _Never_ directly free @dev after calling this function, even > * if it returned an error! Always use put_device() to give up the > * reference initialized in this function instead. > */ > so we don't free vdev until vdev->vdev.dev.release be called. > > Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> > --- > drivers/misc/mic/vop/vop_main.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > > diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c > index a341938..3633202 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); > } > > /* > @@ -466,7 +468,7 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, > unsigned int offset, struct vop_device *vpdev, > int dnode) > { > - struct _vop_vdev *vdev; > + struct _vop_vdev *vdev, *reg_dev = NULL;Similarly, not a fan of that pointer variable, but it's fine.> int ret; > u8 type = ioread8(&d->type); > > @@ -497,6 +499,7 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, > vdev->c2h_vdev_db = ioread8(&vdev->dc->c2h_vdev_db); > > ret = register_virtio_device(&vdev->vdev); > + reg_dev = vdev; > if (ret) { > dev_err(_vop_dev(vdev), > "Failed to register vop device %u type %u\n", > @@ -512,7 +515,10 @@ static int _vop_add_device(struct mic_device_desc __iomem *d, > free_irq: > vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev); > kfree: > - kfree(vdev); > + if (reg_dev) > + put_device(&vdev->vdev.dev); > + else > + kfree(vdev); > return ret; > } > > @@ -568,7 +574,7 @@ static int _vop_remove_device(struct mic_device_desc __iomem *d, > iowrite8(-1, &dc->h2c_vdev_db); > if (status & VIRTIO_CONFIG_S_DRIVER_OK) > wait_for_completion(&vdev->reset_done); > - kfree(vdev); > + put_device(&vdev->vdev.dev); > iowrite8(1, &dc->guest_ack); > dev_dbg(&vpdev->dev, "%s %d guest_ack %d\n", > __func__, __LINE__, ioread8(&dc->guest_ack));Reviewed-by: Cornelia Huck <cohuck at redhat.com>
Cornelia Huck
2017-Dec-20 16:12 UTC
[PATCH v4 4/4] virtio_remoteproc: don't kfree device on register failure
On Wed, 20 Dec 2017 12:27:33 +0800 weiping zhang <zwp10758 at gmail.com> wrote:> rproc_virtio_dev_release will be called iff virtio_device.dev's > refer count became to 0. Here we should check if we call device_register"reference count drops to 0" s/call/called/> or not, if called, put vdev.dev, and then rproc->dev's cleanup will be > done in rproc_virtio_dev_release, otherwise we do cleanup directly. > > Signed-off-by: weiping zhang <zhangweiping at didichuxing.com> > --- > drivers/remoteproc/remoteproc_virtio.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c > index 2946348..1073ea3 100644 > --- a/drivers/remoteproc/remoteproc_virtio.c > +++ b/drivers/remoteproc/remoteproc_virtio.c > @@ -304,7 +304,7 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id) > { > struct rproc *rproc = rvdev->rproc; > struct device *dev = &rproc->dev; > - struct virtio_device *vdev = &rvdev->vdev; > + struct virtio_device *vdev = &rvdev->vdev, *reg_dev = NULL; > int ret; > > vdev->id.device = id, > @@ -326,15 +326,24 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id) > kref_get(&rvdev->refcount); > > ret = register_virtio_device(vdev); > + reg_dev = vdev; > if (ret) { > - put_device(&rproc->dev); > dev_err(dev, "failed to register vdev: %d\n", ret); > goto out; > } > > dev_info(dev, "registered %s (type %d)\n", dev_name(&vdev->dev), id); > > + return 0; > + > out: > + if (reg_dev) > + put_device(&vdev->dev); > + else { > + kref_put(&rvdev->refcount, rproc_vdev_release); > + put_device(&rproc->dev); > + } > + > return ret; > } >I think in this case using the marker makes a straightforward cleanup way too complicated. There's a single way we can get to the out label, and that's when register_virtio_device() failed. Switching put_device(&rproc->dev) to put_device(@vdev->dev) (what your first patch did) seems like the way to go. (It also may be good to cc: the maintainers for this driver.)
Seemingly Similar Threads
- [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add
- [PATCH v5 1/4] virtio: split device_register into device_initialize and device_add
- [PATCH v4 0/4] use put_device to cleanup resource
- [PATCH v4 0/4] use put_device to cleanup resource
- [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add