Re: [RFC PATCH 0/2] use larger max_request_size for virtio_blk
2018-04-05 22:29 GMT+08:00 Jens Axboe <ax...@kernel.dk>: > On 4/5/18 4:09 AM, Weiping Zhang wrote: >> Hi, >> >> For virtio block device, actually there is no a hard limit for max request >> size, and virtio_blk driver set -1 to blk_queue_max_hw_sectors(q, -1U);. >> But it doesn't work, because there is a default upper limitation >> BLK_DEF_MAX_SECTORS (1280 sectors). So this series want to add a new helper >> blk_queue_max_hw_sectors_no_limit to set a proper max reqeust size. >> >> Weiping Zhang (2): >> blk-setting: add new helper blk_queue_max_hw_sectors_no_limit >> virtio_blk: add new module parameter to set max request size >> >> block/blk-settings.c | 20 >> drivers/block/virtio_blk.c | 32 ++-- >> include/linux/blkdev.h | 2 ++ >> 3 files changed, 52 insertions(+), 2 deletions(-) > > The driver should just use blk_queue_max_hw_sectors() to set the limit, > and then the soft limit can be modified by a udev rule. Technically the > driver doesn't own the software limit, it's imposed to ensure that we > don't introduce too much latency per request. > > Your situation is no different from many other setups, where the > hw limit is much higher than the default 1280k. > Hi Martin, Jens, It seems more reasonable to change software limitation by udev rule, thanks you. > > ___ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 0/2] use larger max_request_size for virtio_blk
Hi, For virtio block device, actually there is no a hard limit for max request size, and virtio_blk driver set -1 to blk_queue_max_hw_sectors(q, -1U);. But it doesn't work, because there is a default upper limitation BLK_DEF_MAX_SECTORS (1280 sectors). So this series want to add a new helper blk_queue_max_hw_sectors_no_limit to set a proper max reqeust size. Weiping Zhang (2): blk-setting: add new helper blk_queue_max_hw_sectors_no_limit virtio_blk: add new module parameter to set max request size block/blk-settings.c | 20 drivers/block/virtio_blk.c | 32 ++-- include/linux/blkdev.h | 2 ++ 3 files changed, 52 insertions(+), 2 deletions(-) -- 2.9.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 1/2] blk-setting: add new helper blk_queue_max_hw_sectors_no_limit
There is a default upper limitation BLK_DEF_MAX_SECTORS, but for
some virtual block device driver there is no such limitation. So
add a new help to set max request size.
Signed-off-by: Weiping Zhang <zhangweip...@didichuxing.com>
---
block/blk-settings.c | 20
include/linux/blkdev.h | 2 ++
2 files changed, 22 insertions(+)
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 48ebe6b..685c30c 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -253,6 +253,26 @@ void blk_queue_max_hw_sectors(struct request_queue *q,
unsigned int max_hw_secto
}
EXPORT_SYMBOL(blk_queue_max_hw_sectors);
+/* same as blk_queue_max_hw_sectors but without default upper limitation */
+void blk_queue_max_hw_sectors_no_limit(struct request_queue *q,
+ unsigned int max_hw_sectors)
+{
+ struct queue_limits *limits = >limits;
+ unsigned int max_sectors;
+
+ if ((max_hw_sectors << 9) < PAGE_SIZE) {
+ max_hw_sectors = 1 << (PAGE_SHIFT - 9);
+ printk(KERN_INFO "%s: set to minimum %d\n",
+ __func__, max_hw_sectors);
+ }
+
+ limits->max_hw_sectors = max_hw_sectors;
+ max_sectors = min_not_zero(max_hw_sectors, limits->max_dev_sectors);
+ limits->max_sectors = max_sectors;
+ q->backing_dev_info->io_pages = max_sectors >> (PAGE_SHIFT - 9);
+}
+EXPORT_SYMBOL(blk_queue_max_hw_sectors_no_limit);
+
/**
* blk_queue_chunk_sectors - set size of the chunk for this queue
* @q: the request queue for the device
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index ed63f3b..2250709 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1243,6 +1243,8 @@ extern void blk_cleanup_queue(struct request_queue *);
extern void blk_queue_make_request(struct request_queue *, make_request_fn *);
extern void blk_queue_bounce_limit(struct request_queue *, u64);
extern void blk_queue_max_hw_sectors(struct request_queue *, unsigned int);
+extern void blk_queue_max_hw_sectors_no_limit(struct request_queue *,
+ unsigned int);
extern void blk_queue_chunk_sectors(struct request_queue *, unsigned int);
extern void blk_queue_max_segments(struct request_queue *, unsigned short);
extern void blk_queue_max_discard_segments(struct request_queue *,
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH 2/2] virtio_blk: add new module parameter to set max request size
Actually there is no upper limitation, so add new module parameter
to provide a way to set a proper max request size for virtio block.
Using a larger request size can improve sequence performance in theory,
and reduce the interaction between guest and hypervisor.
Signed-off-by: Weiping Zhang <zhangweip...@didichuxing.com>
---
drivers/block/virtio_blk.c | 32 ++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4a07593c..5ac6d59 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -64,6 +64,34 @@ struct virtblk_req {
struct scatterlist sg[];
};
+
+static int max_request_size_set(const char *val, const struct kernel_param
*kp);
+
+static const struct kernel_param_ops max_request_size_ops = {
+ .set = max_request_size_set,
+ .get = param_get_uint,
+};
+
+static unsigned long max_request_size = 4096; /* in unit of KiB */
+module_param_cb(max_request_size, _request_size_ops, _request_size,
+ 0444);
+MODULE_PARM_DESC(max_request_size, "set max request size, in unit of KiB");
+
+static int max_request_size_set(const char *val, const struct kernel_param *kp)
+{
+ int ret;
+ unsigned int size_kb, page_kb = 1 << (PAGE_SHIFT - 10);
+
+ ret = kstrtouint(val, 10, _kb);
+ if (ret != 0)
+ return -EINVAL;
+
+ if (size_kb < page_kb)
+ return -EINVAL;
+
+ return param_set_uint(val, kp);
+}
+
static inline blk_status_t virtblk_result(struct virtblk_req *vbr)
{
switch (vbr->status) {
@@ -730,8 +758,8 @@ static int virtblk_probe(struct virtio_device *vdev)
/* We can handle whatever the host told us to handle. */
blk_queue_max_segments(q, vblk->sg_elems-2);
- /* No real sector limit. */
- blk_queue_max_hw_sectors(q, -1U);
+ /* No real sector limit, use 512b (max_request_size << 10) >> 9 */
+ blk_queue_max_hw_sectors_no_limit(q, max_request_size << 1);
/* Host can optionally specify maximum segment size and number of
* segments. */
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[RFC PATCH] virtio: add space for device features show
make features string more readable, add space every 8bit.
example:
00101010 0111 1100
Signed-off-by: weiping zhang <zhangweip...@didichuxing.com>
---
drivers/virtio/virtio.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 59e36ef..d7d2db1 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -50,9 +50,12 @@ static ssize_t features_show(struct device *_d,
/* We actually represent this as a bitstring, as it could be
* arbitrary length in future. */
- for (i = 0; i < sizeof(dev->features)*8; i++)
- len += sprintf(buf+len, "%c",
+ for (i = 0; i < sizeof(dev->features) * 8; i++) {
+ len += sprintf(buf + len, "%c",
__virtio_test_bit(dev, i) ? '1' : '0');
+ if (i % 8 == 7)
+ len += sprintf(buf + len, " ");
+ }
len += sprintf(buf+len, "\n");
return len;
}
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v5 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 <zhangweip...@didichuxing.com>
Reviewed-by: Cornelia Huck <coh...@redhat.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(>type);
@@ -497,6 +499,7 @@ static int _vop_add_device(struct mic_device_desc __iomem
*d,
vdev->c2h_vdev_db = ioread8(>dc->c2h_vdev_db);
ret = register_virtio_device(>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.dev);
+ else
+ kfree(vdev);
return ret;
}
@@ -568,7 +574,7 @@ static int _vop_remove_device(struct mic_device_desc
__iomem *d,
iowrite8(-1, >h2c_vdev_db);
if (status & VIRTIO_CONFIG_S_DRIVER_OK)
wait_for_completion(>reset_done);
- kfree(vdev);
+ put_device(>vdev.dev);
iowrite8(1, >guest_ack);
dev_dbg(>dev, "%s %d guest_ack %d\n",
__func__, __LINE__, ioread8(>guest_ack));
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v5 4/4] 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.
Signed-off-by: weiping zhang <zhangweip...@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(>dev);
+ put_device(>dev);
dev_err(dev, "failed to register vdev: %d\n", ret);
goto out;
}
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v5 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 <zhangweip...@didichuxing.com>
Reviewed-by: Cornelia Huck <coh...@redhat.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(_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(_dev->vdev.dev);
+ else
+ kfree(vp_dev);
return rc;
}
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v5 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 succeeds,
so the caller can always use put_device when register_virtio_device faild.
Signed-off-by: weiping zhang <zhangweip...@didichuxing.com>
Suggested-by: Cornelia Huck <coh...@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..59e36ef 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 to be registered
+ *
+ * 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 = _bus;
+ device_initialize(>dev);
/* Assign a unique device index and hence name. */
err = ida_simple_get(_index_ida, 0, 0, GFP_KERNEL);
@@ -330,9 +340,11 @@ int register_virtio_device(struct virtio_device *dev)
INIT_LIST_HEAD(>vqs);
- /* device_register() causes the bus infrastructure to look for a
-* matching driver. */
- err = device_register(>dev);
+ /*
+* device_add() causes the bus infrastructure to look for a matching
+* driver.
+*/
+ err = device_add(>dev);
if (err)
ida_simple_remove(_index_ida, dev->index);
out:
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v5 0/4] use put_device to cleanup resource
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. v4->v5: * virtio: correct some comments * virtio_remoteproc: use put_device directly, not use temp reg_dev 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: correct put_device virtio_device.dev drivers/misc/mic/vop/vop_main.c| 20 +--- drivers/remoteproc/remoteproc_virtio.c | 2 +- drivers/virtio/virtio.c| 18 +++--- drivers/virtio/virtio_pci_common.c | 8 ++-- 4 files changed, 35 insertions(+), 13 deletions(-) -- 2.9.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 4/4] virtio_remoteproc: don't kfree device on register failure
2017-12-21 0:12 GMT+08:00 Cornelia Huck <coh...@redhat.com>:
> On Wed, 20 Dec 2017 12:27:33 +0800
> weiping zhang <zwp10...@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 <zhangweip...@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 = >dev;
>> - struct virtio_device *vdev = >vdev;
>> + struct virtio_device *vdev = >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(>refcount);
>>
>> ret = register_virtio_device(vdev);
>> + reg_dev = vdev;
>> if (ret) {
>> - put_device(>dev);
>> dev_err(dev, "failed to register vdev: %d\n", ret);
>> goto out;
>> }
>>
>> dev_info(dev, "registered %s (type %d)\n", dev_name(>dev), id);
>>
>> + return 0;
>> +
>> out:
>> + if (reg_dev)
>> + put_device(>dev);
>> + else {
>> + kref_put(>refcount, rproc_vdev_release);
>> + put_device(>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(>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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add
2017-12-21 10:37 GMT+08:00 weiping zhang <zwp10...@gmail.com>: > 2017-12-20 23:53 GMT+08:00 Cornelia Huck <coh...@redhat.com>: >> On Wed, 20 Dec 2017 12:26:25 +0800 >> weiping zhang <zwp10...@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) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 1/4] virtio: split device_register into device_initialize and device_add
2017-12-20 23:53 GMT+08:00 Cornelia Huck <coh...@redhat.com>: > On Wed, 20 Dec 2017 12:26:25 +0800 > weiping zhang <zwp10...@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) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[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 <zhangweip...@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 = >dev;
- struct virtio_device *vdev = >vdev;
+ struct virtio_device *vdev = >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(>refcount);
ret = register_virtio_device(vdev);
+ reg_dev = vdev;
if (ret) {
- put_device(>dev);
dev_err(dev, "failed to register vdev: %d\n", ret);
goto out;
}
dev_info(dev, "registered %s (type %d)\n", dev_name(>dev), id);
+ return 0;
+
out:
+ if (reg_dev)
+ put_device(>dev);
+ else {
+ kref_put(>refcount, rproc_vdev_release);
+ put_device(>dev);
+ }
+
return ret;
}
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[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 <zhangweip...@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(>type);
@@ -497,6 +499,7 @@ static int _vop_add_device(struct mic_device_desc __iomem
*d,
vdev->c2h_vdev_db = ioread8(>dc->c2h_vdev_db);
ret = register_virtio_device(>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.dev);
+ else
+ kfree(vdev);
return ret;
}
@@ -568,7 +574,7 @@ static int _vop_remove_device(struct mic_device_desc
__iomem *d,
iowrite8(-1, >h2c_vdev_db);
if (status & VIRTIO_CONFIG_S_DRIVER_OK)
wait_for_completion(>reset_done);
- kfree(vdev);
+ put_device(>vdev.dev);
iowrite8(1, >guest_ack);
dev_dbg(>dev, "%s %d guest_ack %d\n",
__func__, __LINE__, ioread8(>guest_ack));
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[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 <zhangweip...@didichuxing.com>
Suggested-by: Cornelia Huck <coh...@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 = _bus;
+ device_initialize(>dev);
/* Assign a unique device index and hence name. */
err = ida_simple_get(_index_ida, 0, 0, GFP_KERNEL);
@@ -330,9 +340,11 @@ int register_virtio_device(struct virtio_device *dev)
INIT_LIST_HEAD(>vqs);
- /* device_register() causes the bus infrastructure to look for a
-* matching driver. */
- err = device_register(>dev);
+ /*
+* device_add() causes the bus infrastructure to look for a matching
+* driver.
+*/
+ err = device_add(>dev);
if (err)
ida_simple_remove(_index_ida, dev->index);
out:
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[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 <zhangweip...@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(_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(_dev->vdev.dev);
+ else
+ kfree(vp_dev);
return rc;
}
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v4 0/4] use put_device to cleanup resource
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 5/5] virtio: add comments for virtio_register_device
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.
*/
virtio_register_device may fail before/after call device_register, the
caller should do a proper cleanup. Caller cann't use kfree directly,
if virtio_register_device has already called device_register. Caller
cann't use put_device directly, if virtio_register_device has not yet
call device_register, because kobject_put may give a warning cause
dev->kobj has not been initialized.
Signed-off-by: weiping zhang <zhangweip...@didichuxing.com>
---
drivers/virtio/virtio.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index c5b057bd..4f0718b 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -309,6 +309,19 @@ void unregister_virtio_driver(struct virtio_driver *driver)
}
EXPORT_SYMBOL_GPL(unregister_virtio_driver);
+/**
+ * register_virtio_device - register virtio device
+ *
+ * @dev: virtio device interested
+ *
+ * This funciton may fail after call device_register, as mentioned at
+ * drivers/base/core.c we must use put_device(), _never_ directly free @dev.
+ * The caller should take care of the status of @dev and determine to use
+ * put_device or kfree. If @dev in VIRTIO_CONFIG_S_ACKNOWLEDGE status that
+ * means caller should use put_device otherwise use kfree directly.
+ *
+ * Returns: 0 on success, error on failure.
+ */
int register_virtio_device(struct virtio_device *dev)
{
int err;
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 4/5] 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 <zhangweip...@didichuxing.com>
---
drivers/remoteproc/remoteproc_virtio.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/remoteproc/remoteproc_virtio.c
b/drivers/remoteproc/remoteproc_virtio.c
index 2946348..ad5d6d1 100644
--- a/drivers/remoteproc/remoteproc_virtio.c
+++ b/drivers/remoteproc/remoteproc_virtio.c
@@ -327,14 +327,22 @@ int rproc_add_virtio_dev(struct rproc_vdev *rvdev, int id)
ret = register_virtio_device(vdev);
if (ret) {
- put_device(>dev);
dev_err(dev, "failed to register vdev: %d\n", ret);
goto out;
}
dev_info(dev, "registered %s (type %d)\n", dev_name(>dev), id);
+ return 0;
+
out:
+ if (VIRTIO_CONFIG_S_ACKNOWLEDGE & virtio_get_status(vdev))
+ put_device(>dev);
+ else {
+ kref_put(>refcount, rproc_vdev_release);
+ put_device(>dev);
+ }
+
return ret;
}
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 3/5] 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 <zhangweip...@didichuxing.com>
---
drivers/misc/mic/vop/vop_main.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index a341938..53eaa9d 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);
}
/*
@@ -512,7 +514,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 (VIRTIO_CONFIG_S_ACKNOWLEDGE & virtio_get_status(>vdev))
+ put_device(>vdev.dev);
+ else
+ kfree(vdev);
return ret;
}
@@ -568,7 +573,7 @@ static int _vop_remove_device(struct mic_device_desc
__iomem *d,
iowrite8(-1, >h2c_vdev_db);
if (status & VIRTIO_CONFIG_S_DRIVER_OK)
wait_for_completion(>reset_done);
- kfree(vdev);
+ put_device(>vdev.dev);
iowrite8(1, >guest_ack);
dev_dbg(>dev, "%s %d guest_ack %d\n",
__func__, __LINE__, ioread8(>guest_ack));
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 1/5] virtio: add helper virtio_get_status
add helper function to simplify dev->config->get_status().
Signed-off-by: weiping zhang <zhangweip...@didichuxing.com>
---
drivers/virtio/virtio.c | 6 ++
include/linux/virtio_config.h | 2 ++
2 files changed, 8 insertions(+)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index bf7ff39..c5b057bd 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -165,6 +165,12 @@ void virtio_add_status(struct virtio_device *dev, unsigned
int status)
}
EXPORT_SYMBOL_GPL(virtio_add_status);
+unsigned int virtio_get_status(struct virtio_device *dev)
+{
+ return dev->config->get_status(dev);
+}
+EXPORT_SYMBOL_GPL(virtio_get_status);
+
int virtio_finalize_features(struct virtio_device *dev)
{
int ret = dev->config->finalize_features(dev);
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index 5559a2d..30972c4 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -429,6 +429,8 @@ static inline void virtio_cwrite64(struct virtio_device
*vdev,
vdev->config->set(vdev, offset, , sizeof(val));
}
+unsigned int virtio_get_status(struct virtio_device *dev);
+
/* Conditional config space accessors. */
#define virtio_cread_feature(vdev, fbit, structname, member, ptr) \
({ \
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 0/5] proper cleanup if fail to register_virtio_device
Hi, Patch1 add a helper to get virtio_device's status which will be used later. Patch2~4: check virtio_device's status is RTIO_CONFIG_S_ACKNOWLEDGE or not, if so use put_device otherwise use kfree. Patch5: add comments for virtio_register_device help caller do a proper cleanup if got failure. weiping zhang (5): virtio: add helper virtio_get_status 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 virtio: add comments for virtio_register_device drivers/misc/mic/vop/vop_main.c| 17 +++-- drivers/remoteproc/remoteproc_virtio.c | 10 +- drivers/virtio/virtio.c| 19 +++ drivers/virtio/virtio_pci_common.c | 5 - include/linux/virtio_config.h | 2 ++ 5 files changed, 45 insertions(+), 8 deletions(-) -- 2.9.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv2] virtio_mmio: fix devm cleanup
2017-12-15 2:48 GMT+08:00 Michael S. Tsirkin <m...@redhat.com>:
> On Wed, Dec 13, 2017 at 02:34:14PM +, Mark Rutland wrote:
>> On Tue, Dec 12, 2017 at 06:02:23PM +0100, Cornelia Huck wrote:
>> > On Tue, 12 Dec 2017 13:45:50 +
>> > Mark Rutland <mark.rutl...@arm.com> wrote:
>> >
>> > > Recent rework of the virtio_mmio probe/remove paths balanced a
>> > > devm_ioremap() with an iounmap() rather than its devm variant. This ends
>> > > up corrupting the devm datastructures, and results in the following
>> > > boot-time splat on arm64 under QEMU 2.9.0:
>> > >
>> > > [3.450397] [ cut here ]
>> > > [3.453822] Trying to vfree() nonexistent vm area (c05b4844)
>> > > [3.460534] WARNING: CPU: 1 PID: 1 at mm/vmalloc.c:1525
>> > > __vunmap+0x1b8/0x220
>> > > [3.475898] Kernel panic - not syncing: panic_on_warn set ...
>> > > [3.475898]
>> > > [3.493933] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.15.0-rc3 #1
>> > > [3.513109] Hardware name: linux,dummy-virt (DT)
>> > > [3.525382] Call trace:
>> > > [3.531683] dump_backtrace+0x0/0x368
>> > > [3.543921] show_stack+0x20/0x30
>> > > [3.547767] dump_stack+0x108/0x164
>> > > [3.559584] panic+0x25c/0x51c
>> > > [3.569184] __warn+0x29c/0x31c
>> > > [3.576023] report_bug+0x1d4/0x290
>> > > [3.586069] bug_handler.part.2+0x40/0x100
>> > > [3.597820] bug_handler+0x4c/0x88
>> > > [3.608400] brk_handler+0x11c/0x218
>> > > [3.613430] do_debug_exception+0xe8/0x318
>> > > [3.627370] el1_dbg+0x18/0x78
>> > > [3.634037] __vunmap+0x1b8/0x220
>> > > [3.648747] vunmap+0x6c/0xc0
>> > > [3.653864] __iounmap+0x44/0x58
>> > > [3.659771] devm_ioremap_release+0x34/0x68
>> > > [3.672983] release_nodes+0x404/0x880
>> > > [3.683543] devres_release_all+0x6c/0xe8
>> > > [3.695692] driver_probe_device+0x250/0x828
>> > > [3.706187] __driver_attach+0x190/0x210
>> > > [3.717645] bus_for_each_dev+0x14c/0x1f0
>> > > [3.728633] driver_attach+0x48/0x78
>> > > [3.740249] bus_add_driver+0x26c/0x5b8
>> > > [3.752248] driver_register+0x16c/0x398
>> > > [3.757211] __platform_driver_register+0xd8/0x128
>> > > [3.770860] virtio_mmio_init+0x1c/0x24
>> > > [3.782671] do_one_initcall+0xe0/0x398
>> > > [3.791890] kernel_init_freeable+0x594/0x660
>> > > [3.798514] kernel_init+0x18/0x190
>> > > [3.810220] ret_from_fork+0x10/0x18
>> > >
>> > > To fix this, we can simply rip out the explicit cleanup that the devm
>> > > infrastructure will do for us when our probe function returns an error
>> > > code, or when our remove function returns.
>> > >
>> > > We only need to ensure that we call put_device() if a call to
>> > > register_virtio_device() fails in the probe path.
>> > >
>> > > Signed-off-by: Mark Rutland <mark.rutl...@arm.com>
>> > > Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for
>> > > virtio_mmio_probe")
>> > > Fixes: 25f32223bce5c580 ("virtio_mmio: add cleanup for
>> > > virtio_mmio_remove")
>> > > Cc: Cornelia Huck <coh...@redhat.com>
>> > > Cc: Michael S. Tsirkin <m...@redhat.com>
>> > > Cc: weiping zhang <zhangweip...@didichuxing.com>
>> > > Cc: virtualization@lists.linux-foundation.org
>> > > ---
>> > > drivers/virtio/virtio_mmio.c | 43
>> > > +--
>> > > 1 file changed, 9 insertions(+), 34 deletions(-)
>> >
>> > In the hope that I have grokked the devm_* interface by now,
>> >
>> > Reviewed-by: Cornelia Huck <coh...@redhat.com>
>>
>> Thanks!
>>
>> Michael, could you please queue this as a fix for v4.15?
>>
>> This regressed arm64 VMs booting between v4.15-rc1 and v4-15-rc2,
>> impacting our automated regression testing, and I'd very much like to
>> get back to testing pure mainline rather than mainline + local fixes.
>>
>> Thanks,
>> Mark.
>
> Yep, plan to.
> Thanks!
Sorry to bother again,
As we know if we call device_regist
Re: [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
2017-12-15 3:13 GMT+08:00 Michael S. Tsirkin <m...@redhat.com>:
> On Tue, Dec 12, 2017 at 09:24:02PM +0800, weiping zhang 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.
>
> seeing as 5739411acbaa63a6c22c91e340fdcdbcc7d82a51 adding these
> annotations went to stable, should this go there too?
>
just let people know the detail reason of using put_device.
>> Signed-off-by: weiping zhang <zhangweip...@didichuxing.com>
>> Reviewed-by: Cornelia Huck <coh...@redhat.com>
>
> OK but this relies on users knowing that register_virtio_device
> calls device_register. I think we want to add a comment
> to register_virtio_device.
>
> Also the cleanup is uglified.
>
> I really think the right thing would be to change device_register making
> it safe to kfree. People have the right to expect register on failure to
> have no effect.
>
> That just might be too hard to implement though.
>
> For now, my suggestion - add a variable.
>
>> ---
>> 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(_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(_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
>
> I'd prefer something like the below.
>
> --->
>
> virtio_pci: don't kfree device on register failure
>
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
>
> ---
>
> diff --git a/drivers/virtio/virtio_pci_common.c
> b/drivers/virtio/virtio_pci_common.c
> index 1c4797e..995ab03 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,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> pci_set_master(pci_dev);
>
> rc = register_virtio_device(_dev->vdev);
> + /* NOTE: device is considered registered even if register failed. */
> + reg_dev = vp_dev;
> if (rc)
> goto err_register;
>
> @@ -564,7 +566,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(dev);
> + else
> + kfree(vp_dev);
> return rc;
> }
looks more cleaner and same coding style.
Need I send V3 or apply your patch directly ?
> ___
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCHv2] virtio_mmio: fix devm cleanup
2017-12-12 22:45 GMT+08:00 Mark Rutland <mark.rutl...@arm.com>:
> On Tue, Dec 12, 2017 at 10:26:24PM +0800, weiping zhang wrote:
>> 2017-12-12 21:45 GMT+08:00 Mark Rutland <mark.rutl...@arm.com>:
>> Hi Mark,
>
> Hi,
>
>> thanks your patch, I dig into these three devm_xxx funciton,
>> all of them represented by a struct devres as following,
>>
>> struct devres_node {
>> struct list_headentry;
>> dr_release_trelease;
>> #ifdef CONFIG_DEBUG_DEVRES
>> const char *name;
>> size_t size;
>> #endif
>>
>> };
>>
>> struct devres {
>> struct devres_node node;
>> /* -- 3 pointers */
>> unsigned long long data[]; /* guarantee ull alignment */
>> };
>
>> 2) devm_kzalloc -> devm_kmalloc
>>
>> dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev));
>> "devm_kmalloc_release" is noop, do nothing.
>
> Please note that the release function is there to perform cleanup prior
> to the devm infrastructure releasing the memory.
>
> The devm_kmalloc_release function is a no-op since nothing has to be
> done prior to memory being freed, but the memory itself is still freed.
>
> In alloc_dr(), the struct devres is allocated together with the memory,
> since alloc_dr() does:
>
> size_t tot_size = sizeof(struct devres) + size;
> struct devres *dr;
>
> dr = kmalloc_node_track_caller(tot_size, gfp, nid);
>
> return dr->data;
>
> ... where dr->data points at the memory after the struct devres.
>
> Later, in release_nodes() we do:
>
> list_for_each_entry_safe_reverse(dr, tmp, , node.entry) {
> devres_log(dev, >node, "REL");
> dr->node.release(dev, dr->data);
> kfree(dr);
> }
>
> ... which will invoke the no-op devm_kmalloc_release, then free the
> devres allocation, including the dr->data memory the user requested.
>
>> so for case 2) above, we need a devm_kfree() before call
>> register_virtio_device
>
> As above, I do not believe that is the case.
>
Oh I see, thanks your detail explanation. Thanks a lot.
> Thanks,
> Mark.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 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 <zhangweip...@didichuxing.com>
Reviewed-by: Cornelia Huck <coh...@redhat.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(>dev);
+ put_device(>dev);
dev_err(dev, "failed to register vdev: %d\n", ret);
goto out;
}
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 1/3] virtio_pci: use put_device instead of kfree
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 <zhangweip...@didichuxing.com>
Reviewed-by: Cornelia Huck <coh...@redhat.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(_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(_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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 2/3] virtio: use put_device instead of kfree
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_vdev until vp_vdev.dev.release be called.
Signed-off-by: weiping zhang <zhangweip...@didichuxing.com>
---
drivers/misc/mic/vop/vop_main.c | 18 ++
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index a341938..456e969 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.dev);
+ return ret;
}
writeq((u64)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;
@@ -568,7 +570,7 @@ static int _vop_remove_device(struct mic_device_desc
__iomem *d,
iowrite8(-1, >h2c_vdev_db);
if (status & VIRTIO_CONFIG_S_DRIVER_OK)
wait_for_completion(>reset_done);
- kfree(vdev);
+ put_device(>vdev.dev);
iowrite8(1, >guest_ack);
dev_dbg(>dev, "%s %d guest_ack %d\n",
__func__, __LINE__, ioread8(>guest_ack));
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 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. V1->V2: * virtio_pci: add comments for the reason use put_device * virtio vop: also use put_device in in _vop_remove_device() 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| 18 ++ drivers/remoteproc/remoteproc_virtio.c | 2 +- drivers/virtio/virtio_pci_common.c | 17 + 3 files changed, 20 insertions(+), 17 deletions(-) -- 2.9.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/3] virtio: use put_device instead of kfree
On Tue, Dec 12, 2017 at 11:17:42AM +0100, Cornelia Huck wrote:
> On Mon, 11 Dec 2017 23:55:26 +0800
> weiping zhang <zhangweip...@didichuxing.com> wrote:
>
> > don't free vp_vdev until vp_vdev.dev.release be called.
>
> Same comment as for the virtio_pci patch.
OK, I'll add it at V2.
>
> >
> > Signed-off-by: weiping zhang <zhangweip...@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.dev);
> > + return ret;
> > }
> > writeq((u64)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).
thanks for your comments, I'll fix it at v2.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] virtio_pci: use put_device instead of kfree
On Tue, Dec 12, 2017 at 11:00:25AM +0100, Cornelia Huck wrote:
> On Mon, 11 Dec 2017 23:55:16 +0800
> weiping zhang <zhangweip...@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?
OK, I'll add it at V2.
> >
> > Signed-off-by: weiping zhang <zhangweip...@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(_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(_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 <coh...@redhat.com>
Thanks a ton
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[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 <zhangweip...@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(_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(_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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[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 <zhangweip...@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(>dev);
+ put_device(>dev);
dev_err(dev, "failed to register vdev: %d\n", ret);
goto out;
}
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH 2/3] virtio: use put_device instead of kfree
don't free vp_vdev until vp_vdev.dev.release be called.
Signed-off-by: weiping zhang <zhangweip...@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.dev);
+ return ret;
}
writeq((u64)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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 1/2] virtio_mmio: add cleanup for virtio_mmio_probe
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 vm_dev until vm_dev.dev.release be called.
Signed-off-by: weiping zhang <zhangweip...@didichuxing.com>
---
drivers/virtio/virtio_mmio.c | 51 +++-
1 file changed, 41 insertions(+), 10 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 74dc717..ec40104 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -493,7 +493,16 @@ static const struct virtio_config_ops
virtio_mmio_config_ops = {
};
-static void virtio_mmio_release_dev_empty(struct device *_d) {}
+static void virtio_mmio_release_dev(struct device *_d)
+{
+ struct virtio_device *vdev =
+ container_of(_d, struct virtio_device, dev);
+ struct virtio_mmio_device *vm_dev =
+ container_of(vdev, struct virtio_mmio_device, vdev);
+ struct platform_device *pdev = vm_dev->pdev;
+
+ devm_kfree(>dev, vm_dev);
+}
/* Platform device */
@@ -513,25 +522,30 @@ static int virtio_mmio_probe(struct platform_device *pdev)
return -EBUSY;
vm_dev = devm_kzalloc(>dev, sizeof(*vm_dev), GFP_KERNEL);
- if (!vm_dev)
- return -ENOMEM;
+ if (!vm_dev) {
+ rc = -ENOMEM;
+ goto free_mem;
+ }
vm_dev->vdev.dev.parent = >dev;
- vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
+ vm_dev->vdev.dev.release = virtio_mmio_release_dev;
vm_dev->vdev.config = _mmio_config_ops;
vm_dev->pdev = pdev;
INIT_LIST_HEAD(_dev->virtqueues);
spin_lock_init(_dev->lock);
vm_dev->base = devm_ioremap(>dev, mem->start, resource_size(mem));
- if (vm_dev->base == NULL)
- return -EFAULT;
+ if (vm_dev->base == NULL) {
+ rc = -EFAULT;
+ goto free_vmdev;
+ }
/* Check magic value */
magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
dev_warn(>dev, "Wrong magic value 0x%08lx!\n", magic);
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
/* Check device version */
@@ -539,7 +553,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
if (vm_dev->version < 1 || vm_dev->version > 2) {
dev_err(>dev, "Version %ld not supported!\n",
vm_dev->version);
- return -ENXIO;
+ rc = -ENXIO;
+ goto unmap;
}
vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
@@ -548,7 +563,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
* virtio-mmio device with an ID 0 is a (dummy) placeholder
* with no function. End probing now with no error reported.
*/
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
@@ -573,7 +589,22 @@ static int virtio_mmio_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, vm_dev);
- return register_virtio_device(_dev->vdev);
+ rc = register_virtio_device(_dev->vdev);
+ if (rc) {
+ iounmap(vm_dev->base);
+ devm_release_mem_region(>dev, mem->start,
+ resource_size(mem));
+ put_device(_dev->vdev.dev);
+ }
+ return rc;
+unmap:
+ iounmap(vm_dev->base);
+free_mem:
+ devm_release_mem_region(>dev, mem->start,
+ resource_size(mem));
+free_vmdev:
+ devm_kfree(>dev, vm_dev);
+ return rc;
}
static int virtio_mmio_remove(struct platform_device *pdev)
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 0/2] Add cleanup for virtio_mmio driver
this patchset try to add cleanup for virtio_mmio driver, include virtio_mmio_probe and virtio_mmio_remove weiping zhang (2): virtio_mmio: add cleanup for virtio_mmio_probe virtio_mmio: add cleanup for virtio_mmio_remove drivers/virtio/virtio_mmio.c | 57 1 file changed, 47 insertions(+), 10 deletions(-) -- 2.9.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v3 2/2] virtio_mmio: add cleanup for virtio_mmio_remove
cleanup all resource allocated by virtio_mmio_probe.
Signed-off-by: weiping zhang <zhangweip...@didichuxing.com>
---
drivers/virtio/virtio_mmio.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index ec40104..a9192fe 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -610,7 +610,13 @@ static int virtio_mmio_probe(struct platform_device *pdev)
static int virtio_mmio_remove(struct platform_device *pdev)
{
struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
+ struct resource *mem;
+ iounmap(vm_dev->base);
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (mem)
+ devm_release_mem_region(>dev, mem->start,
+ resource_size(mem));
unregister_virtio_device(_dev->vdev);
return 0;
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe
2017-12-06 19:11 GMT+08:00 Cornelia Huck <coh...@redhat.com>: > On Tue, 5 Dec 2017 19:57:10 +0800 > weiping zhang <zhangweip...@didichuxing.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. >> */ >> >> Normal we do cleanup for @vm_dev by contianer_of(@dev), but in this case >> we need release @mem resource from @pdev and vm_dev->base. It make >> @pdev->vm_dev.dev.release() too complicated, so put_device just put the >> reference of register_virtio_device->device_register->device_initialize >> and release all resource in virtio_mmio_probe. > > Releasing the resources when unwinding on error can work, but I think > there still are some issues (more below). This is all very tangly > code :( > >> >> Signed-off-by: weiping zhang <zhangweip...@didichuxing.com> >> --- >> drivers/virtio/virtio_mmio.c | 36 >> 1 file changed, 28 insertions(+), 8 deletions(-) >> > >> @@ -573,7 +580,20 @@ static int virtio_mmio_probe(struct platform_device >> *pdev) >> >> platform_set_drvdata(pdev, vm_dev); >> >> - return register_virtio_device(_dev->vdev); >> + rc = register_virtio_device(_dev->vdev); >> + if (rc) >> + goto put_dev; >> + return 0; >> +put_dev: >> + put_device(_dev->vdev.dev); > > Here you give up the extra reference from device_initialize(), which > may or may not be the last reference (since you don't know if > device_add() had already exposed the struct device to other code that > might have acquired a reference). As the device has an empty release > function, touching the device structure after that is not a real > problem, but... > >> +unmap: >> + iounmap(vm_dev->base); >> +free_mem: >> + devm_release_mem_region(>dev, mem->start, >> + resource_size(mem)); >> +free_vmdev: >> + devm_kfree(>dev, vm_dev); > > ...unconditionally freeing the device here would be a problem if other > code had acquired a reference above. (Unlikely, but we should try to > get this right.) > that's true, so we don't free it until it's refer count decrease to 0 and ->release called. >> + return rc; >> } >> >> static int virtio_mmio_remove(struct platform_device *pdev) > > So, I think there are basically two ways of doing that: > - Move the cleanup into the currently empty release callback. Then, you > won't need to touch the remove function. The problem with that is > that you can't trigger a cleanup via put_device() if you did not call > register_virtio_device() yet. > - Move just devm_kfree() into the release function. Cleanup the > resources here, do the put_device() last thing if had you called > register_virtio_device() before and devm_kfree() if you didn't. > I prefer go second way. I'll send v3 later. > [Of course, I still might be missing some devm subtility, so other > comments are welcome.] -- Thanks very much weiping > ___ > Virtualization mailing list > Virtualization@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe
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.
*/
Normal we do cleanup for @vm_dev by contianer_of(@dev), but in this case
we need release @mem resource from @pdev and vm_dev->base. It make
@pdev->vm_dev.dev.release() too complicated, so put_device just put the
reference of register_virtio_device->device_register->device_initialize
and release all resource in virtio_mmio_probe.
Signed-off-by: weiping zhang <zhangweip...@didichuxing.com>
---
drivers/virtio/virtio_mmio.c | 36
1 file changed, 28 insertions(+), 8 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 74dc717..f984510 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -513,8 +513,10 @@ static int virtio_mmio_probe(struct platform_device *pdev)
return -EBUSY;
vm_dev = devm_kzalloc(>dev, sizeof(*vm_dev), GFP_KERNEL);
- if (!vm_dev)
- return -ENOMEM;
+ if (!vm_dev) {
+ rc = -ENOMEM;
+ goto free_mem;
+ }
vm_dev->vdev.dev.parent = >dev;
vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
@@ -524,14 +526,17 @@ static int virtio_mmio_probe(struct platform_device *pdev)
spin_lock_init(_dev->lock);
vm_dev->base = devm_ioremap(>dev, mem->start, resource_size(mem));
- if (vm_dev->base == NULL)
- return -EFAULT;
+ if (vm_dev->base == NULL) {
+ rc = -EFAULT;
+ goto free_vmdev;
+ }
/* Check magic value */
magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
dev_warn(>dev, "Wrong magic value 0x%08lx!\n", magic);
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
/* Check device version */
@@ -539,7 +544,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
if (vm_dev->version < 1 || vm_dev->version > 2) {
dev_err(>dev, "Version %ld not supported!\n",
vm_dev->version);
- return -ENXIO;
+ rc = -ENXIO;
+ goto unmap;
}
vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
@@ -548,7 +554,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
* virtio-mmio device with an ID 0 is a (dummy) placeholder
* with no function. End probing now with no error reported.
*/
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
@@ -573,7 +580,20 @@ static int virtio_mmio_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, vm_dev);
- return register_virtio_device(_dev->vdev);
+ rc = register_virtio_device(_dev->vdev);
+ if (rc)
+ goto put_dev;
+ return 0;
+put_dev:
+ put_device(_dev->vdev.dev);
+unmap:
+ iounmap(vm_dev->base);
+free_mem:
+ devm_release_mem_region(>dev, mem->start,
+ resource_size(mem));
+free_vmdev:
+ devm_kfree(>dev, vm_dev);
+ return rc;
}
static int virtio_mmio_remove(struct platform_device *pdev)
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 0/2] Add cleanup for virtio_mmio driver
this patchset try to add cleanup for virtio_mmio driver, include virtio_mmio_probe and virtio_mmio_remove weiping zhang (2): virtio_mmio: add cleanup for virtio_mmio_probe virtio_mmio: add cleanup for virtio_mmio_remove drivers/virtio/virtio_mmio.c | 43 +++ 1 file changed, 35 insertions(+), 8 deletions(-) -- 2.9.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 2/2] virtio_mmio: add cleanup for virtio_mmio_remove
cleanup all resource allocated by virtio_mmio_probe.
Signed-off-by: weiping zhang <zhangweip...@didichuxing.com>
---
drivers/virtio/virtio_mmio.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index f984510..5e2ca34 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -599,8 +599,15 @@ static int virtio_mmio_probe(struct platform_device *pdev)
static int virtio_mmio_remove(struct platform_device *pdev)
{
struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
+ struct resource *mem;
unregister_virtio_device(_dev->vdev);
+ iounmap(vm_dev->base);
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (mem)
+ devm_release_mem_region(>dev, mem->start,
+ resource_size(mem));
+ devm_kfree(>dev, vm_dev);
return 0;
}
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_mmio: add cleanup for virtio_mmio_probe
2017-12-04 18:24 GMT+08:00 Cornelia Huck <coh...@redhat.com>:
> On Sat, 2 Dec 2017 01:51:40 +0800
> weiping zhang <zwp10...@gmail.com> wrote:
>
>> cleanup all resource allocated by virtio_mmio_probe.
>>
>> Signed-off-by: weiping zhang <zhangweip...@didichuxing.com>
>> ---
>> drivers/virtio/virtio_mmio.c | 34 ++
>> 1 file changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
>> index 74dc717..3fd0e66 100644
>> --- a/drivers/virtio/virtio_mmio.c
>> +++ b/drivers/virtio/virtio_mmio.c
>> @@ -513,8 +513,10 @@ static int virtio_mmio_probe(struct platform_device
>> *pdev)
>> return -EBUSY;
>>
>> vm_dev = devm_kzalloc(>dev, sizeof(*vm_dev), GFP_KERNEL);
>> - if (!vm_dev)
>> - return -ENOMEM;
>> + if (!vm_dev) {
>> + rc = -ENOMEM;
>
> Touching this would be a good time to remove the extra space in front
> of the -ENOMEM :)
Thanks, I fix this at V2.
>
>> + goto free_mem;
>> + }
>>
>> vm_dev->vdev.dev.parent = >dev;
>> vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
>
> (...)
>
>> @@ -573,7 +580,18 @@ static int virtio_mmio_probe(struct platform_device
>> *pdev)
>>
>> platform_set_drvdata(pdev, vm_dev);
>>
>> - return register_virtio_device(_dev->vdev);
>> + rc = register_virtio_device(_dev->vdev);
>> + if (rc)
>> + goto unmap;
>> + return 0;
>> +unmap:
>> + iounmap(vm_dev->base);
>> +free_mem:
>> + devm_release_mem_region(>dev, mem->start,
>> + resource_size(mem));
>> +free_vmdev:
>> + devm_kfree(>dev, vm_dev);
>
> I think this is problematic as vm_dev embeds a struct device (via
> embedding a struct virtio_device). I think the right way to do this is
> - call this only if register_virtio_device() has not been called
> - put the devm_kfree() into the ->release callback for the
> virtio_device (IOW, replace virtio_mmio_release_dev_empty() with a
> function that calls this)
> - do a put_device() if register_virtio_device() failed
>
> I might be missing some interaction between the usual driver model
> handling and devm resources, though.
>
if fail in virtio_mmio_probe, we need free @mem of @pdev, vdev->base,
so I prefer clean all this resource in virtio_mmio_probe itself, keep
vdev.dev.release do nothing.
also we need release these resources at virtio_mmio_remove.
I'll send V2.
--
thanks a ton
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: release virtio index when fail to device_register
2017-12-04 17:38 GMT+08:00 Cornelia Huck <coh...@redhat.com>: > On Sat, 2 Dec 2017 00:55:39 +0800 > weiping zhang <zwp10...@gmail.com> wrote: > >> On Wed, Nov 29, 2017 at 10:50:44AM +0100, Cornelia Huck wrote: > >> > We hold an extra reference to the struct device, even after a failed >> > register, and it is the responsibility of the caller to give up that >> > reference once no longer needed. As callers toregister_virtio_device() >> > embed the struct virtio_device, it needs to be their responsibility. >> > Looking at the existing callers, >> > >> > - ccw does a put_device >> > - pci, mmio and remoteproc do nothing, causing a leak >> > - vop does a free on the embedding structure, which is a big no-no >> > >> > Thoughts? >> Sorry to relay late and thanks for your review. >> Do you mean the "extra reference to the struct device" caused by the >> following code? >> >> err = device_register(>dev); >> device_add(dev) >> get_device(dev) >> If I'm understand right, I think there is no extra reference if we fail >> virtio_register_device, because if device_register we don't get a >> reference. > > The device_initialize() already gives you a reference. If device_add() > fails, it has cleaned up any additional reference it might have > obtained, but the initial reference is still there and needs to be > released by the caller. Thanks your clarify, I also notice the comments at device_register, device_initialize, device_add, * 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. -- Thanks weiping ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio: release virtio index when fail to device_register
index can be reused by other virtio device. Signed-off-by: weiping zhang <zhangweip...@didichuxing.com> --- drivers/virtio/virtio.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 48230a5..bf7ff39 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev) /* device_register() causes the bus infrastructure to look for a * matching driver. */ err = device_register(>dev); + if (err) + ida_simple_remove(_index_ida, dev->index); out: if (err) virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); -- 2.9.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio_mmio: add cleanup for virtio_mmio_probe
cleanup all resource allocated by virtio_mmio_probe.
Signed-off-by: weiping zhang <zhangweip...@didichuxing.com>
---
drivers/virtio/virtio_mmio.c | 34 ++
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 74dc717..3fd0e66 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -513,8 +513,10 @@ static int virtio_mmio_probe(struct platform_device *pdev)
return -EBUSY;
vm_dev = devm_kzalloc(>dev, sizeof(*vm_dev), GFP_KERNEL);
- if (!vm_dev)
- return -ENOMEM;
+ if (!vm_dev) {
+ rc = -ENOMEM;
+ goto free_mem;
+ }
vm_dev->vdev.dev.parent = >dev;
vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
@@ -524,14 +526,17 @@ static int virtio_mmio_probe(struct platform_device *pdev)
spin_lock_init(_dev->lock);
vm_dev->base = devm_ioremap(>dev, mem->start, resource_size(mem));
- if (vm_dev->base == NULL)
- return -EFAULT;
+ if (vm_dev->base == NULL) {
+ rc = -EFAULT;
+ goto free_vmdev;
+ }
/* Check magic value */
magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
dev_warn(>dev, "Wrong magic value 0x%08lx!\n", magic);
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
/* Check device version */
@@ -539,7 +544,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
if (vm_dev->version < 1 || vm_dev->version > 2) {
dev_err(>dev, "Version %ld not supported!\n",
vm_dev->version);
- return -ENXIO;
+ rc = -ENXIO;
+ goto unmap;
}
vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
@@ -548,7 +554,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
* virtio-mmio device with an ID 0 is a (dummy) placeholder
* with no function. End probing now with no error reported.
*/
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
@@ -573,7 +580,18 @@ static int virtio_mmio_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, vm_dev);
- return register_virtio_device(_dev->vdev);
+ rc = register_virtio_device(_dev->vdev);
+ if (rc)
+ goto unmap;
+ return 0;
+unmap:
+ iounmap(vm_dev->base);
+free_mem:
+ devm_release_mem_region(>dev, mem->start,
+ resource_size(mem));
+free_vmdev:
+ devm_kfree(>dev, vm_dev);
+ return rc;
}
static int virtio_mmio_remove(struct platform_device *pdev)
--
2.9.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio: release virtio index when fail to device_register
2017-12-02 0:55 GMT+08:00 weiping zhang <zwp10...@gmail.com>: > On Wed, Nov 29, 2017 at 10:50:44AM +0100, Cornelia Huck wrote: >> On Wed, 29 Nov 2017 09:23:01 +0800 >> weiping zhang <zwp10...@gmail.com> wrote: >> >> > index can be reused by other virtio device. >> > >> > Signed-off-by: weiping zhang <zhangweip...@didichuxing.com> >> >> Reviewed-by: Cornelia Huck <coh...@redhat.com> >> >> > --- >> > drivers/virtio/virtio.c | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c >> > index 48230a5..bf7ff39 100644 >> > --- a/drivers/virtio/virtio.c >> > +++ b/drivers/virtio/virtio.c >> > @@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev) >> > /* device_register() causes the bus infrastructure to look for a >> > * matching driver. */ >> > err = device_register(>dev); >> > + if (err) >> > + ida_simple_remove(_index_ida, dev->index); >> > out: >> > if (err) >> > virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); >> >> I think your patch is correct (giving up the index if we failed to >> register), but this made me look at our error handling if a virtio >> device failed to register. >> >> We hold an extra reference to the struct device, even after a failed >> register, and it is the responsibility of the caller to give up that >> reference once no longer needed. As callers toregister_virtio_device() >> embed the struct virtio_device, it needs to be their responsibility. >> Looking at the existing callers, >> >> - ccw does a put_device >> - pci, mmio and remoteproc do nothing, causing a leak >> - vop does a free on the embedding structure, which is a big no-no >> >> Thoughts? > Sorry to relay late and thanks for your review. > Do you mean the "extra reference to the struct device" caused by the > following code? > > err = device_register(>dev); > device_add(dev) > get_device(dev) > If I'm understand right, I think there is no extra reference if we fail > virtio_register_device, because if device_register we don't get a > reference. I go through these callers, virtio_mmio_probe should do more cleanup. I'll sent a patch fix that. > -- > Thanks > weiping ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] virtio: release virtio index when fail to device_register
index can be reused by other virtio device. Signed-off-by: weiping zhang <zhangweip...@didichuxing.com> --- drivers/virtio/virtio.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 48230a5..bf7ff39 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev) /* device_register() causes the bus infrastructure to look for a * matching driver. */ err = device_register(>dev); + if (err) + ida_simple_remove(_index_ida, dev->index); out: if (err) virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); -- 2.9.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
