* [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add
2017-12-20 4:26 [PATCH v4 0/4] use put_device to cleanup resource weiping zhang
@ 2017-12-20 4:26 ` weiping zhang
2017-12-20 15:53 ` Cornelia Huck
2017-12-20 4:26 ` [PATCH v4 2/4] virtio_pci: don't kfree device on register failure weiping zhang
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: weiping zhang @ 2017-12-20 4:26 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
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@didichuxing.com>
Suggested-by: Cornelia Huck <cohuck@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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add
2017-12-20 4:26 ` [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add weiping zhang
@ 2017-12-20 15:53 ` Cornelia Huck
2017-12-21 2:37 ` weiping zhang
0 siblings, 1 reply; 12+ messages in thread
From: Cornelia Huck @ 2017-12-20 15:53 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
On Wed, 20 Dec 2017 12:26:25 +0800
weiping zhang <zwp10758@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@didichuxing.com>
> Suggested-by: Cornelia Huck <cohuck@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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add
2017-12-20 15:53 ` Cornelia Huck
@ 2017-12-21 2:37 ` weiping zhang
2017-12-21 2:43 ` weiping zhang
0 siblings, 1 reply; 12+ messages in thread
From: weiping zhang @ 2017-12-21 2:37 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, Michael S . Tsirkin
2017-12-20 23:53 GMT+08:00 Cornelia Huck <cohuck@redhat.com>:
> On Wed, 20 Dec 2017 12:26:25 +0800
> weiping zhang <zwp10758@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..."
>
>> + *
>> + * 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."
>
It's good to understand, further more dev->name may has a bit mem leak.
anyway, I'll correct all comments at V5. Thanks very much.
>> + *
>> + * Returns: 0 on suceess, -error on failure
>> + */
>> int register_virtio_device(struct virtio_device *dev)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add
2017-12-21 2:37 ` weiping zhang
@ 2017-12-21 2:43 ` weiping zhang
0 siblings, 0 replies; 12+ messages in thread
From: weiping zhang @ 2017-12-21 2:43 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, Michael S . Tsirkin
2017-12-21 10:37 GMT+08:00 weiping zhang <zwp10758@gmail.com>:
> 2017-12-20 23:53 GMT+08:00 Cornelia Huck <cohuck@redhat.com>:
>> On Wed, 20 Dec 2017 12:26:25 +0800
>> weiping zhang <zwp10758@gmail.com> wrote:
>>
>> [you used a different mail address in your From: than in your s-o-b:;
>> same for the other patches]
a wrong setting of my email client, correct it next time, thanks.
>>> 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..."
>>
>>> + *
>>> + * 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."
>>
> It's good to understand, further more dev->name may has a bit mem leak.
> anyway, I'll correct all comments at V5. Thanks very much.
>>> + *
>>> + * Returns: 0 on suceess, -error on failure
>>> + */
>>> int register_virtio_device(struct virtio_device *dev)
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 2/4] virtio_pci: don't kfree device on register failure
2017-12-20 4:26 [PATCH v4 0/4] use put_device to cleanup resource weiping zhang
2017-12-20 4:26 ` [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add weiping zhang
@ 2017-12-20 4:26 ` weiping zhang
2017-12-20 15:55 ` Cornelia Huck
2017-12-20 4:27 ` [PATCH v4 3/4] virtio_vop: " weiping zhang
2017-12-20 4:27 ` [PATCH v4 4/4] virtio_remoteproc: " weiping zhang
3 siblings, 1 reply; 12+ messages in thread
From: weiping zhang @ 2017-12-20 4:26 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
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@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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v4 2/4] virtio_pci: don't kfree device on register failure
2017-12-20 4:26 ` [PATCH v4 2/4] virtio_pci: don't kfree device on register failure weiping zhang
@ 2017-12-20 15:55 ` Cornelia Huck
0 siblings, 0 replies; 12+ messages in thread
From: Cornelia Huck @ 2017-12-20 15:55 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
On Wed, 20 Dec 2017 12:26:43 +0800
weiping zhang <zwp10758@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@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@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 3/4] virtio_vop: don't kfree device on register failure
2017-12-20 4:26 [PATCH v4 0/4] use put_device to cleanup resource weiping zhang
2017-12-20 4:26 ` [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add weiping zhang
2017-12-20 4:26 ` [PATCH v4 2/4] virtio_pci: don't kfree device on register failure weiping zhang
@ 2017-12-20 4:27 ` weiping zhang
2017-12-20 15:57 ` Cornelia Huck
2017-12-20 4:27 ` [PATCH v4 4/4] virtio_remoteproc: " weiping zhang
3 siblings, 1 reply; 12+ messages in thread
From: weiping zhang @ 2017-12-20 4:27 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
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@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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v4 3/4] virtio_vop: don't kfree device on register failure
2017-12-20 4:27 ` [PATCH v4 3/4] virtio_vop: " weiping zhang
@ 2017-12-20 15:57 ` Cornelia Huck
0 siblings, 0 replies; 12+ messages in thread
From: Cornelia Huck @ 2017-12-20 15:57 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
On Wed, 20 Dec 2017 12:27:04 +0800
weiping zhang <zwp10758@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@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@redhat.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v4 4/4] virtio_remoteproc: don't kfree device on register failure
2017-12-20 4:26 [PATCH v4 0/4] use put_device to cleanup resource weiping zhang
` (2 preceding siblings ...)
2017-12-20 4:27 ` [PATCH v4 3/4] virtio_vop: " weiping zhang
@ 2017-12-20 4:27 ` weiping zhang
2017-12-20 16:12 ` Cornelia Huck
3 siblings, 1 reply; 12+ messages in thread
From: weiping zhang @ 2017-12-20 4:27 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
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@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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v4 4/4] virtio_remoteproc: don't kfree device on register failure
2017-12-20 4:27 ` [PATCH v4 4/4] virtio_remoteproc: " weiping zhang
@ 2017-12-20 16:12 ` Cornelia Huck
2017-12-21 3:08 ` weiping zhang
0 siblings, 1 reply; 12+ messages in thread
From: Cornelia Huck @ 2017-12-20 16:12 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
On Wed, 20 Dec 2017 12:27:33 +0800
weiping zhang <zwp10758@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@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.)
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v4 4/4] virtio_remoteproc: don't kfree device on register failure
2017-12-20 16:12 ` Cornelia Huck
@ 2017-12-21 3:08 ` weiping zhang
0 siblings, 0 replies; 12+ messages in thread
From: weiping zhang @ 2017-12-21 3:08 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, Michael S . Tsirkin
2017-12-21 0:12 GMT+08:00 Cornelia Huck <cohuck@redhat.com>:
> On Wed, 20 Dec 2017 12:27:33 +0800
> weiping zhang <zwp10758@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/
rproc_add_virtio_dev is not like other virtio driver, it always call
register_virtio_device,
so commit message would be:
virtio_remoteproc: correct put_device virtio_device.dev
rproc_virtio_dev_release will be called iff virtio_device.dev's
reference count drops to 0. Here we just put vdev.dev, and then
rproc->dev's cleanup will be done in rproc_virtio_dev_release.
>> 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@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.
OK, put it directly at v5.
> (It also may be good to cc: the maintainers for this driver.)
OK, thanks
^ permalink raw reply [flat|nested] 12+ messages in thread