Re: [PATCH 6/9] Introduce virtio-testdev

[Christoffer Dall <christoffer.dall@linaro.org>]

On Thu, Oct 17, 2013 at 11:51:26AM +0200, Andrew Jones wrote:
> On Wed, Oct 16, 2013 at 06:06:14PM -0700, Christoffer Dall wrote:
> > There's an interesting mix of space and tab indentations in this file...
> > 
> > I assume using the kernel coding style would be approprate here...
> > Actually, that could be added to the readme too. (Or whichever format we
> > choose).
> > 
> > Could you fix?
> 
> I was attempting to adopt the style from the common code lib/*, see
> lib/string.c, lib/printf.c, lib/argv,c. The style used there is
> 
> 1. 'four spaces'
> 2. 'tab'
> 3. 'tab + four spaces'
> 4. 'tab + tab'
> ...

that sounds like the most horrible broken coding style I have ever heard
of.  At least it reads like sh*t in mutt.

> 
> However, I'd actually prefer to use the kernel style as well, and I see
> that not even those three common files are consistent. argv.c only uses
> spaces. Other x86 files also have a variety of styles (kernel style
> being one of them). So probably the best choice is to go with kernel
> style, and to add another cleanup patch that changes lib/* files as
> well.
> 
> > > +    for (i = 0; i < m->nr; ++i) {
> > > +	volatile u32 *base = (u32 *)compat_ptr(m->addrs[i]);
> > > +	u32 devid32 = base[VIRTIO_MMIO_DEVICEID / 4];

Yeah, I think it's is by far the easiest and most convenient for
everyone.

> > 
> > do we have readl/writel in this framework?  If not, maybe we should to
> > indicate IO read/writes and ensure the compiler doesn't reorder things
> > and that we have the necessary memory barriers etc...?
> > 
> > That would apply to virtio_testdev above as well.
> 
> OK, I agree it makes sense to add readl and writel in. I'll do that, and
> then rework my mmio reads to use them.
> 
> > +	printf("Refusing to run with virtio-testdev major = %d, minor = %d\n",
> > +			major, minor);
> 
> > "Refusing to run"?
> > How about "incompatible version of virtio-testdev"?
> 
> Sure. OK.
> 
> drew