On Wed, 18 May 2016, Sell, Timothy C wrote:
> We have a bus driver currently in drivers/staging/unisys/visorbus/ that
> we are trying to get out of staging and into the kernel proper. Since
> "visorbus" is a driver to host a virtual bus presented to a Linux guest
> in a hypervisor environment (refer to
> drivers/staging/unisys/Documentation/overview.txt for more details),
> Greg KH and Jes Sorensen have suggested the possibility that drivers/virt/
> might be a good place for visorbus. But right now, we see that the only
> driver under drivers/virt/ is the Freescale hypervisor environment, which
> made us wonder whether this was really the correct place.
>
> Would you have any guidance for us?
> Our intent is to push our visorbus out of staging immediately following
> the current merge window.
What's the problem with Gregs and Jes suggestion? I don't see any.
There is bigger fish to fry than the final place of this driver. I had just a
peek at the staging code and there is enough stuff which wants to be cleaned
up before moving anywhere. I don't have time to do a proper review now, but
here are a few hints upfront:
1) Locking:
visordriver_callback_lock:
That should be a mutex, not a semaphore
periodic_work->lock:
Why is this a rw_lock if it's only locked with write_lock? And what's
the purpose of this lock at all?
2) Memory barriers:
Completely undocumented wmb()s without corresponding rmb()s to do obscure
protection of that periodic work stuff.
3) periodic_work:
That set of functions is obscure. Especially visor_periodic_work_stop()
makes me shudder. See also #2.
That work->lock does not inspire my confidence further.
4) Exports:
A gazillion of exports which are just wrappers around another set of
exports
5) Function comments:
Try to mimic kerneldoc comments, i.e. start with: /**
but do not implement any of the kerneldoc requirements.
I'll try do find a time slot for a proper review of that thing, but don't
expect that to happen in the next days.
Thanks,
tglx
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
