* Re: [virtio-dev] Re: [PATCH v19 3/7] xbitmap: add more operations
From: Wei Wang @ 2017-12-14 11:47 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, willy,
quan.xu, nilal, riel, cornelia.huck, mhocko, linux-kernel,
amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <5A31F445.6070504@intel.com>
On 12/14/2017 11:47 AM, Wei Wang wrote:
> On 12/13/2017 10:16 PM, Tetsuo Handa wrote:
>
>
>>
>>> if (set)
>>> ret = find_next_bit(&tmp,
>>> BITS_PER_LONG, ebit);
>>> else
>>> ret = find_next_zero_bit(&tmp,
>>> BITS_PER_LONG,
>>> ebit);
>>> if (ret < BITS_PER_LONG)
>>> return ret - 2 + ida_start;
>>> } else if (bitmap) {
>>> if (set)
>>> ret = find_next_bit(bitmap->bitmap,
>>> IDA_BITMAP_BITS, bit);
>>> else
>>> ret =
>>> find_next_zero_bit(bitmap->bitmap,
>>> IDA_BITMAP_BITS, bit);
>> "bit" may not be 0 for the first round and "bit" is always 0 afterwords.
>> But where is the guaranteed that "end" is a multiple of
>> IDA_BITMAP_BITS ?
>> Please explain why it is correct to use IDA_BITMAP_BITS unconditionally
>> for the last round.
>
> There missed something here, it will be:
>
> nbits = min(end - ida_start + 1, IDA_BITMAP_BITS - bit);
captured a bug here, should be:
nbits = min(end - ida_start + 1, (unsigned long)IDA_BITMAP_BITS);
> if (set)
> ret = find_next_bit(bitmap->bitmap, nbits, bit);
> else
> ret = find_next_zero_bit(bitmap->bitmap,
> nbits, bit);
> if (ret < nbits)
> return ret + ida_start;
>
>
Best,
Wei
^ permalink raw reply
* [PULL 1/1] virtio/s390: implement PM operations for virtio_ccw
From: Cornelia Huck @ 2017-12-14 9:59 UTC (permalink / raw)
To: mst; +Cc: linux-s390, kvm, Cornelia Huck, virtualization
In-Reply-To: <20171214095954.7615-1-cohuck@redhat.com>
From: Christian Borntraeger <borntraeger@de.ibm.com>
Suspend/Resume to/from disk currently fails. Let us wire
up the necessary callbacks. This is mostly just forwarding
the requests to the virtio drivers. The only thing that
has to be done in virtio_ccw itself is to re-set the
virtio revision.
Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Message-Id: <20171207141102.70190-2-borntraeger@de.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
drivers/s390/virtio/virtio_ccw.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
index b18fe2014cf2..330b3fa3430a 100644
--- a/drivers/s390/virtio/virtio_ccw.c
+++ b/drivers/s390/virtio/virtio_ccw.c
@@ -1300,6 +1300,9 @@ static int virtio_ccw_cio_notify(struct ccw_device *cdev, int event)
vcdev->device_lost = true;
rc = NOTIFY_DONE;
break;
+ case CIO_OPER:
+ rc = NOTIFY_OK;
+ break;
default:
rc = NOTIFY_DONE;
break;
@@ -1312,6 +1315,25 @@ static struct ccw_device_id virtio_ids[] = {
{},
};
+static int virtio_ccw_freeze(struct ccw_device *cdev)
+{
+ struct virtio_ccw_device *vcdev = dev_get_drvdata(&cdev->dev);
+
+ return virtio_device_freeze(&vcdev->vdev);
+}
+
+static int virtio_ccw_restore(struct ccw_device *cdev)
+{
+ struct virtio_ccw_device *vcdev = dev_get_drvdata(&cdev->dev);
+ int ret;
+
+ ret = virtio_ccw_set_transport_rev(vcdev);
+ if (ret)
+ return ret;
+
+ return virtio_device_restore(&vcdev->vdev);
+}
+
static struct ccw_driver virtio_ccw_driver = {
.driver = {
.owner = THIS_MODULE,
@@ -1324,6 +1346,9 @@ static struct ccw_driver virtio_ccw_driver = {
.set_online = virtio_ccw_online,
.notify = virtio_ccw_cio_notify,
.int_class = IRQIO_VIR,
+ .freeze = virtio_ccw_freeze,
+ .thaw = virtio_ccw_restore,
+ .restore = virtio_ccw_restore,
};
static int __init pure_hex(char **cp, unsigned int *val, int min_digit,
--
2.13.6
^ permalink raw reply related
* [PULL 0/1] s390/virtio update
From: Cornelia Huck @ 2017-12-14 9:59 UTC (permalink / raw)
To: mst; +Cc: linux-s390, kvm, Cornelia Huck, virtualization
The following changes since commit e073f74a5a39c6dc45f28a5006c21aa94490d9b8:
Merge branch 'this' into vhost (2017-12-07 18:39:24 +0200)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git tags/virtio-s390-20171214
for you to fetch changes up to 619b4b0ba832144d4be899640a2047f9675df849:
virtio/s390: implement PM operations for virtio_ccw (2017-12-14 10:32:21 +0100)
----------------------------------------------------------------
Hibernation support for virtio-ccw.
----------------------------------------------------------------
Christian Borntraeger (1):
virtio/s390: implement PM operations for virtio_ccw
drivers/s390/virtio/virtio_ccw.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
--
2.13.6
^ permalink raw reply
* Re: [PATCH v19 3/7] xbitmap: add more operations
From: Wei Wang @ 2017-12-14 3:47 UTC (permalink / raw)
To: Tetsuo Handa
Cc: yang.zhang.wz, kvm, mst, liliang.opensource, qemu-devel,
virtualization, linux-mm, aarcange, virtio-dev, mawilcox, willy,
quan.xu, nilal, riel, cornelia.huck, mhocko, linux-kernel,
amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <201712132316.EJJ57332.MFOSJHOFFVLtQO@I-love.SAKURA.ne.jp>
On 12/13/2017 10:16 PM, Tetsuo Handa wrote:
> Wei Wang wrote:
>> On 12/12/2017 09:20 PM, Tetsuo Handa wrote:
>>> Wei Wang wrote:
>>>> +void xb_clear_bit_range(struct xb *xb, unsigned long start, unsigned long end)
>>>> +{
>>>> + struct radix_tree_root *root = &xb->xbrt;
>>>> + struct radix_tree_node *node;
>>>> + void **slot;
>>>> + struct ida_bitmap *bitmap;
>>>> + unsigned int nbits;
>>>> +
>>>> + for (; start < end; start = (start | (IDA_BITMAP_BITS - 1)) + 1) {
>>>> + unsigned long index = start / IDA_BITMAP_BITS;
>>>> + unsigned long bit = start % IDA_BITMAP_BITS;
>>>> +
>>>> + bitmap = __radix_tree_lookup(root, index, &node, &slot);
>>>> + if (radix_tree_exception(bitmap)) {
>>>> + unsigned long ebit = bit + 2;
>>>> + unsigned long tmp = (unsigned long)bitmap;
>>>> +
>>>> + nbits = min(end - start + 1, BITS_PER_LONG - ebit);
>>>> +
>>>> + if (ebit >= BITS_PER_LONG)
>>> What happens if we hit this "continue;" when "index == ULONG_MAX / IDA_BITMAP_BITS" ?
>> Thanks. I also improved the test case for this. I plan to change the
>> implementation a little bit to avoid such overflow (has passed the test
>> case that I have, just post out for another set of eyes):
>>
>> {
>> ...
>> unsigned long idx = start / IDA_BITMAP_BITS;
>> unsigned long bit = start % IDA_BITMAP_BITS;
>> unsigned long idx_end = end / IDA_BITMAP_BITS;
>> unsigned long ret;
>>
>> for (idx = start / IDA_BITMAP_BITS; idx <= idx_end; idx++) {
>> unsigned long ida_start = idx * IDA_BITMAP_BITS;
>>
>> bitmap = __radix_tree_lookup(root, idx, &node, &slot);
>> if (radix_tree_exception(bitmap)) {
>> unsigned long tmp = (unsigned long)bitmap;
>> unsigned long ebit = bit + 2;
>>
>> if (ebit >= BITS_PER_LONG)
>> continue;
> Will you please please do eliminate exception path?
Please first see my explanations below, I'll try to help you understand
it thoroughly. If it is really too complex to understand it finally,
then I think we can start from the fundamental part by removing the
exceptional path if no objections from others.
> I can't interpret what "ebit >= BITS_PER_LONG" means.
> The reason you "continue;" is that all bits beyond are "0", isn't it?
> Then, it would make sense to "continue;" when finding next "1" because
> all bits beyond are "0". But how does it make sense to "continue;" when
> finding next "0" despite all bits beyond are "0"?
Not the case actually. Please see this example:
1) xb_set_bit(10); // bit 10 is set, so an exceptional entry (i.e.
[0:62]) is used
2) xb_clear_bit_range(66, 2048);
- One ida bitmap size is 1024 bits, so this clear will be performed
with 2 loops, first to clear [66, 1024), second to clear [1024, 2048)
- When the first loop clears [66, 1024), and finds that it is an
exception entry (because bit 10 is set, and the 62 bit entry is enough
to cover). Another point we have to remember is that an exceptional
entry implies that the rest of bits [63, 1024) are all 0s.
- The starting bit 66 already exceeds the the exceptional entry bit
range [0, 62], and with the fact that the rest of bits are all 0s, so it
is time to just "continue", which goes to the second range [1024, 2048)
I used the example of xb_clear_bit_range(), and xb_find_next_bit() is
the same fundamentally. Please let me know if anywhere still looks fuzzy.
>
>> if (set)
>> ret = find_next_bit(&tmp,
>> BITS_PER_LONG, ebit);
>> else
>> ret = find_next_zero_bit(&tmp,
>> BITS_PER_LONG,
>> ebit);
>> if (ret < BITS_PER_LONG)
>> return ret - 2 + ida_start;
>> } else if (bitmap) {
>> if (set)
>> ret = find_next_bit(bitmap->bitmap,
>> IDA_BITMAP_BITS, bit);
>> else
>> ret = find_next_zero_bit(bitmap->bitmap,
>> IDA_BITMAP_BITS, bit);
> "bit" may not be 0 for the first round and "bit" is always 0 afterwords.
> But where is the guaranteed that "end" is a multiple of IDA_BITMAP_BITS ?
> Please explain why it is correct to use IDA_BITMAP_BITS unconditionally
> for the last round.
There missed something here, it will be:
nbits = min(end - ida_start + 1, IDA_BITMAP_BITS - bit);
if (set)
ret = find_next_bit(bitmap->bitmap, nbits, bit);
else
ret = find_next_zero_bit(bitmap->bitmap,
nbits, bit);
if (ret < nbits)
return ret + ida_start;
>>>> +/**
>>>> + * xb_find_next_set_bit - find the next set bit in a range
>>>> + * @xb: the xbitmap to search
>>>> + * @start: the start of the range, inclusive
>>>> + * @end: the end of the range, exclusive
>>>> + *
>>>> + * Returns: the index of the found bit, or @end + 1 if no such bit is found.
>>>> + */
>>>> +unsigned long xb_find_next_set_bit(struct xb *xb, unsigned long start,
>>>> + unsigned long end)
>>>> +{
>>>> + return xb_find_next_bit(xb, start, end, 1);
>>>> +}
>>> Won't "exclusive" loose ability to handle ULONG_MAX ? Since this is a
>>> library module, missing ability to handle ULONG_MAX sounds like an omission.
>>> Shouldn't we pass (or return) whether "found or not" flag (e.g. strtoul() in
>>> C library function)?
>>>
>>> bool xb_find_next_set_bit(struct xb *xb, unsigned long start, unsigned long end, unsigned long *result);
>>> unsigned long xb_find_next_set_bit(struct xb *xb, unsigned long start, unsigned long end, bool *found);
>> Yes, ULONG_MAX needs to be tested by xb_test_bit(). Compared to checking
>> the return value, would it be the same to let the caller check for the
>> ULONG_MAX boundary?
>>
> Why the caller needs to care about whether it is ULONG_MAX or not?
I don't stick with this one, and will use the method that you suggested.
Thanks for the review.
>
> Also, one more thing you need to check. Have you checked how long does
> xb_find_next_set_bit(xb, 0, ULONG_MAX) on an empty xbitmap takes?
> If it causes soft lockup warning, should we add cond_resched() ?
> If yes, you have to document that this API might sleep. If no, you
> have to document that the caller of this API is responsible for
> not to pass such a large value range.
Yes, that will take too long time. Probably we can document some
comments as a reminder for the callers.
Best,
Wei
^ permalink raw reply
* Re: [PATCHv2] virtio_mmio: fix devm cleanup
From: Mark Rutland @ 2017-12-13 14:34 UTC (permalink / raw)
To: Cornelia Huck, Michael S . Tsirkin
Cc: weiping zhang, linux-kernel, virtualization
In-Reply-To: <20171212180223.2dfb3b40.cohuck@redhat.com>
On Tue, Dec 12, 2017 at 06:02:23PM +0100, Cornelia Huck wrote:
> On Tue, 12 Dec 2017 13:45:50 +0000
> Mark Rutland <mark.rutland@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 (00000000c05b4844)
> > [ 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.rutland@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 <cohuck@redhat.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: weiping zhang <zhangweiping@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 <cohuck@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.
^ permalink raw reply
* Re: [PATCH v19 3/7] xbitmap: add more operations
From: Wei Wang @ 2017-12-13 12:26 UTC (permalink / raw)
To: Tetsuo Handa, virtio-dev, linux-kernel, qemu-devel,
virtualization, kvm, linux-mm, mst, mhocko, akpm, mawilcox
Cc: aarcange, yang.zhang.wz, riel, liliang.opensource, willy,
amit.shah, quan.xu, cornelia.huck, pbonzini, nilal, mgorman
In-Reply-To: <201712122220.IFH05261.LtJOFFSFHVMQOO@I-love.SAKURA.ne.jp>
On 12/12/2017 09:20 PM, Tetsuo Handa wrote:
> Wei Wang wrote:
>> +void xb_clear_bit_range(struct xb *xb, unsigned long start, unsigned long end)
>> +{
>> + struct radix_tree_root *root = &xb->xbrt;
>> + struct radix_tree_node *node;
>> + void **slot;
>> + struct ida_bitmap *bitmap;
>> + unsigned int nbits;
>> +
>> + for (; start < end; start = (start | (IDA_BITMAP_BITS - 1)) + 1) {
>> + unsigned long index = start / IDA_BITMAP_BITS;
>> + unsigned long bit = start % IDA_BITMAP_BITS;
>> +
>> + bitmap = __radix_tree_lookup(root, index, &node, &slot);
>> + if (radix_tree_exception(bitmap)) {
>> + unsigned long ebit = bit + 2;
>> + unsigned long tmp = (unsigned long)bitmap;
>> +
>> + nbits = min(end - start + 1, BITS_PER_LONG - ebit);
>> +
>> + if (ebit >= BITS_PER_LONG)
> What happens if we hit this "continue;" when "index == ULONG_MAX / IDA_BITMAP_BITS" ?
Thanks. I also improved the test case for this. I plan to change the
implementation a little bit to avoid such overflow (has passed the test
case that I have, just post out for another set of eyes):
{
...
unsigned long idx = start / IDA_BITMAP_BITS;
unsigned long bit = start % IDA_BITMAP_BITS;
unsigned long idx_end = end / IDA_BITMAP_BITS;
unsigned long ret;
for (idx = start / IDA_BITMAP_BITS; idx <= idx_end; idx++) {
unsigned long ida_start = idx * IDA_BITMAP_BITS;
bitmap = __radix_tree_lookup(root, idx, &node, &slot);
if (radix_tree_exception(bitmap)) {
unsigned long tmp = (unsigned long)bitmap;
unsigned long ebit = bit + 2;
if (ebit >= BITS_PER_LONG)
continue;
if (set)
ret = find_next_bit(&tmp,
BITS_PER_LONG, ebit);
else
ret = find_next_zero_bit(&tmp,
BITS_PER_LONG,
ebit);
if (ret < BITS_PER_LONG)
return ret - 2 + ida_start;
} else if (bitmap) {
if (set)
ret = find_next_bit(bitmap->bitmap,
IDA_BITMAP_BITS, bit);
else
ret = find_next_zero_bit(bitmap->bitmap,
IDA_BITMAP_BITS, bit);
if (ret < IDA_BITMAP_BITS)
return ret + ida_start;
} else if (!bitmap && !set) {
return bit + IDA_BITMAP_BITS * idx;
}
bit = 0;
}
return end;
}
>
> Can you eliminate exception path and fold all xbitmap patches into one, and
> post only one xbitmap patch without virtio-baloon changes? If exception path
> is valuable, you can add exception path after minimum version is merged.
> This series is too difficult for me to close corner cases.
That exception path is claimed to save memory, and I don't have a strong
reason to remove that part.
Matthew, could we get your feedback on this?
>
>> +/**
>> + * xb_find_next_set_bit - find the next set bit in a range
>> + * @xb: the xbitmap to search
>> + * @start: the start of the range, inclusive
>> + * @end: the end of the range, exclusive
>> + *
>> + * Returns: the index of the found bit, or @end + 1 if no such bit is found.
>> + */
>> +unsigned long xb_find_next_set_bit(struct xb *xb, unsigned long start,
>> + unsigned long end)
>> +{
>> + return xb_find_next_bit(xb, start, end, 1);
>> +}
> Won't "exclusive" loose ability to handle ULONG_MAX ? Since this is a
> library module, missing ability to handle ULONG_MAX sounds like an omission.
> Shouldn't we pass (or return) whether "found or not" flag (e.g. strtoul() in
> C library function)?
>
> bool xb_find_next_set_bit(struct xb *xb, unsigned long start, unsigned long end, unsigned long *result);
> unsigned long xb_find_next_set_bit(struct xb *xb, unsigned long start, unsigned long end, bool *found);
Yes, ULONG_MAX needs to be tested by xb_test_bit(). Compared to checking
the return value, would it be the same to let the caller check for the
ULONG_MAX boundary?
Best,
Wei
^ permalink raw reply
* [bpf-next V1-RFC PATCH 11/14] virtio_net: setup xdp_rxq_info
From: Jesper Dangaard Brouer @ 2017-12-13 11:20 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov
Cc: Michael S. Tsirkin, netdev, Jesper Dangaard Brouer,
virtualization, dsahern, gospo, bjorn.topel, michael.chan
In-Reply-To: <151316391502.14967.13292358380181773729.stgit@firesoul>
The virtio_net driver doesn't dynamically change the RX-ring queue
layout and backing pages, but instead reject XDP setup if all the
conditions for XDP is not meet. Thus, the xdp_rxq_info also remains
fairly static. This allow us to simply add the init+reg/unreg to
net_device open/close functions.
Driver hook points for xdp_rxq_info:
* init+reg: virtnet_open
* unreg : virtnet_close
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
drivers/net/virtio_net.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 19a985ef9104..9792183befbf 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -31,6 +31,7 @@
#include <linux/average.h>
#include <linux/filter.h>
#include <net/route.h>
+#include <net/xdp.h>
static int napi_weight = NAPI_POLL_WEIGHT;
module_param(napi_weight, int, 0444);
@@ -115,6 +116,8 @@ struct receive_queue {
/* Name of this receive queue: input.$index */
char name[40];
+
+ struct xdp_rxq_info xdp_rxq;
};
struct virtnet_info {
@@ -556,6 +559,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
xdp.data = xdp.data_hard_start + xdp_headroom;
xdp_set_data_meta_invalid(&xdp);
xdp.data_end = xdp.data + len;
+ xdp.rxq = &rq->xdp_rxq;
orig_data = xdp.data;
act = bpf_prog_run_xdp(xdp_prog, &xdp);
@@ -1229,6 +1233,13 @@ static int virtnet_open(struct net_device *dev)
/* Make sure we have some buffers: if oom use wq. */
if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
schedule_delayed_work(&vi->refill, 0);
+
+ /* XDP RX queue info */
+ xdp_rxq_info_init(&vi->rq[i].xdp_rxq);
+ vi->rq[i].xdp_rxq.dev = dev;
+ vi->rq[i].xdp_rxq.queue_index = i;
+ xdp_rxq_info_reg(&vi->rq[i].xdp_rxq);
+
virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
virtnet_napi_tx_enable(vi, vi->sq[i].vq, &vi->sq[i].napi);
}
@@ -1557,6 +1568,7 @@ static int virtnet_close(struct net_device *dev)
cancel_delayed_work_sync(&vi->refill);
for (i = 0; i < vi->max_queue_pairs; i++) {
+ xdp_rxq_info_unreg(&vi->rq[i].xdp_rxq);
napi_disable(&vi->rq[i].napi);
virtnet_napi_tx_disable(&vi->sq[i].napi);
}
^ permalink raw reply related
* Re: [PATCH v2 2/3] virtio: use put_device instead of kfree
From: Cornelia Huck @ 2017-12-13 8:27 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
In-Reply-To: <11026fc88be1033d7ce5bc8ecb9b355d27d82075.1513083885.git.zhangweiping@didichuxing.com>
On Tue, 12 Dec 2017 21:24:33 +0800
weiping zhang <zhangweiping@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.
> */
> so we don't free vp_vdev until vp_vdev.dev.release be called.
>
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
> drivers/misc/mic/vop/vop_main.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply
* Re: [PATCHv2] virtio_mmio: fix devm cleanup
From: Cornelia Huck @ 2017-12-12 17:02 UTC (permalink / raw)
To: Mark Rutland
Cc: virtualization, weiping zhang, linux-kernel, Michael S . Tsirkin
In-Reply-To: <20171212134550.18378-1-mark.rutland@arm.com>
On Tue, 12 Dec 2017 13:45:50 +0000
Mark Rutland <mark.rutland@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 (00000000c05b4844)
> [ 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.rutland@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 <cohuck@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: weiping zhang <zhangweiping@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 <cohuck@redhat.com>
^ permalink raw reply
* (unknown),
From: Solen win @ 2017-12-12 16:06 UTC (permalink / raw)
To: virtualization
[-- Attachment #1.1: Type: text/plain, Size: 23 bytes --]
Solenwin@freshdesk.com
[-- Attachment #1.2: Type: text/html, Size: 141 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCHv2] virtio_mmio: fix devm cleanup
From: weiping zhang @ 2017-12-12 15:04 UTC (permalink / raw)
To: Mark Rutland
Cc: Michael S . Tsirkin, Cornelia Huck, weiping zhang, linux-kernel,
virtualization
In-Reply-To: <20171212144506.c4z6aqieo5uo3pcn@lakrids.cambridge.arm.com>
2017-12-12 22:45 GMT+08:00 Mark Rutland <mark.rutland@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.rutland@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_head entry;
>> dr_release_t release;
>> #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, &todo, node.entry) {
> devres_log(dev, &dr->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.
^ permalink raw reply
* Re: [PATCHv2] virtio_mmio: fix devm cleanup
From: Mark Rutland @ 2017-12-12 14:45 UTC (permalink / raw)
To: weiping zhang
Cc: Michael S . Tsirkin, Cornelia Huck, weiping zhang, linux-kernel,
virtualization
In-Reply-To: <CAA70yB7qN_50xFFmx5Vxs61teSjBjm2jCzZY=XqwNiLU_dWMgw@mail.gmail.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.rutland@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_head entry;
> dr_release_t release;
> #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, &todo, node.entry) {
devres_log(dev, &dr->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.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCHv2] virtio_mmio: fix devm cleanup
From: weiping zhang @ 2017-12-12 14:26 UTC (permalink / raw)
To: Mark Rutland
Cc: Michael S . Tsirkin, Cornelia Huck, weiping zhang, linux-kernel,
virtualization
In-Reply-To: <20171212134550.18378-1-mark.rutland@arm.com>
2017-12-12 21:45 GMT+08:00 Mark Rutland <mark.rutland@arm.com>:
> 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 (00000000c05b4844)
> [ 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.rutland@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 <cohuck@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: weiping zhang <zhangweiping@didichuxing.com>
> Cc: virtualization@lists.linux-foundation.org
> ---
> drivers/virtio/virtio_mmio.c | 43 +++++++++----------------------------------
> 1 file changed, 9 insertions(+), 34 deletions(-)
>
> Since v1 [1]:
> * Fix cleanup in virtio_mmio_remove
>
> Mark.
>
> [1] https://lkml.kernel.org/r/20171212125302.6846-1-mark.rutland@arm.com
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index a9192fe4f345..c92131edfaba 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -522,10 +522,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> return -EBUSY;
>
> vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
> - if (!vm_dev) {
> - rc = -ENOMEM;
> - goto free_mem;
> - }
> + if (!vm_dev)
> + return -ENOMEM;
>
> vm_dev->vdev.dev.parent = &pdev->dev;
> vm_dev->vdev.dev.release = virtio_mmio_release_dev;
> @@ -535,17 +533,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> spin_lock_init(&vm_dev->lock);
>
> vm_dev->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
> - if (vm_dev->base == NULL) {
> - rc = -EFAULT;
> - goto free_vmdev;
> - }
> + if (vm_dev->base == NULL)
> + return -EFAULT;
>
> /* Check magic value */
> magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
> if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
> dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
> - rc = -ENODEV;
> - goto unmap;
> + return -ENODEV;
> }
>
> /* Check device version */
> @@ -553,8 +548,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> if (vm_dev->version < 1 || vm_dev->version > 2) {
> dev_err(&pdev->dev, "Version %ld not supported!\n",
> vm_dev->version);
> - rc = -ENXIO;
> - goto unmap;
> + return -ENXIO;
> }
>
> vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
> @@ -563,8 +557,7 @@ 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.
> */
> - rc = -ENODEV;
> - goto unmap;
> + return -ENODEV;
> }
> vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
>
> @@ -590,33 +583,15 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, vm_dev);
>
> rc = register_virtio_device(&vm_dev->vdev);
> - if (rc) {
> - iounmap(vm_dev->base);
> - devm_release_mem_region(&pdev->dev, mem->start,
> - resource_size(mem));
> + if (rc)
> put_device(&vm_dev->vdev.dev);
> - }
> - return rc;
> -unmap:
> - iounmap(vm_dev->base);
> -free_mem:
> - devm_release_mem_region(&pdev->dev, mem->start,
> - resource_size(mem));
> -free_vmdev:
> - devm_kfree(&pdev->dev, vm_dev);
> +
> return rc;
> }
>
> 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(&pdev->dev, mem->start,
> - resource_size(mem));
> unregister_virtio_device(&vm_dev->vdev);
>
> return 0;
> --
> 2.11.0
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Hi Mark,
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_head entry;
dr_release_t release;
#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 */
};
1) devm_request_mem_region -> __devm_request_region
dr = devres_alloc(devm_region_release, sizeof(struct region_devres),
"devm_region_release" will call __release_region to release resource
2) devm_kzalloc -> devm_kmalloc
dr = alloc_dr(devm_kmalloc_release, size, gfp, dev_to_node(dev));
"devm_kmalloc_release" is noop, do nothing.
3) devm_ioremap -> ... -> __devres_alloc_node
ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
devm_ioremap_release do iounmap
so for case 2) above, we need a devm_kfree() before call register_virtio_device
^ permalink raw reply
* [PATCHv2] virtio_mmio: fix devm cleanup
From: Mark Rutland @ 2017-12-12 13:45 UTC (permalink / raw)
To: linux-kernel
Cc: Mark Rutland, Cornelia Huck, weiping zhang, virtualization,
Michael S . Tsirkin
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 (00000000c05b4844)
[ 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.rutland@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 <cohuck@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: weiping zhang <zhangweiping@didichuxing.com>
Cc: virtualization@lists.linux-foundation.org
---
drivers/virtio/virtio_mmio.c | 43 +++++++++----------------------------------
1 file changed, 9 insertions(+), 34 deletions(-)
Since v1 [1]:
* Fix cleanup in virtio_mmio_remove
Mark.
[1] https://lkml.kernel.org/r/20171212125302.6846-1-mark.rutland@arm.com
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index a9192fe4f345..c92131edfaba 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -522,10 +522,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
return -EBUSY;
vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
- if (!vm_dev) {
- rc = -ENOMEM;
- goto free_mem;
- }
+ if (!vm_dev)
+ return -ENOMEM;
vm_dev->vdev.dev.parent = &pdev->dev;
vm_dev->vdev.dev.release = virtio_mmio_release_dev;
@@ -535,17 +533,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
spin_lock_init(&vm_dev->lock);
vm_dev->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
- if (vm_dev->base == NULL) {
- rc = -EFAULT;
- goto free_vmdev;
- }
+ if (vm_dev->base == NULL)
+ return -EFAULT;
/* Check magic value */
magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
- rc = -ENODEV;
- goto unmap;
+ return -ENODEV;
}
/* Check device version */
@@ -553,8 +548,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
if (vm_dev->version < 1 || vm_dev->version > 2) {
dev_err(&pdev->dev, "Version %ld not supported!\n",
vm_dev->version);
- rc = -ENXIO;
- goto unmap;
+ return -ENXIO;
}
vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
@@ -563,8 +557,7 @@ 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.
*/
- rc = -ENODEV;
- goto unmap;
+ return -ENODEV;
}
vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
@@ -590,33 +583,15 @@ static int virtio_mmio_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, vm_dev);
rc = register_virtio_device(&vm_dev->vdev);
- if (rc) {
- iounmap(vm_dev->base);
- devm_release_mem_region(&pdev->dev, mem->start,
- resource_size(mem));
+ if (rc)
put_device(&vm_dev->vdev.dev);
- }
- return rc;
-unmap:
- iounmap(vm_dev->base);
-free_mem:
- devm_release_mem_region(&pdev->dev, mem->start,
- resource_size(mem));
-free_vmdev:
- devm_kfree(&pdev->dev, vm_dev);
+
return rc;
}
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(&pdev->dev, mem->start,
- resource_size(mem));
unregister_virtio_device(&vm_dev->vdev);
return 0;
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] virtio_mmio: fix devm cleanup
From: Mark Rutland @ 2017-12-12 13:34 UTC (permalink / raw)
To: Cornelia Huck
Cc: virtualization, weiping zhang, linux-kernel, Michael S . Tsirkin
In-Reply-To: <20171212143148.4a022816.cohuck@redhat.com>
On Tue, Dec 12, 2017 at 02:31:48PM +0100, Cornelia Huck wrote:
> On Tue, 12 Dec 2017 13:29:02 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> > On Tue, Dec 12, 2017 at 02:09:52PM +0100, Cornelia Huck wrote:
>
> > > Shouldn't the cleanup in _remove() then be removed as well?
> >
> > Ah, I'd missed that.
> >
> > I believe that can be removed, yes.
> >
> > Should I spin a v2 with that folded in?
>
> I think it has the same devm_ioremap vs. iounmap discrepancy, so it
> should probably be folded in.
Ah, yes. I'll spin a v2 now.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH] virtio_mmio: fix devm cleanup
From: Cornelia Huck @ 2017-12-12 13:31 UTC (permalink / raw)
To: Mark Rutland
Cc: virtualization, weiping zhang, linux-kernel, Michael S . Tsirkin
In-Reply-To: <20171212132902.t5vxs2yujsth2v2h@lakrids.cambridge.arm.com>
On Tue, 12 Dec 2017 13:29:02 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Dec 12, 2017 at 02:09:52PM +0100, Cornelia Huck wrote:
> > Shouldn't the cleanup in _remove() then be removed as well?
>
> Ah, I'd missed that.
>
> I believe that can be removed, yes.
>
> Should I spin a v2 with that folded in?
I think it has the same devm_ioremap vs. iounmap discrepancy, so it
should probably be folded in.
^ permalink raw reply
* Re: [PATCH] virtio_mmio: fix devm cleanup
From: Mark Rutland @ 2017-12-12 13:29 UTC (permalink / raw)
To: Cornelia Huck
Cc: virtualization, weiping zhang, linux-kernel, Michael S . Tsirkin
In-Reply-To: <20171212140952.64d7908a.cohuck@redhat.com>
On Tue, Dec 12, 2017 at 02:09:52PM +0100, Cornelia Huck wrote:
> On Tue, 12 Dec 2017 12:53:02 +0000
> Mark Rutland <mark.rutland@arm.com> wrote:
>
> > Recent rework of the virtio_mmio probe path 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 failure on
> > arm64 under QEMU 2.9.0:
> >
> > [ 3.450397] ------------[ cut here ]------------
> > [ 3.453822] Trying to vfree() nonexistent vm area (00000000c05b4844)
> > [ 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
>
> Oops.
>
> > 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. We only need to ensure that we call put_device() if a call to
> > register_virtio_device() fails.
>
> OK, that was the subtility I obviously missed. Reading through the
> code, this seems correct (although I find the infrastructure a bit
> unintuitive).
No worries.
> Shouldn't the cleanup in _remove() then be removed as well?
Ah, I'd missed that.
I believe that can be removed, yes.
Should I spin a v2 with that folded in?
Thanks,
Mark.
^ permalink raw reply
* [PATCH v2 3/3] virtio: put reference count of virtio_device.dev
From: weiping zhang @ 2017-12-12 13:25 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
In-Reply-To: <cover.1513083885.git.zhangweiping@didichuxing.com>
rproc_virtio_dev_release will be called iff virtio_device.dev's
refer count became to 0. Here we should put vdev.dev, and then
rproc->dev's cleanup will be done in rproc_virtio_dev_release.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
Reviewed-by: Cornelia Huck <cohuck@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(&rproc->dev);
+ put_device(&vdev->dev);
dev_err(dev, "failed to register vdev: %d\n", ret);
goto out;
}
--
2.9.4
^ permalink raw reply related
* [PATCH v2 2/3] virtio: use put_device instead of kfree
From: weiping zhang @ 2017-12-12 13:24 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
In-Reply-To: <cover.1513083885.git.zhangweiping@didichuxing.com>
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 <zhangweiping@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->vdev.dev);
+ return ret;
}
writeq((u64)vdev, &vdev->dc->vdev);
dev_dbg(_vop_dev(vdev), "%s: registered vop device %u type %u vdev %p\n",
@@ -509,8 +513,6 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
return 0;
-free_irq:
- vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
kfree:
kfree(vdev);
return ret;
@@ -568,7 +570,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
* [PATCH v2 1/3] virtio_pci: use put_device instead of kfree
From: weiping zhang @ 2017-12-12 13:24 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
In-Reply-To: <cover.1513083885.git.zhangweiping@didichuxing.com>
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>
Reviewed-by: Cornelia Huck <cohuck@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(&vp_dev->vdev);
- if (rc)
- goto err_register;
+ if (rc) {
+ if (vp_dev->ioaddr)
+ virtio_pci_legacy_remove(vp_dev);
+ else
+ virtio_pci_modern_remove(vp_dev);
+ pci_disable_device(pci_dev);
+ put_device(&vp_dev->vdev.dev);
+ }
- return 0;
+ return rc;
-err_register:
- if (vp_dev->ioaddr)
- virtio_pci_legacy_remove(vp_dev);
- else
- virtio_pci_modern_remove(vp_dev);
err_probe:
pci_disable_device(pci_dev);
err_enable_device:
--
2.9.4
^ permalink raw reply related
* [PATCH v2 0/3] fix cleanup for fail to register_virtio_device
From: weiping zhang @ 2017-12-12 13:13 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
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
^ permalink raw reply
* Re: [PATCH 2/3] virtio: use put_device instead of kfree
From: weiping zhang @ 2017-12-12 13:13 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, mst
In-Reply-To: <20171212111742.4249c49e.cohuck@redhat.com>
On Tue, Dec 12, 2017 at 11:17:42AM +0100, Cornelia Huck wrote:
> On Mon, 11 Dec 2017 23:55:26 +0800
> weiping zhang <zhangweiping@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 <zhangweiping@didichuxing.com>
> > ---
> > drivers/misc/mic/vop/vop_main.c | 16 +++++++++-------
> > 1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
> > index a341938..8c716a0 100644
> > --- a/drivers/misc/mic/vop/vop_main.c
> > +++ b/drivers/misc/mic/vop/vop_main.c
> > @@ -452,10 +452,12 @@ static irqreturn_t vop_virtio_intr_handler(int irq, void *data)
> >
> > static void vop_virtio_release_dev(struct device *_d)
> > {
> > - /*
> > - * No need for a release method similar to virtio PCI.
> > - * Provide an empty one to avoid getting a warning from core.
> > - */
> > + struct virtio_device *vdev =
> > + container_of(_d, struct virtio_device, dev);
> > + struct _vop_vdev *vop_vdev =
> > + container_of(vdev, struct _vop_vdev, vdev);
> > +
> > + kfree(vop_vdev);
> > }
> >
> > /*
> > @@ -501,7 +503,9 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
> > dev_err(_vop_dev(vdev),
> > "Failed to register vop device %u type %u\n",
> > offset, type);
> > - goto free_irq;
> > + vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
> > + put_device(&vdev->vdev.dev);
> > + return ret;
> > }
> > writeq((u64)vdev, &vdev->dc->vdev);
> > dev_dbg(_vop_dev(vdev), "%s: registered vop device %u type %u vdev %p\n",
> > @@ -509,8 +513,6 @@ static int _vop_add_device(struct mic_device_desc __iomem *d,
> >
> > return 0;
> >
> > -free_irq:
> > - vpdev->hw_ops->free_irq(vpdev, vdev->virtio_cookie, vdev);
> > kfree:
> > kfree(vdev);
> > return ret;
>
> BTW, the kfree(vdev) in _vop_remove_device() is also wrong (needs to be
> a put_device() as well).
thanks for your comments, I'll fix it at v2.
^ permalink raw reply
* Re: [PATCH 1/3] virtio_pci: use put_device instead of kfree
From: weiping zhang @ 2017-12-12 13:11 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, mst
In-Reply-To: <20171212110025.5d029fce.cohuck@redhat.com>
On Tue, Dec 12, 2017 at 11:00:25AM +0100, Cornelia Huck wrote:
> On Mon, 11 Dec 2017 23:55:16 +0800
> weiping zhang <zhangweiping@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 <zhangweiping@didichuxing.com>
> > ---
> > drivers/virtio/virtio_pci_common.c | 17 +++++++++--------
> > 1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > index 1c4797e..91d20f7 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -551,16 +551,17 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> > pci_set_master(pci_dev);
> >
> > rc = register_virtio_device(&vp_dev->vdev);
> > - if (rc)
> > - goto err_register;
> > + if (rc) {
> > + if (vp_dev->ioaddr)
> > + virtio_pci_legacy_remove(vp_dev);
> > + else
> > + virtio_pci_modern_remove(vp_dev);
> > + pci_disable_device(pci_dev);
> > + put_device(&vp_dev->vdev.dev);
> > + }
> >
> > - return 0;
> > + return rc;
> >
> > -err_register:
> > - if (vp_dev->ioaddr)
> > - virtio_pci_legacy_remove(vp_dev);
> > - else
> > - virtio_pci_modern_remove(vp_dev);
> > err_probe:
> > pci_disable_device(pci_dev);
> > err_enable_device:
>
> Otherwise, looks good.
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Thanks a ton
^ permalink raw reply
* Re: [PATCH] virtio_mmio: fix devm cleanup
From: Cornelia Huck @ 2017-12-12 13:09 UTC (permalink / raw)
To: Mark Rutland
Cc: virtualization, weiping zhang, linux-kernel, Michael S . Tsirkin
In-Reply-To: <20171212125302.6846-1-mark.rutland@arm.com>
On Tue, 12 Dec 2017 12:53:02 +0000
Mark Rutland <mark.rutland@arm.com> wrote:
> Recent rework of the virtio_mmio probe path 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 failure on
> arm64 under QEMU 2.9.0:
>
> [ 3.450397] ------------[ cut here ]------------
> [ 3.453822] Trying to vfree() nonexistent vm area (00000000c05b4844)
> [ 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
>
Oops.
> 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. We only need to ensure that we call put_device() if a call to
> register_virtio_device() fails.
OK, that was the subtility I obviously missed. Reading through the
code, this seems correct (although I find the infrastructure a bit
unintuitive).
>
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Fixes: 7eb781b1bbb7136f ("virtio_mmio: add cleanup for virtio_mmio_probe")
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: weiping zhang <zhangweiping@didichuxing.com>
> Cc: virtualization@lists.linux-foundation.org
> ---
> drivers/virtio/virtio_mmio.c | 36 +++++++++---------------------------
> 1 file changed, 9 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index a9192fe4f345..793b1085967f 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -522,10 +522,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> return -EBUSY;
>
> vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
> - if (!vm_dev) {
> - rc = -ENOMEM;
> - goto free_mem;
> - }
> + if (!vm_dev)
> + return -ENOMEM;
>
> vm_dev->vdev.dev.parent = &pdev->dev;
> vm_dev->vdev.dev.release = virtio_mmio_release_dev;
> @@ -535,17 +533,14 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> spin_lock_init(&vm_dev->lock);
>
> vm_dev->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
> - if (vm_dev->base == NULL) {
> - rc = -EFAULT;
> - goto free_vmdev;
> - }
> + if (vm_dev->base == NULL)
> + return -EFAULT;
>
> /* Check magic value */
> magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
> if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
> dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
> - rc = -ENODEV;
> - goto unmap;
> + return -ENODEV;
> }
>
> /* Check device version */
> @@ -553,8 +548,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> if (vm_dev->version < 1 || vm_dev->version > 2) {
> dev_err(&pdev->dev, "Version %ld not supported!\n",
> vm_dev->version);
> - rc = -ENXIO;
> - goto unmap;
> + return -ENXIO;
> }
>
> vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
> @@ -563,8 +557,7 @@ 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.
> */
> - rc = -ENODEV;
> - goto unmap;
> + return -ENODEV;
> }
> vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
>
> @@ -590,20 +583,9 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, vm_dev);
>
> rc = register_virtio_device(&vm_dev->vdev);
> - if (rc) {
> - iounmap(vm_dev->base);
> - devm_release_mem_region(&pdev->dev, mem->start,
> - resource_size(mem));
> + if (rc)
> put_device(&vm_dev->vdev.dev);
> - }
> - return rc;
> -unmap:
> - iounmap(vm_dev->base);
> -free_mem:
> - devm_release_mem_region(&pdev->dev, mem->start,
> - resource_size(mem));
> -free_vmdev:
> - devm_kfree(&pdev->dev, vm_dev);
> +
> return rc;
> }
>
Shouldn't the cleanup in _remove() then be removed as well?
^ permalink raw reply
* Re: [PATCH v19 1/7] xbitmap: Introduce xbitmap
From: Philippe Ombredanne @ 2017-12-12 12:53 UTC (permalink / raw)
To: Wei Wang, mawilcox
Cc: yang.zhang.wz, kvm, mst, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev, willy,
quan.xu, nilal, riel, cornelia.huck, mhocko, LKML, amit.shah,
Paolo Bonzini, akpm, mgorman
In-Reply-To: <1513079759-14169-2-git-send-email-wei.w.wang@intel.com>
Matthew, Wei,
On Tue, Dec 12, 2017 at 12:55 PM, Wei Wang <wei.w.wang@intel.com> wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
>
> The eXtensible Bitmap is a sparse bitmap representation which is
> efficient for set bits which tend to cluster. It supports up to
> 'unsigned long' worth of bits, and this commit adds the bare bones --
> xb_set_bit(), xb_clear_bit() and xb_test_bit().
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Cc: Matthew Wilcox <mawilcox@microsoft.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
[...]
> --- /dev/null
> +++ b/include/linux/xbitmap.h
> @@ -0,0 +1,52 @@
> +/*
> + * eXtensible Bitmaps
> + * Copyright (c) 2017 Microsoft Corporation <mawilcox@microsoft.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * eXtensible Bitmaps provide an unlimited-size sparse bitmap facility.
> + * All bits are initially zero.
> + */
Have you considered using the new SPDX ids here instead? eg. this
would come out as a top line this way:
> +/* SPDX-License-Identifer: GPL-2.0+ */
Overall you get less boilerplate comment and more code, so this is a
win-win for everyone. This would nicely remove the legalese
boilerplate with the same effect, unless you are a legalese lover of
course. See Thomas doc patches for extra details.... and while you are
it if you could spread the words in your team and use it for all past,
present and future contributions, that would be much appreciated.
And as a side benefit to me, it will help me save on paper and ink
ribbons supplies for my dot matrix line printer each time I print the
source code of the whole kernel. ;)
Thanks for your consideration there.
--
Cordially
Philippe Ombredanne
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox