Re: [PATCH 5/8] [RFC] qlge: Add main source module qlge_main.c

Hi Franco,

I have cleaned up a lot of the code per your suggestions in this thread and will
do another submission this afternoon.
Regarding the delay/sleep, I've changed from mdelay to udelay and reduced the loop
into the 10s of microseconds instead of the hundreds of milliseconds.
I found out the milliseconds was intended for an emulated test
environment and not for the silicon.  Thanks again for bringing this to
my attention.

Regards,
Ron Mercer


On Fri, Sep 12, 2008 at 10:15:36AM +0200, Franco Fichtner wrote:

Hi Ron,

Thanks for the clarification. 4000 lines of code are just too much to
grasp at the first glance. :)

quoted

Regarding ql_wait_idx_reg() and ql_wait_addr_reg() they provide waiting
while gaining access for read/writes to/from a secondary set of
registers.

These registers are once per hardware, right? Then your spinlocking
would do nothing on single CPU systems, because it won't get into the
binaries. Meaning: your registers are NOT protected the way you would
have hoped. And even with SMP systems... if another CPU hits a locked
spinlock it does nothing but wait, quite synchronously to your mdelay()
loop. So both scenarios are rather suboptimal. If that's how things are,
you're better off using a semaphore.

On non-SMP systems, you're gonna put another access on the waiting list
by using a semaphore, which in return will give you time to finish your
operations on the registers and free the semaphore. And, last but not
least, SMP systems will do work they otherwise would not be able to do.

There is just the slight problem of ql_wait_idx_reg() and
ql_wait_addr_reg() being executed from interrupt context? If that is the
case, then you can't use the standard semaphores, because sleeping isn't
allowed. I do hope that is not the case...

quoted

To my understanding, you can't sleep while holding a
spinlock.  I tried this before and saw massive warnings.  I have
"Spinlock debugging: sleep-inside-spinlock checking" turned on in my
kernel and perhaps that's why I saw them.

You'd normally use spinlocks in interrupt context where you just have to
make sure no other CPU mangles with your variables or hardware registers
the fastest way possible. You don't keep a spinlock forever, not even
for the course of a few function calls, which will make the code harder
to read, thus harder to understand, and thus harder to debug.

The warning messages are right, but (I think) they are intended to avoid 
deadlocks as early as possible. Though, with msleep() I don't think
you'll get into any more serious problems than before, because unless 
the Kernel is really not doing what it should, you should always get
back to your loop. Using schedule() and wait queues in spinlocked 
scenarios is a whole other thing. (Anyone, please correct me if I'm wrong.)

quoted

At that point I changed from msleep to mdelay.
After reviewing your comments I've reduced and standardized the mdelay
wait to 5ms with a count of 50.

It may help introduce defines to get rid of magic numbers inside the
code as well. Something like:

#define QLGE_TIMEOUT_COUNT	50
#define QLGE_TIMEOUT_DELAY	5

That will help maintaining code and makes it more easily readable (and
consistent as a bonus... less lines to update).

quoted

These secondary registers are
accessed only during open(), reset(), and a few
netdev-API like set_multi and add_vlan_vid.

Okay, I see. But still, I get the vibe that this is already a bit too
much to justify keeping the current way of locking in the code. Sooner
or later someone else will complain about this. ;)


Franco