Re: [PATCH 2/9] kvm tools: Add basic ioport dynamic allocation

Ingo Molnar
2011-05-25T14:34:48Z

* Sasha Levin <levinsasha928@gmail.com> wrote:

> +u16 ioport__find_free_range(void)
> +{
> + static u16 cur_loc;

Please don't put statics inside function bodies! (even if they are
only used within a single function)

I had to look three times to discover that it's really a global
variable. These should be where other global variables are, or should
be put before the function, in plain sight.

Also, if you do that i'd suggest a more descriptive name -
free_io_port_idx perhaps?

> + return IOPORT_START + (cur_loc++ * IOPORT_SIZE);

So this is SMP unsafe really. While ioport registrations are
currently only used from initdev() functions and are thus serialized,
it's not completely unfeasible that we would want to have async
initcalls like the kernel does, to improve bootup/startup
performance!

So it would be nice to make this all SMP safe. A single mutex would
suffice i suspect.

Thanks,

Ingo