* Sasha Levin <[email protected]> wrote:
> +#define PCI_VENDOR_ID_REDHAT_QUMRANET 0x1af4
> +#define PCI_DEVICE_ID_VIRTIO_RNG 0x1004
> +#define PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET 0x1af4
> +#define PCI_SUBSYSTEM_ID_VIRTIO_RNG 0x0004
> +#define PCI_VIRTIO_RNG_DEVNUM 4
> +
> +#define VIRTIO_RNG_IRQ 11
> +#define VIRTIO_RNG_PIN 1
> +
> +#define NUM_VIRT_QUEUES 1
> +
> +#define VIRTIO_RNG_QUEUE_SIZE 128
> +
> +struct rng_device {
> + uint8_t status;
> + uint16_t config_vector;
> + int fd_rng;
> +
> + /* virtio queue */
> + uint16_t queue_selector;
> + struct virt_queue vqs[NUM_VIRT_QUEUES];
> + void *jobs[NUM_VIRT_QUEUES];
> +};
Really, have you *looked* at this source code from a distance? Does it look
neat and orderly to you??
It does not look readable to me at all: it's full of vertical spacing
misalignments even within the *same* syntactic unit. (Not to mention file-scope
alignment convention which is all over the place.)
> +static struct ioport_operations virtio_rng_io_ops = {
> + .io_in = virtio_rng_pci_io_in,
> + .io_out = virtio_rng_pci_io_out,
> +};
> +
> +static struct pci_device_header virtio_rng_pci_device = {
> + .vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET,
> + .device_id = PCI_DEVICE_ID_VIRTIO_RNG,
> + .header_type = PCI_HEADER_TYPE_NORMAL,
> + .revision_id = 0,
> + .class = 0x010000,
> + .subsys_vendor_id = PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET,
> + .subsys_id = PCI_SUBSYSTEM_ID_VIRTIO_RNG,
> + .bar[0] = IOPORT_VIRTIO_RNG |
> PCI_BASE_ADDRESS_SPACE_IO,
> + .irq_pin = VIRTIO_RNG_PIN,
> + .irq_line = VIRTIO_RNG_IRQ,
> +};
Same here! It looks like as if random new lines were jumbled within something
copy & pasted from elsewhere, with no care taken that they look good together
...
There's also other details like:
> + int fd_rng;
The _rng postfix is superfluous, we already know that this is a rng thing, it's
within struct rng_device! The result is suboptimal usage like this:
rng_device.fd_rng
which says 'rng' twice and clutters the code needlessly.
Plus remember the blk_device argument i made yesterday? The *same* issue gets
reintroduced here:
static struct rng_device rng_device;
We generally try to use different names for the structure and the local (or as
here, global) variables - and we try to use *short* (while still expressive)
names for variables.
So the proper and canonical naming, in line with blk_dev and net_dev would be
rng_dev, not rng_device.
The code looks correct but we really need to try harder to keep the tools/kvm/
code maintainable!
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
