Re: [PATCHv4] virtio_console: Add support for remoteproc serial

From: Amit Shah
Date: Tue Sep 25 2012 - 00:02:09 EST


On (Mon) 24 Sep 2012 [23:50:01], Sjur BRENDELAND wrote:
> Hi Amit,
>
> > I'm sorry for not being able to look at this earlier.
>
> No worries. I'll try to respin and retest this patch by tomorrow.
> If you by any chance could find time to review so could make it in time
> for 3.7 it would be great :-)

I think it might be late for 3.7 already, I'd prefer to let this bake
for a while, ensure it passes my test suites, at the least. But I'll
let Rusty take the final call.

> > A general comment is to base this patchset on linux-next; we've been
> > seeing more than usual activity for virtio_console this time around.
> > I don't expect the conflicts to be big, though.
>
> Sure, I'll based the next patch on linux-next.
>
> ...
> > > This implementation reuses the existing virtio_console
> > > implementation, and adds support for DMA allocation
> > > of data buffers and disables use of tty console and
> > > the virtio control queue.
> >
> > Any specific reason to not use the control queue? It's just another
> > virtio-serial port; the only special thing about it being it's an
> > internal channel between the device and driver.
>
> Yes, as mention to Michael earlier. I use rproc_serial for talking
> to a modem running in early boot phases, before the OS has started,
> or when the modem is executing it's crash handler. In both these
> cases the modem run in a very limited execution environment, so
> I want to keep the protocol and handling of the vqs as simple as
> possible. Due to this I really don't want more than single pair
> of vqs.

OK.

> We also have very simple use-cases. The port is opened once
> in the life-time of the modem, and only reopened after a
> cold-start of the modem. So I should not get into any issues
> with race conditions.
>
> > If you're not going to implement any control commands, I guess you
> > could conveniently not use the actual port, but keep it around, in
> > case you find use for it later. The advantage will be that older
> > kernels will work without any updates on newer devices.
>
> With the current usage pattern I have in mind, I'd rather add this
> feature later when/if needed. We can always add a new feature bit
> for this if we introduce the control channel later on.

OK.

> > > static void free_buf(struct port_buffer *buf)
> > > {
> > > - kfree(buf->buf);
> > > + unsigned long flags;
> > > +
> > > + if (!buf->dev) {
> > > + kfree(buf->buf);
> > > + goto freebuf;
> > > + }
> > > +
> > > + BUG_ON(!rproc_enabled);
> > > +
> > > + /* dma_free_coherent requires interrupts to be enabled */
> > > + if (rproc_enabled && !irqs_disabled()) {
> >
> > You don't need to check for rproc_enabled here.
>
> Actually I do need this check. The reason is that I am
> exploiting gcc's ability to discard dead code. When I compile
> for arch's that does not have DMA, this block is dead and will be
> discarded. This way I avoid the link error for the missing
> symbol dma_free_coherent(). But I can add a comment on this.

OK, I see. The BUG_ON would guarantee at run-time, but the
compile-time advantage wasn't obvious.

> > Then, you can just invert the if condition (if (irqs_disabled()) and
> > include the relevant block here. This way, you can make do without
> > the goto and return mess below.
>
> Yeah, I did an earlier version without goto, but I wanted to separate
> the rproc / non-rproc clearly to make it easier to see what happened
> if rproc was disabled. But I'll have a stab at refactoring this code
> again.

> > > @@ -485,7 +582,10 @@ static void reclaim_consumed_buffers(struct port
> > *port)
> > > return;
> > > }
> > > while ((buf = virtqueue_get_buf(port->out_vq, &len))) {
> > > - kfree(buf);
> > > + if (is_console_port(port))
> > > + kfree(buf);
> > > + else
> > > + free_buf(buf);
> >
> > Hm?
>
> See below.
>
> >
> > > port->outvq_full = false;
> > > }
> > > }
> > > @@ -498,6 +598,7 @@ static ssize_t send_buf(struct port *port, void
> > *in_buf, size_t in_count,
> > > ssize_t ret;
> > > unsigned long flags;
> > > unsigned int len;
> > > + struct port_buffer *buf = in_buf;
> >
> > This looks wrong: the buffer we receive here is the actual data
> > (buf->buf). It can never be a port_buffer (buf).
>
> See below.
>
> >
> > >
> > > out_vq = port->out_vq;
> > >
> > > @@ -505,8 +606,11 @@ static ssize_t send_buf(struct port *port, void
> > *in_buf, size_t in_count,
> > >
> > > reclaim_consumed_buffers(port);
> > >
> > > - sg_init_one(sg, in_buf, in_count);
> > > - ret = virtqueue_add_buf(out_vq, sg, 1, 0, in_buf, GFP_ATOMIC);
> > > + if (is_console_port(port))
> >
> > I think you're misinterpreting what is_console_port() is. It means if
> > a port is associated with an hvc/tty device.
>
> See below.
>
> >
> > > + sg_init_one(sg, in_buf, in_count);
> > > + else
> > > + sg_init_one(sg, buf->buf, in_count);
> > > + ret = virtqueue_add_buf(out_vq, sg, 1, 0, buf, GFP_ATOMIC);
> > >
> > > /* Tell Host to go! */
> > > virtqueue_kick(out_vq);
> > > @@ -669,7 +773,7 @@ static ssize_t port_fops_write(struct file *filp,
> > const char __user *ubuf,
> > > size_t count, loff_t *offp)
> > > {
> > > struct port *port;
> > > - char *buf;
> > > + struct port_buffer *buf;
> > > ssize_t ret;
> > > bool nonblock;
> > >
> > > @@ -696,11 +800,11 @@ static ssize_t port_fops_write(struct file
> > *filp, const char __user *ubuf,
> > >
> > > count = min((size_t)(32 * 1024), count);
> > >
> > > - buf = kmalloc(count, GFP_KERNEL);
> > > + buf = alloc_buf(port->out_vq, count);
> > > if (!buf)
> > > return -ENOMEM;
> > >
> > > - ret = copy_from_user(buf, ubuf, count);
> > > + ret = copy_from_user(buf->buf, ubuf, count);
> > > if (ret) {
> > > ret = -EFAULT;
> > > goto free_buf;
> > > @@ -720,7 +824,7 @@ static ssize_t port_fops_write(struct file *filp,
> > const char __user *ubuf,
> > > goto out;
> > >
> > > free_buf:
> > > - kfree(buf);
> > > + free_buf(buf);
> > > out:
> > > return ret;
> > > }
> >
> > OK, I now get what you did with send_buf() above. However, send_buf()
> > now should be completely broken for non-rproc devices: you're
> > allocating a buf instead of a buf->buf and passing that on to
> > send_buf() as a void*. You should instead modify send_buf() to accept
> > a struct port_buffer instead.
> >
> > Second, send_buf() receives a struct port_buffer(), but in the
> > 'is_console_port()' case, you ignore that fact, and just pass on the
> > void* pointer to sg_init_one(). You should instead pass buf->buf.
>
> OK, so the issue here it that currently put_chars() passes a
> char-buffer to send_buf() instead of a port_buffer. The tests above
> tries to handle this case, distingusing between a tty and char device.
> I agree that this is not the best solution.
>
> But if I change put_chars to create a port_buffer and copy
> data into it I can avoid the crap you pointed at above.

Yes, it's much easier if we don't have special cases in these generic
routines.

> ...
> > > - if (virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> > > + if (!is_rproc_serial(vdev) &&
> > > + virtio_has_feature(vdev, VIRTIO_CONSOLE_F_SIZE))
> > > hvc_resize(port->cons.hvc, port->cons.ws);
> >
> > Why do you want to ensure !is_rproc_serial() here? As long as the
> > device doesn't expose the VIRTIO_CONSOLE_F_SIZE feature, you should be
> > fine, so this hunk can be dropped.
>
> I need this test because virtio_check_driver_offered_feature() called
> from virtio_has_feature will throw a BUG() if you test on a feature
> not declared in the driver's feature-set.

Ah, OK.

> > > @@ -1102,10 +1209,10 @@ static unsigned int fill_queue(struct
> > virtqueue *vq, spinlock_t *lock)
> > >
> > > nr_added_bufs = 0;
> > > do {
> > > - buf = alloc_buf(PAGE_SIZE);
> > > + buf = alloc_buf(vq, PAGE_SIZE);
> > > if (!buf)
> > > break;
> > > -
> > > + memset(buf->buf, 0, PAGE_SIZE);
> >
> > Why this memset here?
> >
> > 1. alloc_buf() already does kzalloc()
>
> It used to do that, but not anymore. This patch
> changes kzalloc() to kmalloc() in alloc_buf()

Obviously I missed it :)

> > 2. Is there any specific reason you want the buffer to be zeroed?
> >
> > I've recently realised zeroing out the buffer before giving it to the
> > device serves no real purpose, and we're just slowing down the
> > allocation here, so I'm tempted to convert the kzalloc() to
> > kmalloc(), unless you have a specific need for zeroed pages.
>
> Agree, the only reason is that I did memset was not to change legacy
> behavior. I'd prefer to skip the memset too, so let's do that.

Can you please split that into a separate patch?

> > This hunk is already present in linux-next; rebasing over that should
> > get rid of it.
>
> Sure, I'll rebase next patch to linux-next and send a new patch tomorrow.

Thanks!

Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/