| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <871uhxplvb.fsf@rustcorp.com.au>
Date: Thu, 20 Sep 2012 10:10:39 +0930
From: Rusty Russell <rusty@...tcorp.com.au>
To: sjur.brandeland@...ricsson.com
Cc: Sjur Brændeland <sjurbren@...il.com>,
linux-kernel@...r.kernel.org,
Linus Walleij <linus.walleij@...aro.org>,
virtualization@...ts.linux-foundation.org,
Sjur Brændeland <sjur.brandeland@...ricsson.com>,
"Michael S. Tsirkin" <mst@...hat.com>,
Amit Shah <amit.shah@...hat.com>,
Ohad Ben-Cohen <ohad@...ery.com>
Subject: Re: [PATCHv2] virtio_console: Add support for remoteproc serial
sjur.brandeland@...ricsson.com writes:
> From: Sjur Brændeland <sjur.brandeland@...ricsson.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists