Re: [patch v2] virtio: console: cleanup an error message
Dan Carpenter writes:
> The PTR_ERR(NULL) here is not useful.
>
> Signed-off-by: Dan Carpenter
> ---
> v2: completely different
Applied.
Thanks,
Rusty.
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 1b456fe..4cf46d8 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -2215,10 +2215,8 @@ static int __init init(void)
> }
>
> pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
> - if (!pdrvdata.debugfs_dir) {
> - pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
> -PTR_ERR(pdrvdata.debugfs_dir));
> - }
> + if (!pdrvdata.debugfs_dir)
> + pr_warning("Error creating debugfs dir for virtio-ports\n");
> INIT_LIST_HEAD(&pdrvdata.consoles);
> INIT_LIST_HEAD(&pdrvdata.portdevs);
>
> ___
> 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: [PATCH] kexec/kdump implementation for Xen PV domU
On Mon, Jul 29, 2013 at 07:15:43PM +0200, Daniel Kiper wrote: > Hi all, > > Here I am sending as attachments patches enabling kexec/kdump > support in Xen PV domU. Only x84_64 architecture is supported. > There is no support for i386 but some code could be easily reused. > Here is a description of patches: [...] > - kexec-kernel-only_20121203.patch: this patch fixes timer > issue on Amazon EC2 machines. Hi Daniel, Do you know the cause of this issue? Does it have something to do with singleshot timer migration when offlining/onlining SMP CPUs? --msw ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] kexec/kdump implementation for Xen PV domU
On Mon, Jul 29, 2013 at 07:15:43PM +0200, Daniel Kiper wrote: > Hi all, > > Here I am sending as attachments patches enabling kexec/kdump > support in Xen PV domU. Only x84_64 architecture is supported. > There is no support for i386 but some code could be easily reused. > Here is a description of patches: > - kexec-tools-2.0.3_20120522.patch: patch for kexec-tools > which cleanly applies to version 2.0.3, > - kexec-kernel-only_20120522.patch: main kexec/kdump kernel patch; > it was prepared for quite old custom version of Xen Linux Kernel 2.6.18; > it should apply to publicly available Xen Linux Kernel 2.6.18 after > doing some needed changes, > - kexec-kernel-only_20121119.patch: patch fixes initial boot > structures overwrites on machines with memory larger than 1 GiB; > this is partial solution, > - kexec-kernel-only_20121203.patch: this patch fixes timer > issue on Amazon EC2 machines. > All code is GPL 2 licensed (http://www.gnu.org/licenses/gpl-2.0.html). > Feel free to base your development on this patchset but please do not > remove any copyrights. Addtionally, I am happy to help anybody who is > interested in work on this stuff. > > Big thank you for Acunu Ltd. (http://www.acunu.com/) for > sponsoring initial work on Xen PV domU. Thanks so much for releasing this code, it's much appreciated. I'll take this and work to get it merged upstream as much as possible. greg k-h ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [patch v2] virtio: console: cleanup an error message
On Mon, Jul 29, 2013 at 04:35:38PM +0300, Dan Carpenter wrote:
> On Mon, Jul 29, 2013 at 06:12:31AM -0700, Greg Kroah-Hartman wrote:
> > On Mon, Jul 29, 2013 at 12:41:31PM +0530, Amit Shah wrote:
> > > On (Mon) 22 Jul 2013 [23:41:00], Dan Carpenter wrote:
> > > > The PTR_ERR(NULL) here is not useful.
> > > >
> > > > Signed-off-by: Dan Carpenter
> > > > ---
> > > > v2: completely different
> > > >
> > > > diff --git a/drivers/char/virtio_console.c
> > > > b/drivers/char/virtio_console.c
> > > > index 1b456fe..4cf46d8 100644
> > > > --- a/drivers/char/virtio_console.c
> > > > +++ b/drivers/char/virtio_console.c
> > > > @@ -2215,10 +2215,8 @@ static int __init init(void)
> > > > }
> > > >
> > > > pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
> > > > - if (!pdrvdata.debugfs_dir) {
> > > > - pr_warning("Error %ld creating debugfs dir for
> > > > virtio-ports\n",
> > > > - PTR_ERR(pdrvdata.debugfs_dir));
> > > > - }
> > > > + if (!pdrvdata.debugfs_dir)
> > > > + pr_warning("Error creating debugfs dir for
> > > > virtio-ports\n");
> > >
> > > When debugfs is enabled and creating the dir fails, we'll print this
> > > warning message.
> > >
> > > When debugfs is disabled, we'll get an error return, and not print any
> > > message.
> >
> > Not true, you will still print the message if debugfs is disabled, as
> > .debugfs_dir will not be NULL.
> >
> > Why even print anything at all? It's debugfs, you shouldn't really care
> > that much about it :)
> >
>
> Yes yes. That's what this code does. It only checks for NULL. The
> debugfs was a little confusing at first but I get it now.
Ugh, you are right, that's what I get for writing email before my
morning coffee...
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [patch v2] virtio: console: cleanup an error message
On Mon, Jul 29, 2013 at 06:12:31AM -0700, Greg Kroah-Hartman wrote:
> On Mon, Jul 29, 2013 at 12:41:31PM +0530, Amit Shah wrote:
> > On (Mon) 22 Jul 2013 [23:41:00], Dan Carpenter wrote:
> > > The PTR_ERR(NULL) here is not useful.
> > >
> > > Signed-off-by: Dan Carpenter
> > > ---
> > > v2: completely different
> > >
> > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > > index 1b456fe..4cf46d8 100644
> > > --- a/drivers/char/virtio_console.c
> > > +++ b/drivers/char/virtio_console.c
> > > @@ -2215,10 +2215,8 @@ static int __init init(void)
> > > }
> > >
> > > pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
> > > - if (!pdrvdata.debugfs_dir) {
> > > - pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
> > > -PTR_ERR(pdrvdata.debugfs_dir));
> > > - }
> > > + if (!pdrvdata.debugfs_dir)
> > > + pr_warning("Error creating debugfs dir for virtio-ports\n");
> >
> > When debugfs is enabled and creating the dir fails, we'll print this
> > warning message.
> >
> > When debugfs is disabled, we'll get an error return, and not print any
> > message.
>
> Not true, you will still print the message if debugfs is disabled, as
> .debugfs_dir will not be NULL.
>
> Why even print anything at all? It's debugfs, you shouldn't really care
> that much about it :)
>
Yes yes. That's what this code does. It only checks for NULL. The
debugfs was a little confusing at first but I get it now.
regards,
dan carpenter
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [patch v2] virtio: console: cleanup an error message
On Mon, Jul 29, 2013 at 12:41:31PM +0530, Amit Shah wrote:
> On (Mon) 22 Jul 2013 [23:41:00], Dan Carpenter wrote:
> > The PTR_ERR(NULL) here is not useful.
> >
> > Signed-off-by: Dan Carpenter
> > ---
> > v2: completely different
> >
> > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> > index 1b456fe..4cf46d8 100644
> > --- a/drivers/char/virtio_console.c
> > +++ b/drivers/char/virtio_console.c
> > @@ -2215,10 +2215,8 @@ static int __init init(void)
> > }
> >
> > pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
> > - if (!pdrvdata.debugfs_dir) {
> > - pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
> > - PTR_ERR(pdrvdata.debugfs_dir));
> > - }
> > + if (!pdrvdata.debugfs_dir)
> > + pr_warning("Error creating debugfs dir for virtio-ports\n");
>
> When debugfs is enabled and creating the dir fails, we'll print this
> warning message.
>
> When debugfs is disabled, we'll get an error return, and not print any
> message.
Not true, you will still print the message if debugfs is disabled, as
.debugfs_dir will not be NULL.
Why even print anything at all? It's debugfs, you shouldn't really care
that much about it :)
thanks,
greg k-h
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 2/9] virtio: console: fix race in port_fops_open() and port unplug
Amit Shah writes:
> Between open() being called and processed, the port can be unplugged.
> Check if this happened, and bail out.
>
> A simple test script to reproduce this is:
>
> while true; do for i in $(seq 1 100); do echo $i > /dev/vport0p3; done; done;
>
> This opens and closes the port a lot of times; unplugging the port while
> this is happening triggers the bug.
>
> CC:
> Signed-off-by: Amit Shah
> ---
> drivers/char/virtio_console.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 291f437..b04ec95 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1024,6 +1024,10 @@ static int port_fops_open(struct inode *inode, struct
> file *filp)
>
> /* We get the port with a kref here */
> port = find_port_by_devt(cdev->dev);
> + if (!port) {
> + /* Port was unplugged before we could proceed */
> + return -ENXIO;
> + }
> filp->private_data = port;
>
> /*
> --
> 1.8.1.4
Applied.
Thanks,
Rusty.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 9/9] virtio: console: prevent use-after-free of port name in port unplug
Amit Shah writes: > Remove the debugfs path before freeing port->name, to prevent a possible > use-after-free. > > Reported-by: Jason Wang > Signed-off-by: Amit Shah Applied, Rusty. > --- > drivers/char/virtio_console.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index 4e380c1..e910bec 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -1574,9 +1574,8 @@ static void unplug_port(struct port *port) > device_destroy(pdrvdata.class, port->dev->devt); > cdev_del(port->cdev); > > - kfree(port->name); > - > debugfs_remove(port->debugfs_file); > + kfree(port->name); > > /* >* Locks around here are not necessary - a port can't be > -- > 1.8.1.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 7/9] virtio: console: add locking in port unplug path
Amit Shah writes:
> Port unplug can race with close() in port_fops_release().
> port_fops_release() already takes the necessary locks, ensure
> unplug_port() does that too.
>
> Signed-off-by: Amit Shah
Applied,
Rusty.
> ---
> drivers/char/virtio_console.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 9cbed93..e8b707d 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1541,6 +1541,7 @@ static void unplug_port(struct port *port)
> list_del(&port->list);
> spin_unlock_irq(&port->portdev->ports_lock);
>
> + spin_lock_irq(&port->inbuf_lock);
> if (port->guest_connected) {
> /* Let the app know the port is going down. */
> send_sigio_to_port(port);
> @@ -1551,6 +1552,7 @@ static void unplug_port(struct port *port)
>
> wake_up_interruptible(&port->waitqueue);
> }
> + spin_unlock_irq(&port->inbuf_lock);
>
> if (is_console_port(port)) {
> spin_lock_irq(&pdrvdata_lock);
> --
> 1.8.1.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 8/9] virtio: console: fix locking around send_sigio_to_port()
Amit Shah writes: > send_sigio_to_port() checks the value of guest_connected, which we > always modify under the inbuf_lock; make sure invocations of > send_sigio_to_port() have take the inbuf_lock around the call. > > Signed-off-by: Amit Shah Applied, Rusty. > --- > drivers/char/virtio_console.c | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index e8b707d..4e380c1 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -1670,7 +1670,9 @@ static void handle_control_message(struct ports_device > *portdev, >* If the guest is connected, it'll be interested in >* knowing the host connection state changed. >*/ > + spin_lock_irq(&port->inbuf_lock); > send_sigio_to_port(port); > + spin_unlock_irq(&port->inbuf_lock); > break; > case VIRTIO_CONSOLE_PORT_NAME: > /* > @@ -1790,13 +1792,13 @@ static void in_intr(struct virtqueue *vq) > if (!port->guest_connected && !is_rproc_serial(port->portdev->vdev)) > discard_port_data(port); > > + /* Send a SIGIO indicating new data in case the process asked for it */ > + send_sigio_to_port(port); > + > spin_unlock_irqrestore(&port->inbuf_lock, flags); > > wake_up_interruptible(&port->waitqueue); > > - /* Send a SIGIO indicating new data in case the process asked for it */ > - send_sigio_to_port(port); > - > if (is_console_port(port) && hvc_poll(port->cons.hvc)) > hvc_kick(); > } > -- > 1.8.1.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 6/9] virtio: console: add locks around buffer removal in port unplug path
Amit Shah writes:
> The removal functions act on the vqs, and the vq operations need to be
> locked.
>
> Signed-off-by: Amit Shah
Applied,
Rusty.
> ---
> drivers/char/virtio_console.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 2b68075..9cbed93 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1512,18 +1512,22 @@ static void remove_port_data(struct port *port)
> {
> struct port_buffer *buf;
>
> + spin_lock_irq(&port->inbuf_lock);
> /* Remove unused data this port might have received. */
> discard_port_data(port);
>
> - reclaim_consumed_buffers(port);
> -
> /* Remove buffers we queued up for the Host to send us data in. */
> while ((buf = virtqueue_detach_unused_buf(port->in_vq)))
> free_buf(buf, true);
> + spin_unlock_irq(&port->inbuf_lock);
> +
> + spin_lock_irq(&port->outvq_lock);
> + reclaim_consumed_buffers(port);
>
> /* Free pending buffers from the out-queue. */
> while ((buf = virtqueue_detach_unused_buf(port->out_vq)))
> free_buf(buf, true);
> + spin_unlock_irq(&port->outvq_lock);
> }
>
> /*
> --
> 1.8.1.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 1/9] virtio: console: fix race with port unplug and open/close
Amit Shah writes:
> There's a window between find_port_by_devt() returning a port and us
> taking a kref on the port, where the port could get unplugged. Fix it
> by taking the reference in find_port_by_devt() itself.
>
> Problem reported and analyzed by Mateusz Guzik.
This fix is clearly correct, but what about the other find_port_by_*
functions?
Applied,
Rusty.
>
> CC:
> Reported-by: Mateusz Guzik
> Signed-off-by: Amit Shah
> ---
> drivers/char/virtio_console.c | 13 ++---
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 1b456fe..291f437 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -272,9 +272,12 @@ static struct port *find_port_by_devt_in_portdev(struct
> ports_device *portdev,
> unsigned long flags;
>
> spin_lock_irqsave(&portdev->ports_lock, flags);
> - list_for_each_entry(port, &portdev->ports, list)
> - if (port->cdev->dev == dev)
> + list_for_each_entry(port, &portdev->ports, list) {
> + if (port->cdev->dev == dev) {
> + kref_get(&port->kref);
> goto out;
> + }
> + }
> port = NULL;
> out:
> spin_unlock_irqrestore(&portdev->ports_lock, flags);
> @@ -1019,14 +1022,10 @@ static int port_fops_open(struct inode *inode, struct
> file *filp)
> struct port *port;
> int ret;
>
> + /* We get the port with a kref here */
> port = find_port_by_devt(cdev->dev);
> filp->private_data = port;
>
> - /* Prevent against a port getting hot-unplugged at the same time */
> - spin_lock_irq(&port->portdev->ports_lock);
> - kref_get(&port->kref);
> - spin_unlock_irq(&port->portdev->ports_lock);
> -
> /*
>* Don't allow opening of console port devices -- that's done
>* via /dev/hvc
> --
> 1.8.1.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 4/9] virtio: console: fix raising SIGIO after port unplug
Amit Shah writes:
> SIGIO should be sent when a port gets unplugged. It should only be sent
> to prcesses that have the port opened, and have asked for SIGIO to be
> delivered. We were clearing out guest_connected before calling
> send_sigio_to_port(), resulting in a sigio not getting sent to
> processes.
>
> Fix by setting guest_connected to false after invoking the sigio
> function.
>
> CC:
> Signed-off-by: Amit Shah
Applied,
Rusty.
> ---
> drivers/char/virtio_console.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 6bf0df3..3435348 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1534,12 +1534,14 @@ static void unplug_port(struct port *port)
> spin_unlock_irq(&port->portdev->ports_lock);
>
> if (port->guest_connected) {
> + /* Let the app know the port is going down. */
> + send_sigio_to_port(port);
> +
> + /* Do this after sigio is actually sent */
> port->guest_connected = false;
> port->host_connected = false;
> - wake_up_interruptible(&port->waitqueue);
>
> - /* Let the app know the port is going down. */
> - send_sigio_to_port(port);
> + wake_up_interruptible(&port->waitqueue);
> }
>
> if (is_console_port(port)) {
> --
> 1.8.1.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 3/9] virtio: console: clean up port data immediately at time of unplug
Amit Shah writes: > We used to keep the port's char device structs and the /sys entries > around till the last reference to the port was dropped. This is > actually unnecessary, and resulted in buggy behaviour: > > 1. Open port in guest > 2. Hot-unplug port > 3. Hot-plug a port with the same 'name' property as the unplugged one > > This resulted in hot-plug being unsuccessful, as a port with the same > name already exists (even though it was unplugged). > > This behaviour resulted in a warning message like this one: > > ---8<--- > WARNING: at fs/sysfs/dir.c:512 sysfs_add_one+0xc9/0x130() (Not tainted) > Hardware name: KVM > sysfs: cannot create duplicate filename > '/devices/pci:00/:00:04.0/virtio0/virtio-ports/vport0p1' > > Call Trace: > [] ? warn_slowpath_common+0x87/0xc0 > [] ? warn_slowpath_fmt+0x46/0x50 > [] ? sysfs_add_one+0xc9/0x130 > [] ? create_dir+0x68/0xb0 > [] ? sysfs_create_dir+0x39/0x50 > [] ? kobject_add_internal+0xb9/0x260 > [] ? kobject_add_varg+0x38/0x60 > [] ? kobject_add+0x44/0x70 > [] ? get_device_parent+0xf4/0x1d0 > [] ? device_add+0xc9/0x650 > > ---8<--- > > Instead of relying on guest applications to release all references to > the ports, we should go ahead and unregister the port from all the core > layers. Any open/read calls on the port will then just return errors, > and an unplug/plug operation on the host will succeed as expected. > > This also caused buggy behaviour in case of the device removal (not just > a port): when the device was removed (which means all ports on that > device are removed automatically as well), the ports with active > users would clean up only when the last references were dropped -- and > it would be too late then to be referencing char device pointers, > resulting in oopses: > > ---8<--- > PID: 6162 TASK: 8801147ad500 CPU: 0 COMMAND: "cat" > #0 [88011b9d5a90] machine_kexec at 8103232b > #1 [88011b9d5af0] crash_kexec at 810b9322 > #2 [88011b9d5bc0] oops_end at 814f4a50 > #3 [88011b9d5bf0] die at 8100f26b > #4 [88011b9d5c20] do_general_protection at 814f45e2 > #5 [88011b9d5c50] general_protection at 814f3db5 > [exception RIP: strlen+2] > RIP: 81272ae2 RSP: 88011b9d5d00 RFLAGS: 00010246 > RAX: RBX: 880118901c18 RCX: > RDX: 88011799982c RSI: 00d0 RDI: 3a303030302f3030 > RBP: 88011b9d5d38 R8: 0006 R9: a0134500 > R10: 1000 R11: 1000 R12: 880117a1cc10 > R13: 00d0 R14: 0017 R15: 81aff700 > ORIG_RAX: CS: 0010 SS: 0018 > #6 [88011b9d5d00] kobject_get_path at 8126dc5d > #7 [88011b9d5d40] kobject_uevent_env at 8126e551 > #8 [88011b9d5dd0] kobject_uevent at 8126e9eb > #9 [88011b9d5de0] device_del at 813440c7 > > ---8<--- > > So clean up when we have all the context, and all that's left to do when > the references to the port have dropped is to free up the port struct > itself. > > CC: > Reported-by: chayang > Reported-by: YOGANANTH SUBRAMANIAN > Reported-by: FuXiangChun > Reported-by: Qunfang Zhang > Reported-by: Sibiao Luo > Signed-off-by: Amit Shah Applied, thanks. Cheers, Rusty. > --- > drivers/char/virtio_console.c | 16 > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c > index b04ec95..6bf0df3 100644 > --- a/drivers/char/virtio_console.c > +++ b/drivers/char/virtio_console.c > @@ -1501,14 +1501,6 @@ static void remove_port(struct kref *kref) > > port = container_of(kref, struct port, kref); > > - sysfs_remove_group(&port->dev->kobj, &port_attribute_group); > - device_destroy(pdrvdata.class, port->dev->devt); > - cdev_del(port->cdev); > - > - kfree(port->name); > - > - debugfs_remove(port->debugfs_file); > - > kfree(port); > } > > @@ -1566,6 +1558,14 @@ static void unplug_port(struct port *port) >*/ > port->portdev = NULL; > > + sysfs_remove_group(&port->dev->kobj, &port_attribute_group); > + device_destroy(pdrvdata.class, port->dev->devt); > + cdev_del(port->cdev); > + > + kfree(port->name); > + > + debugfs_remove(port->debugfs_file); > + > /* >* Locks around here are not necessary - a port can't be >* opened after we removed the port struct from ports_list > -- > 1.8.1.4 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 5/9] virtio: console: return -ENODEV on all read operations after unplug
Amit Shah writes:
> If a port gets unplugged while a user is blocked on read(), -ENODEV is
> returned. However, subsequent read()s returned 0, indicating there's no
> host-side connection (but not indicating the device went away).
>
> This also happened when a port was unplugged and the user didn't have
> any blocking operation pending. If the user didn't monitor the SIGIO
> signal, they won't have a chance to find out if the port went away.
>
> Fix by returning -ENODEV on all read()s after the port gets unplugged.
> write() already behaves this way.
>
> CC:
> Signed-off-by: Amit Shah
> ---
> drivers/char/virtio_console.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
Applied,
Rusty.
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 3435348..2b68075 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -749,6 +749,10 @@ static ssize_t port_fops_read(struct file *filp, char
> __user *ubuf,
>
> port = filp->private_data;
>
> + /* Port is hot-unplugged. */
> + if (!port->guest_connected)
> + return -ENODEV;
> +
> if (!port_has_data(port)) {
> /*
>* If nothing's connected on the host just return 0 in
> @@ -765,7 +769,7 @@ static ssize_t port_fops_read(struct file *filp, char
> __user *ubuf,
> if (ret < 0)
> return ret;
> }
> - /* Port got hot-unplugged. */
> + /* Port got hot-unplugged while we were waiting above. */
> if (!port->guest_connected)
> return -ENODEV;
> /*
> --
> 1.8.1.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [patch v2] virtio: console: cleanup an error message
On (Mon) 22 Jul 2013 [23:41:00], Dan Carpenter wrote:
> The PTR_ERR(NULL) here is not useful.
>
> Signed-off-by: Dan Carpenter
> ---
> v2: completely different
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 1b456fe..4cf46d8 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -2215,10 +2215,8 @@ static int __init init(void)
> }
>
> pdrvdata.debugfs_dir = debugfs_create_dir("virtio-ports", NULL);
> - if (!pdrvdata.debugfs_dir) {
> - pr_warning("Error %ld creating debugfs dir for virtio-ports\n",
> -PTR_ERR(pdrvdata.debugfs_dir));
> - }
> + if (!pdrvdata.debugfs_dir)
> + pr_warning("Error creating debugfs dir for virtio-ports\n");
When debugfs is enabled and creating the dir fails, we'll print this
warning message.
When debugfs is disabled, we'll get an error return, and not print any
message.
Seems OK to me.
Reviewed-by: Amit Shah
Rusty, please pick this up.
Amit
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/5] Intel MIC Host Driver Changes for Virtio Devices.
On Wed, Jul 24, 2013 at 08:31:34PM -0700, Sudeep Dutt wrote:
> From: Ashutosh Dixit
>
> This patch introduces the host "Virtio over PCIe" interface for
> Intel MIC. It allows creating user space backends on the host and
> instantiating virtio devices for them on the Intel MIC card. A character
> device per MIC is exposed with IOCTL, mmap and poll callbacks. This allows
> the user space backend to:
> (a) add/remove a virtio device via a device page.
> (b) map (R/O) virtio rings and device page to user space.
> (c) poll for availability of data.
> (d) copy a descriptor or entire descriptor chain to/from the card.
> (e) modify virtio configuration.
> (f) handle virtio device reset.
> The buffers are copied over using CPU copies for this initial patch
> and host initiated MIC DMA support is planned for future patches.
> The avail and desc virtio rings are in host memory and the used ring
> is in card memory to maximize writes across PCIe for performance.
>
> Co-author: Sudeep Dutt
> Signed-off-by: Ashutosh Dixit
> Signed-off-by: Caz Yokoyama
> Signed-off-by: Dasaratharaman Chandramouli
>
> Signed-off-by: Nikhil Rao
> Signed-off-by: Harshavardhan R Kharche
> Signed-off-by: Sudeep Dutt
> Acked-by: Yaozu (Eddie) Dong
> Reviewed-by: Peter P Waskiewicz Jr
I decided to look at the security and ordering of ring accesses.
Doing a quick look, I think I found some issues, see comments below.
If it were possible to reuse existing ring handling code,
such issues would go away automatically.
Which brings me to the next question: have you looked at reusing
some code under drivers/vhost for host side processing?
If not, you probably should.
Is code in vringh.c generic enough to support your use-case,
and if not what exactly are the issues preventing this?
Thanks,
> ---
> drivers/misc/mic/common/mic_device.h | 4 +
> drivers/misc/mic/host/Makefile | 2 +
> drivers/misc/mic/host/mic_boot.c | 2 +
> drivers/misc/mic/host/mic_debugfs.c | 137 +++
> drivers/misc/mic/host/mic_fops.c | 280 ++
> drivers/misc/mic/host/mic_fops.h | 37 ++
> drivers/misc/mic/host/mic_main.c | 24 ++
> drivers/misc/mic/host/mic_virtio.c | 703
> +++
> drivers/misc/mic/host/mic_virtio.h | 108 ++
> include/uapi/linux/Kbuild| 1 +
> include/uapi/linux/mic_common.h | 165 +++-
> include/uapi/linux/mic_ioctl.h | 104 ++
> 12 files changed, 1566 insertions(+), 1 deletion(-)
> create mode 100644 drivers/misc/mic/host/mic_fops.c
> create mode 100644 drivers/misc/mic/host/mic_fops.h
> create mode 100644 drivers/misc/mic/host/mic_virtio.c
> create mode 100644 drivers/misc/mic/host/mic_virtio.h
> create mode 100644 include/uapi/linux/mic_ioctl.h
>
> diff --git a/drivers/misc/mic/common/mic_device.h
> b/drivers/misc/mic/common/mic_device.h
> index 24934b1..7cdeb74 100644
> --- a/drivers/misc/mic/common/mic_device.h
> +++ b/drivers/misc/mic/common/mic_device.h
> @@ -78,4 +78,8 @@ mic_mmio_write(struct mic_mw *mw, u32 val, u32 offset)
> #define MIC_DPLO_SPAD 14
> #define MIC_DPHI_SPAD 15
>
> +/* These values are supposed to be in ext_params on an interrupt */
> +#define MIC_VIRTIO_PARAM_DEV_REMOVE 0x1
> +#define MIC_VIRTIO_PARAM_CONFIG_CHANGED 0x2
> +
> #endif
> diff --git a/drivers/misc/mic/host/Makefile b/drivers/misc/mic/host/Makefile
> index 0608bbb..e02abdb 100644
> --- a/drivers/misc/mic/host/Makefile
> +++ b/drivers/misc/mic/host/Makefile
> @@ -9,3 +9,5 @@ mic_host-objs += mic_sysfs.o
> mic_host-objs += mic_boot.o
> mic_host-objs += mic_smpt.o
> mic_host-objs += mic_debugfs.o
> +mic_host-objs += mic_fops.o
> +mic_host-objs += mic_virtio.o
> diff --git a/drivers/misc/mic/host/mic_boot.c
> b/drivers/misc/mic/host/mic_boot.c
> index 6485a87..40bcb90 100644
> --- a/drivers/misc/mic/host/mic_boot.c
> +++ b/drivers/misc/mic/host/mic_boot.c
> @@ -30,6 +30,7 @@
> #include
>
> #include "mic_common.h"
> +#include "mic_virtio.h"
>
> /**
> * mic_reset - Reset the MIC device.
> @@ -112,6 +113,7 @@ void mic_stop(struct mic_device *mdev, bool force)
> {
> mutex_lock(&mdev->mic_mutex);
> if (MIC_OFFLINE != mdev->state || force) {
> + mic_virtio_reset_devices(mdev);
> mic_bootparam_init(mdev);
> mic_reset(mdev);
> if (MIC_RESET_FAILED == mdev->state)
> diff --git a/drivers/misc/mic/host/mic_debugfs.c
> b/drivers/misc/mic/host/mic_debugfs.c
> index 5b7697e..bebc6e3 100644
> --- a/drivers/misc/mic/host/mic_debugfs.c
> +++ b/drivers/misc/mic/host/mic_debugfs.c
> @@ -32,6 +32,7 @@
>
> #include "mic_common.h"
> #include "mic_debugfs.h"
> +#include "mic_virtio.h"
>
> /* Debugfs parent dir */
> static struct dentry *mic_dbg;
> @@ -207,7 +208,13 @@ static const struct file_operations post_code_ops = {
> static int dp_seq_show(struct seq_file *s, void *pos)
> {
> struct mic_device *mdev = s->private;
> + struct mic_device_desc *d;
> +
