lkml.org 
[lkml]   [2012]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCHv2] virtio_console: Add support for remoteproc serial
    Date
    sjur.brandeland@stericsson.com writes:

    > From: Sjur Brændeland <sjur.brandeland@stericsson.com>
    >
    > Add a simple serial connection driver called
    > VIRTIO_ID_RPROC_SERIAL (0xB) for communicating with a
    > remote processor in an asymmetric multi-processing
    > configuration.
    >
    > 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.
    >
    > This enables use of the exising virtio_console code in
    > the remoteproc framework.

    OK, I'll let Amit comment on the console changes, but some minor style
    comments below:

    > +#if IS_ENABLED(CONFIG_REMOTEPROC)
    > +static inline bool is_rproc_serial(struct virtio_device *vdev)
    > +{
    > + return vdev->id.device == VIRTIO_ID_RPROC_SERIAL;
    > +}
    > +#else
    > +static inline bool is_rproc_serial(struct virtio_device *vdev)
    > +{
    > + return false;
    > +}
    > +#endif

    I prefer to avoid inline in C files. The compiler knows, and with
    inline you get no warning if it becomes unused. Also, const struct
    virtio_device *.

    > +/* Allocate data buffer from DMA memory if requested */
    > +static inline void *
    > +alloc_databuf(struct virtio_device *vdev, size_t size, gfp_t flag)
    > +{
    > + if (is_rproc_serial(vdev)) {
    > + dma_addr_t dma;
    > + struct device *dev = &vdev->dev;
    > + /*
    > + * Allocate DMA memory from ancestors. Finding the ancestor
    > + * is a bit quirky when DMA_MEMORY_INCLUDES_CHILDREN is not
    > + * implemented.
    > + */
    > + dev = dev->parent ? dev->parent : dev;
    > + dev = dev->parent ? dev->parent : dev;
    > + return dma_alloc_coherent(dev, size, &dma, flag);

    Wow, up 2 levels? Why 2? What's special about the grandparents?

    > -static void free_buf(struct port_buffer *buf)
    > +static void
    > +free_buf(struct virtqueue *vq, struct port_buffer *buf, size_t buf_size)
    > {

    Generally prefer to indent buf and buf_size, rather than break at
    free_buf.

    > + buf = alloc_databuf(vdev, buf_size, GFP_KERNEL);
    >
    > - buf = kmalloc(count, GFP_KERNEL);
    > if (!buf)
    > return -ENOMEM;

    This effectively adds a blank line between "buf = ..." and "if (!buf)",
    but they're adjacent because they're logically grouped.

    > @@ -767,6 +826,7 @@ static int port_fops_release(struct inode *inode, struct file *filp)
    > spin_unlock_irq(&port->inbuf_lock);
    >
    > spin_lock_irq(&port->outvq_lock);
    > +
    > reclaim_consumed_buffers(port);
    > spin_unlock_irq(&port->outvq_lock);
    >

    Weird whitespace addition. I know you're doing that simply to check if
    I'm reading, right?

    > @@ -1688,7 +1768,7 @@ static void remove_controlq_data(struct ports_device *portdev)
    > * config space to see how many ports the host has spawned. We
    > * initialize each port found.
    > */
    > -static int __devinit virtcons_probe(struct virtio_device *vdev)
    > +static int virtcons_probe(struct virtio_device *vdev)
    > {
    > struct ports_device *portdev;
    > int err;

    Not sure about this change. If you actually turn off CONFIG_HOTPLUG,
    I wouldn't think that remoteproc would work at all any more, since it
    needs the driver core to match up devices?

    > @@ -1724,10 +1804,12 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
    >
    > multiport = false;
    > portdev->config.max_nr_ports = 1;
    > - if (virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
    > - offsetof(struct virtio_console_config,
    > - max_nr_ports),
    > - &portdev->config.max_nr_ports) == 0)
    > + if (is_rproc_serial(vdev))
    > + multiport = false;
    > + else if (virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
    > + offsetof(struct virtio_console_config,
    > + max_nr_ports),
    > + &portdev->config.max_nr_ports) == 0)
    > multiport = true;

    This is a bit weird, to double-assign multiport = false; it looks tacked
    on.

    How about:

    /* Don't test MULTIPORT at all if we're rproc: not a valid feature! */
    if (!is_rproc_serial(vdev)
    && virtio_config_val(vdev, VIRTIO_CONSOLE_F_MULTIPORT,
    offsetof(struct virtio_console_config,
    max_nr_ports),
    &portdev->config.max_nr_ports) == 0) {
    multiport = true;
    } else {
    multiport = false;
    portdev->config.max_nr_ports = 1;
    }
    > err = init_vqs(portdev);
    > @@ -1838,6 +1920,16 @@ static unsigned int features[] = {
    > VIRTIO_CONSOLE_F_MULTIPORT,
    > };
    >
    > +static struct virtio_device_id rproc_serial_id_table[] = {
    > +#if IS_ENABLED(CONFIG_REMOTEPROC)
    > + { VIRTIO_ID_RPROC_SERIAL, VIRTIO_DEV_ANY_ID },
    > +#endif
    > + { 0 },
    > +};
    > +
    > +static unsigned int rproc_serial_features[] = {
    > +};
    > +
    > #ifdef CONFIG_PM
    > static int virtcons_freeze(struct virtio_device *vdev)
    > {
    > @@ -1922,6 +2014,16 @@ static struct virtio_driver virtio_console = {
    > #endif
    > };
    >
    > +static struct virtio_driver virtio_rproc_serial = {
    > + .feature_table = rproc_serial_features,
    > + .feature_table_size = ARRAY_SIZE(rproc_serial_features),
    > + .driver.name = "virtio_rproc_serial",
    > + .driver.owner = THIS_MODULE,
    > + .id_table = rproc_serial_id_table,
    > + .probe = virtcons_probe,
    > + .remove = virtcons_remove,
    > +};
    > +
    > static int __init init(void)
    > {
    > int err;
    > @@ -1941,12 +2043,16 @@ static int __init init(void)
    > INIT_LIST_HEAD(&pdrvdata.consoles);
    > INIT_LIST_HEAD(&pdrvdata.portdevs);
    >
    > - return register_virtio_driver(&virtio_console);
    > + err = register_virtio_driver(&virtio_console);
    > + if (err)
    > + return err;
    > + return register_virtio_driver(&virtio_rproc_serial);

    Hmm, we need to cleanup if the second register fails.

    > #define VIRTIO_ID_RPMSG 7 /* virtio remote processor messaging */
    > #define VIRTIO_ID_SCSI 8 /* virtio scsi */
    > #define VIRTIO_ID_9P 9 /* 9p virtio console */
    > +#define VIRTIO_ID_RPROC_SERIAL 0xB /* virtio remoteproc serial link */

    Prefer decimal here...

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

    \
     
     \ /
      Last update: 2012-09-20 03:01    [W:2.088 / U:0.500 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site