[PATCH] libertas: fix spinlock recursion bug

[PATCH] libertas: fix spinlock recursion bug

[Holger Schurig]

This fixes a bug detected by CONFIG_DEBUG_SPINLOCK:

if_cs_get_int_status() is only called from lbs_thread(), via
priv->hw_get_int_status. However, lbs_thread() has already taken the
priv->driver_lock. So it's a fault to take the same lock again here.

Signed-off-by: Holger Schurig <hs4233@mail.mn-solutions.de>
CONFIG_DEBUG_SPINLOCK,
---

John, this patch might also go into linux-2.6.25-rcX

Index: wireless-testing/drivers/net/wireless/libertas/if_cs.c
===================================================================
--- wireless-testing.orig/drivers/net/wireless/libertas/if_cs.c	2008-03-19 13:46:47.000000000 +0100
+++ wireless-testing/drivers/net/wireless/libertas/if_cs.c	2008-03-19 13:46:53.000000000 +0100
@@ -677,9 +677,7 @@ sbi_get_int_status_exit:
 
 	/* Card has a command result for us */
 	if (*ireg & IF_CS_C_S_CMD_UPLD_RDY) {
-		spin_lock(&priv->driver_lock);
 		ret = if_cs_receive_cmdres(priv, priv->upld_buf, &priv->upld_len);
-		spin_unlock(&priv->driver_lock);
 		if (ret < 0)
 			lbs_pr_err("could not receive cmd from card\n");
 	}

Re: [PATCH] libertas: fix spinlock recursion bug

[Dan Williams]

On Wed, 2008-03-19 at 15:24 +0100, Holger Schurig wrote:
> This fixes a bug detected by CONFIG_DEBUG_SPINLOCK:
> 
> if_cs_get_int_status() is only called from lbs_thread(), via
> priv->hw_get_int_status. However, lbs_thread() has already taken the
> priv->driver_lock. So it's a fault to take the same lock again here.
> 
> Signed-off-by: Holger Schurig <hs4233@mail.mn-solutions.de>

Acked-by: Dan Williams <dcbw@redhat.com>

> CONFIG_DEBUG_SPINLOCK,
> ---
> 
> John, this patch might also go into linux-2.6.25-rcX
> 
> Index: wireless-testing/drivers/net/wireless/libertas/if_cs.c
> ===================================================================
> --- wireless-testing.orig/drivers/net/wireless/libertas/if_cs.c	2008-03-19 13:46:47.000000000 +0100
> +++ wireless-testing/drivers/net/wireless/libertas/if_cs.c	2008-03-19 13:46:53.000000000 +0100
> @@ -677,9 +677,7 @@ sbi_get_int_status_exit:
>  
>  	/* Card has a command result for us */
>  	if (*ireg & IF_CS_C_S_CMD_UPLD_RDY) {
> -		spin_lock(&priv->driver_lock);
>  		ret = if_cs_receive_cmdres(priv, priv->upld_buf, &priv->upld_len);
> -		spin_unlock(&priv->driver_lock);
>  		if (ret < 0)
>  			lbs_pr_err("could not receive cmd from card\n");
>  	}

Re: [PATCH] libertas: fix spinlock recursion bug

[Holger Schurig]

> Acked-by: Dan Williams <dcbw@redhat.com>

linville: ping

This patch is not yet in wireless-testing. :-(

Re: [PATCH] libertas: fix spinlock recursion bug

[Holger Schurig]

Hmm, that isn't the end of my spinlock problems. When I'm 
associated and ping, I get another recursion:

libertas assoc: associated to 00:13:19:80:da:30
BUG: spinlock recursion on CPU#0, lbs_main/5566
 lock: f58b58d8, .magic: dead4ead, .owner: 
lbs_main/5566, .owner_cpu: 0
Pid: 5566, comm: lbs_main Not tainted 2.6.25-rc5-wl #19
 [<c01ccdb7>] spin_bug+0x76/0xb1
 [<c01ccecb>] _raw_spin_lock+0x34/0xc9
 [<c0306984>] _spin_lock_irqsave+0x17/0x1c
 [<f8c33cdf>] lbs_hard_start_xmit+0x61/0x4e2 [libertas]
 [<c0306bef>] ? _spin_unlock+0x8/0x1f
 [<c014f497>] ? cache_alloc_refill+0x18c/0x439
 [<c02b449b>] dev_hard_start_xmit+0x16f/0x1dc
 [<c02c0f8b>] __qdisc_run+0x7a/0x14e
 [<c02b67ba>] dev_queue_xmit+0x119/0x23b
 [<c02bb187>] neigh_resolve_output+0x20a/0x236
 [<c02b978e>] neigh_update+0x2c0/0x36a
 [<c02c04e3>] ? eth_header_cache_update+0x0/0x12
 [<c02e4331>] arp_process+0x4d6/0x50e
 [<c02afc6d>] ? skb_release_data+0x84/0x89
 [<c02afb6d>] ? __kfree_skb+0x61/0x64
 [<c02e440c>] arp_rcv+0xa3/0xb3
 [<c02b4021>] netif_receive_skb+0x1f7/0x26a
 [<c02b643c>] process_backlog+0x5b/0xa5
 [<c02b5fa3>] net_rx_action+0x7f/0x142
 [<c011befe>] __do_softirq+0x38/0x70
 [<c011bf5b>] do_softirq+0x25/0x2a
 [<c02b5ea4>] netif_rx_ni+0x24/0x3e
 [<f8c33b59>] lbs_process_rxed_packet+0x755/0x7c0 [libertas]
 [<c02b024c>] ? __alloc_skb+0x4c/0xf8
 [<f88f9096>] if_cs_get_int_status+0x22a/0x3d8 [libertas_cs]
 [<f8c2ef5e>] lbs_thread+0x2fe/0x745 [libertas]
 [<c0114edd>] ? default_wake_function+0x0/0xd
 [<f8c2ec60>] ? lbs_thread+0x0/0x745 [libertas]
 [<c0126f91>] kthread+0x39/0x60
 [<c0126f58>] ? kthread+0x0/0x60
 [<c01043c3>] kernel_thread_helper+0x7/0x10
 =======================

Re: [PATCH] libertas: fix spinlock recursion bug

[Dan Williams]

On Wed, 2008-03-26 at 15:41 +0100, Holger Schurig wrote:
> Hmm, that isn't the end of my spinlock problems. When I'm 
> associated and ping, I get another recursion:

Hmm, so netif_rx() can eventually result in call to hard_start_xmit()
later down the stack?  Ugh.  It's probably a consequence of the driver
not poking the network stack in the right way to say "hey, I'm busy" or
not punting something to a workqueue or whatever.

I agree the indirection in the current SDIO and USB drivers sucks, but
I'm getting more and more convinced that the way that the CF driver is
handling this sucks too.

Dan

> libertas assoc: associated to 00:13:19:80:da:30
> BUG: spinlock recursion on CPU#0, lbs_main/5566
>  lock: f58b58d8, .magic: dead4ead, .owner: 
> lbs_main/5566, .owner_cpu: 0
> Pid: 5566, comm: lbs_main Not tainted 2.6.25-rc5-wl #19
>  [<c01ccdb7>] spin_bug+0x76/0xb1
>  [<c01ccecb>] _raw_spin_lock+0x34/0xc9
>  [<c0306984>] _spin_lock_irqsave+0x17/0x1c
>  [<f8c33cdf>] lbs_hard_start_xmit+0x61/0x4e2 [libertas]
>  [<c0306bef>] ? _spin_unlock+0x8/0x1f
>  [<c014f497>] ? cache_alloc_refill+0x18c/0x439
>  [<c02b449b>] dev_hard_start_xmit+0x16f/0x1dc
>  [<c02c0f8b>] __qdisc_run+0x7a/0x14e
>  [<c02b67ba>] dev_queue_xmit+0x119/0x23b
>  [<c02bb187>] neigh_resolve_output+0x20a/0x236
>  [<c02b978e>] neigh_update+0x2c0/0x36a
>  [<c02c04e3>] ? eth_header_cache_update+0x0/0x12
>  [<c02e4331>] arp_process+0x4d6/0x50e
>  [<c02afc6d>] ? skb_release_data+0x84/0x89
>  [<c02afb6d>] ? __kfree_skb+0x61/0x64
>  [<c02e440c>] arp_rcv+0xa3/0xb3
>  [<c02b4021>] netif_receive_skb+0x1f7/0x26a
>  [<c02b643c>] process_backlog+0x5b/0xa5
>  [<c02b5fa3>] net_rx_action+0x7f/0x142
>  [<c011befe>] __do_softirq+0x38/0x70
>  [<c011bf5b>] do_softirq+0x25/0x2a
>  [<c02b5ea4>] netif_rx_ni+0x24/0x3e
>  [<f8c33b59>] lbs_process_rxed_packet+0x755/0x7c0 [libertas]
>  [<c02b024c>] ? __alloc_skb+0x4c/0xf8
>  [<f88f9096>] if_cs_get_int_status+0x22a/0x3d8 [libertas_cs]
>  [<f8c2ef5e>] lbs_thread+0x2fe/0x745 [libertas]
>  [<c0114edd>] ? default_wake_function+0x0/0xd
>  [<f8c2ec60>] ? lbs_thread+0x0/0x745 [libertas]
>  [<c0126f91>] kthread+0x39/0x60
>  [<c0126f58>] ? kthread+0x0/0x60
>  [<c01043c3>] kernel_thread_helper+0x7/0x10
>  =======================
> 

Re: [PATCH] libertas: fix spinlock recursion bug

[Holger Schurig]

> I agree the indirection in the current SDIO and USB drivers
> sucks, but I'm getting more and more convinced that the way
> that the CF driver is handling this sucks too.

I don't really can say anything about CF/SDIO driver, but for me 
several things aren't coming into picture:

a) in "struct lbs_private", we have "struct mutex lock". What 
exactly does it lock?  Above it is a comment "/* protected with 
big lock */" but it's not clear to me to what this comment 
refers. Some lines below is the same comment, so maybe this 
should be a /* this variables are protected by 'lock' */" 
and "/* end of protection by 'lock' */".

b) some lines later there is a comment "/* command related 
variables protected by priv->driver_lock */", but some 
variables, like priv->seqnum, are command-related, but in 
the "struct mutex lock" section.

c) then there is the "spinlock_t driver_lock" later. Maybe this 
is what is meant to protect command-related things.

d) my knowledge about locks is so non-existant that I currently 
don't know why one lock is a "struct mutex" and the other is 
a "spinlock_t".

e) I also don't know (yet) when to use spin_lock_irq, 
spin_lock_irqsave or a plain spin_lock. However, I hope to fill 
this knowledge gap with 
http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/

But for clarification of the other points I'd need a helping 
hand :-)  Once we know which variables should be protected by 
which lock we can go and fix this thing. It might even be the 
case that one lock is enought.

Re: [PATCH] libertas: fix spinlock recursion bug

[Dan Williams]

On Wed, 2008-03-26 at 16:37 +0100, Holger Schurig wrote:
> > I agree the indirection in the current SDIO and USB drivers
> > sucks, but I'm getting more and more convinced that the way
> > that the CF driver is handling this sucks too.
> 
> I don't really can say anything about CF/SDIO driver, but for me 
> several things aren't coming into picture:
> 
> a) in "struct lbs_private", we have "struct mutex lock". What 
> exactly does it lock?  Above it is a comment "/* protected with 
> big lock */" but it's not clear to me to what this comment 
> refers. Some lines below is the same comment, so maybe this 
> should be a /* this variables are protected by 'lock' */" 
> and "/* end of protection by 'lock' */".
> 
> b) some lines later there is a comment "/* command related 
> variables protected by priv->driver_lock */", but some 
> variables, like priv->seqnum, are command-related, but in 
> the "struct mutex lock" section.
> 
> c) then there is the "spinlock_t driver_lock" later. Maybe this 
> is what is meant to protect command-related things.

Yes, ->driver_lock is for stuff that might touch hardware or require
commands to be issued and where you want to disable interrupts.  ->lock
is a simple mutex for protecting members of lbs_private from concurrent
accesses via WEXT or command responses.  spinlocks should only be held
for short periods because they spin, mutexes can be held for long
periods because they only sleep.  Any request from userspace like WEXT
should use a mutex (so the user process sleeps) while internally the
driver needs to use spinlocks if it needs to flip interrupts.  AFAIK.

> d) my knowledge about locks is so non-existant that I currently 
> don't know why one lock is a "struct mutex" and the other is 
> a "spinlock_t".
> 
> e) I also don't know (yet) when to use spin_lock_irq, 
> spin_lock_irqsave or a plain spin_lock. However, I hope to fill 
> this knowledge gap with 
> http://www.kernel.org/pub/linux/kernel/people/rusty/kernel-locking/
> 
> But for clarification of the other points I'd need a helping 
> hand :-)  Once we know which variables should be protected by 
> which lock we can go and fix this thing. It might even be the 
> case that one lock is enought.

It's a rat's nest only in the CF driver because the CF driver attempts
to do a s**tload of work in the hw_get_int_status() callback, which was
never meant to do any work at all, and is also under the spinlock for
reasons described below.

So; the main thread sleeps for a while, and will be woken up by the
interface code when something interesting happens (interrupt, new TX
packets from the network stack, firmware events) via lbs_interrupt().
The first thing the main thread does is ask the interface code what,
exactly, it wants the main thread to do.

The main thread grabs the lock for this because the status/event
variables need synchronization.  The main thread needs to read the
interface code's status, but the interface code could be in the
interrupt handler or whatever at the same time, and therefore might
update card->int_cause underneath the main thread if there was no
locking.  Ideally the interface code would, I don't know, atomically
queue up some work or something in the main thread and then wake the
main thread up, and then the thread would already have the interrupt
status and wouldn't need to call back into the interface code to find
out.

Next, if the interface code has some received command responses
(indicated by setting the MRVDRV_CMD_UPLD_RDY bit in it's int_cause
which got returned by hw_get_int_status() call just above), the main
thread will process command responses from the firmware.

Next, if the firmware has raised some event (disassociation, MIC
failures, etc) the main thread will (still under the spinlock!!) ask the
interface code what the firmware raised, and then process that event.

The main thread will then send queued commands down to the interface
code and thus out to the firmware, and then goes back to sleep waiting
for some interrupt/event from the interface code.

So there _definitely_ has to be some synchronization here between the
main thread and the interface code, because the interface code could be
executing in a different context due to interrupts or whatever.  When
the interface code has something interesting for the main thread to do,
it wakes the main thread up.

-----------------------------------------

The objections to the current system are basically that:

1) there's too many stupid callbacks into the interface code;
hw_get_int_status() and hw_get_event_cause() must die because they are
ugly

2) if events come in fast enough, they will get lost because the
interface code only stores the last event in card->event; status
(CARDEVENT, CMD_UPLD_RDY) is OK because it's a bitfield and only ever
OR-ed or selectively cleared

3) the code to handle all of these is just ugly; each card has to hold
onto an 'int_cause' and an 'event', the main thread keeps track of
priv->intcounter, priv->hisregcpy, and priv->eventcause.

4) SBI_EVENT_CAUSE_SHIFT... WTF?  Need I say more?  There's a better
way.

-------------------------------


To start fixing this, lets go back to the basic requirements:

a) the interface code needs to push events to the main thread
b) the interface code needs to push command responses to the main thread
c) the interface code needs to wake up the main thread when (a) or (b)
happens

The way that airo, orinoco, and atmel deal with this issue is to have a
common interrupt handler in the main code that:

1) disables interrupts
2) grabs a driver lock
3) reads interrupt status and event causes from the card
4) processes events and pushes long-running stuff to workqueues or a
main thread like libertas has in the case of airo


Possible fix:
=========================

At the expense of a some memory (solution for that at the end), create a
new structure:

struct lbs_event {
	u32 event;  /* MACREG_INT_CODE_xxxxx */
	u32 len;
	u8 buf[LBS_UPLD_SIZE];
};

then in lbs_private, add:

    struct list_head event_list;
    struct list_head event_free_list;

and pre-fill the free list with ~5 or 10 lbs_event structures.  When the
interface code gets _either_ an event or a new command response, it
calls:

    lbs_interrupt(priv, event, resp_data, resp_len);

lbs_interrupt() then gets rewritten to do something like:

void lbs_interrupt(struct lbs_private *priv, u32 event, u8 *resp_data, u32 resp_len)
{
    unsigned long flags;

    lbs_deb_enter(LBS_DEB_THREAD);

    spin_lock_irqsave (&priv->driver_lock, &flags);

    <grab event struct 'next' off event_free_list>
    if (< no free events>) {
        lbs_pr_alert("event/response queue full!");
        /* do something intelligent */
        goto out;
    }

    next.event = event;
    next.len = resp_len;
    if (resp_len)
        memcpy (next.buf, resp_data, resp_len);

    <add event struct 'next' to tail of event_list>

    if (priv->psstate == PS_STATE_SLEEP)
        priv->psstate = PS_STATE_AWAKE;

    wake_up_interruptible(&priv->waitq);

out:
    spin_unlock_irqrestore (&priv->driver_lock, flags);
    lbs_deb_leave(LBS_DEB_THREAD);
}

then, the main thread can get rid of:

priv->upld_len
priv->upld_buf
hw_get_int_status
hw_read_event_cause
priv->intcounter
priv->eventcause
priv->hisregcpy

and in lbs_thread() right after the "if (priv->surpriseremoved)" bits,
the flow would go like:

  process:
    event = <get first event from priv->event_list>
    if (event->len) {
        spin_unlock_irqrestore(&priv->driver_lock, flags);
	lbs_process_rx_command(priv, event->buf, event->len);
        spin_lock_irqsave(&priv->driver_lock, &flags);
    }

    .... command timeout stuff ....

    if (event->event) {
        spin_unlock_irq(&priv->driver_lock);
        lbs_process_event(priv, event->event);
    }

    .... blah blah blah ....

    < loop to process: until event_list is empty, then sleep >

Seems better than what's there.  I might take a stab at this if people
think it seems sane enough.  The memory hit over what's there now would
be ~15K of allocated memory at driver load per card.  If that's too
much, this could be split into two lists, an event queue and a smaller
command response queue since there should never be more than a few
outstanding command responses, but there might be a lot of events.  You
want to preallocate the list because you don't really want to be
kzalloc()-ing during interrupt handlers and dealing with possible
failures.

Dan

Re: [PATCH] libertas: fix spinlock recursion bug

[Holger Schurig]

Hi Dan !

I like most of your proposition. A few things:


> struct lbs_event {
> 	u32 event;  /* MACREG_INT_CODE_xxxxx */
> 	u32 len;
> 	u8 buf[LBS_UPLD_SIZE];
> };

Ugh, as LBS_UPLD_SIZE equals to to 2312, this looks ugly. I'm not 
sure if accessing the cmd_ctrl_node list directly would be a 
good idea, thought.


> Seems better than what's there.

Definitely. Maybe David Woodhouse can give his opinion as well.


> I might take a stab at this if people think it seems sane
> enough.

Do you have time for such a thing?  I see that you don't ack some 
of my patches, probably because you don't even have time to 
check them. What time frame do you have in mind?


> The memory hit over what's there now would be ~15K of allocated
> memory at driver load per card.

The memory hit won't be nice for embedded devices. But maybe it's 
better to try this solution first, to see if it's working. Then 
we have all the time for optimizations, e.g. with two lists.

Re: [PATCH] libertas: fix spinlock recursion bug

[Holger Schurig]

> struct lbs_event {

add "struct list_head list;"

> 	u32 event;  /* MACREG_INT_CODE_xxxxx */
> 	u32 len;
> 	u8 buf[LBS_UPLD_SIZE];
> };

Hmm, buf is LBS_UPLD_SIZE (2312 bytes). However, if_cs.c, 
if_sdio.c and if_usb.c use LBS_CMD_BUFFER_SIZE, which is 2048.


> void lbs_interrupt(struct lbs_private *priv, u32 event, u8
> *resp_data, u32 resp_len) {

With this approach, the driver would have copy the date from the 
hardware into local buffer. Then it would call lbs_interrupt() 
with a pointer to this local buffer. Then lbs_interrupt() would 
memcpy() this. Then something in main.c and/or cmd.c would again 
memcpy() this.

Let's split lbs_interrupt() into two functions:

struct lbs_event *lbs_get_free_event(struct lbs_private *priv);

void lbs_handle_event(struct lbs_private *priv, struct lbs_event 
*event);

Then the hardware driver can do:

  struct lbs_event *event = lbs_get_free_event(priv);
  if (!event)
     ...
  if_cs_receive_cmdres(priv, &event->buf, &event->len);
  lbs_handle_event(priv, event);

Re: [PATCH] libertas: fix spinlock recursion bug

[Dan Williams]

On Thu, 2008-03-27 at 11:08 +0100, Holger Schurig wrote:
> > struct lbs_event {
> 
> add "struct list_head list;"
> 
> > 	u32 event;  /* MACREG_INT_CODE_xxxxx */
> > 	u32 len;
> > 	u8 buf[LBS_UPLD_SIZE];
> > };
> 
> Hmm, buf is LBS_UPLD_SIZE (2312 bytes). However, if_cs.c, 
> if_sdio.c and if_usb.c use LBS_CMD_BUFFER_SIZE, which is 2048.
> 
> 
> > void lbs_interrupt(struct lbs_private *priv, u32 event, u8
> > *resp_data, u32 resp_len) {
> 
> With this approach, the driver would have copy the date from the 
> hardware into local buffer. Then it would call lbs_interrupt() 
> with a pointer to this local buffer. Then lbs_interrupt() would 
> memcpy() this. Then something in main.c and/or cmd.c would again 
> memcpy() this.

The USB and SDIO driver have skbs that they currently just memcpy to
priv->upld_buf, in this case lbs_interrupt() would take over that
responsibility, so you'd still have the same # of copies: 1.

It looks like the CF driver uses priv->upld_buf directly, which it can
do because it's holding the spinlock, because it's trying to do so much
work from hw_get_int_status(), which is something it shouldn't be doing.
I'd argue that the CF driver should also have an internal buffer like
the SDIO and USB drivers do, and then there's still only one copy.  

The problem with using upld_buf (and therefore zero-copy in the CF
driver) is that you have no idea how long a read from the card will
really take, and what errors might happen during the read from the card.
And you're holding the spinlock with interrupts disabled while you're
doing that.  That read from the card is going to take a lot more time
than a simple memcpy in lbs_interrupt(), and you want to minimize the
time you're holding the spinlock anyway.  That's the reason the CF
driver has this problem in the first place, because it's trying to do
too much under the spinlock.

Dan

> Let's split lbs_interrupt() into two functions:
> 
> struct lbs_event *lbs_get_free_event(struct lbs_private *priv);
> 
> void lbs_handle_event(struct lbs_private *priv, struct lbs_event 
> *event);
> 
> Then the hardware driver can do:
> 
>   struct lbs_event *event = lbs_get_free_event(priv);
>   if (!event)
>      ...
>   if_cs_receive_cmdres(priv, &event->buf, &event->len);
>   lbs_handle_event(priv, event);

Re: [PATCH] libertas: fix spinlock recursion bug

[Holger Schurig]

> The problem with using upld_buf (and therefore zero-copy in
> the CF driver) is that you have no idea how long a read from
> the card will really take, and what errors might happen during
> the read from the card.

Yes, this can take a long time. And, BTW, I don't want to use 
upld_buf.


My idea is this:

> > struct lbs_event *lbs_get_free_event(struct lbs_private
> > *priv);

This will take out an lbs_event from the event_free_list and 
return it.
Taking out has to be done under spinlock, but this is very fast.

Once the lbs_event is no longer in any of the two event list, 
we "own" it and can handle it as long as we want, without the 
need of an additional spinlock.

Then

> > void lbs_handle_event(struct lbs_private *priv, struct
> > lbs_event *event);

would again take the spinlock, insert the event in the 
event_list, and release the spinlock. Which again takes only a 
short time.

This approach can make receiption of command responses from card 
zero-copy like, at least insofar the event processing is 
considered. 



> I'd argue that the CF driver should also have an internal
> buffer like the SDIO and USB drivers do, and then there's still
> only one copy.

Yeah, but I can reduce easily to a 0-copy-scheme.


I hope all of this sounds sane.

Re: [PATCH] libertas: fix spinlock recursion bug

[Dan Williams]

On Thu, 2008-03-27 at 12:57 +0100, Holger Schurig wrote:
> > The problem with using upld_buf (and therefore zero-copy in
> > the CF driver) is that you have no idea how long a read from
> > the card will really take, and what errors might happen during
> > the read from the card.
> 
> Yes, this can take a long time. And, BTW, I don't want to use 
> upld_buf.
> 
> 
> My idea is this:
> 
> > > struct lbs_event *lbs_get_free_event(struct lbs_private
> > > *priv);
> 
> This will take out an lbs_event from the event_free_list and 
> return it.
> Taking out has to be done under spinlock, but this is very fast.
> 
> Once the lbs_event is no longer in any of the two event list, 
> we "own" it and can handle it as long as we want, without the 
> need of an additional spinlock.
> 
> Then
> 
> > > void lbs_handle_event(struct lbs_private *priv, struct
> > > lbs_event *event);
> 
> would again take the spinlock, insert the event in the 
> event_list, and release the spinlock. Which again takes only a 
> short time.

well, if this is being done in lbs_thread() then the call from
lbs_get_free_event() is going to be right above lbs_handle_event()
anyway, so you might as well hold the spinlock over both, right?  I'm
just basing that assumption off the existing structure of lbs_thread(),
maybe you've re-ordered it a lot or something.

But it sounds OK.

Dan

> This approach can make receiption of command responses from card 
> zero-copy like, at least insofar the event processing is 
> considered. 
> 
> 
> 
> > I'd argue that the CF driver should also have an internal
> > buffer like the SDIO and USB drivers do, and then there's still
> > only one copy.
> 
> Yeah, but I can reduce easily to a 0-copy-scheme.
> 
> 
> I hope all of this sounds sane.
>