commit 4e5518ca53be29c1ec3c00089c97bef36bfed515 upstream.
pcmcia_request_irq() and pcmcia_enable_device() are intended
to be called from process context (first function allocate memory
with GFP_KERNEL, second take a mutex). We can not take spin lock
and call them.
It's safe to move spin lock after pcmcia_enable_device() as we
still hold off IRQ until dev->base_addr is 0 and driver will
not proceed with interrupts when is not ready.
Patch resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=643758
Reported-and-tested-by: [email protected]
Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/hostap/hostap_cs.c | 10 ++--------
1 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/hostap/hostap_cs.c b/drivers/net/wireless/hostap/hostap_cs.c
index b4ff1dc..6992f8f 100644
--- a/drivers/net/wireless/hostap/hostap_cs.c
+++ b/drivers/net/wireless/hostap/hostap_cs.c
@@ -662,12 +662,6 @@ static int prism2_config(struct pcmcia_device *link)
link->dev_node = &hw_priv->node;
/*
- * Make sure the IRQ handler cannot proceed until at least
- * dev->base_addr is initialized.
- */
- spin_lock_irqsave(&local->irq_init_lock, flags);
-
- /*
* Allocate an interrupt line. Note that this does not assign a
* handler to the interrupt, unless the 'Handler' member of the
* irq structure is initialized.
@@ -690,9 +684,10 @@ static int prism2_config(struct pcmcia_device *link)
CS_CHECK(RequestConfiguration,
pcmcia_request_configuration(link, &link->conf));
+ /* IRQ handler cannot proceed until at dev->base_addr is initialized */
+ spin_lock_irqsave(&local->irq_init_lock, flags);
dev->irq = link->irq.AssignedIRQ;
dev->base_addr = link->io.BasePort1;
-
spin_unlock_irqrestore(&local->irq_init_lock, flags);
/* Finally, report what we've done */
@@ -724,7 +719,6 @@ static int prism2_config(struct pcmcia_device *link)
return ret;
cs_failed:
- spin_unlock_irqrestore(&local->irq_init_lock, flags);
cs_error(link, last_fn, last_ret);
failed:
--
1.7.1
On Tue, Jan 18, 2011 at 02:10:18PM -0700, Tim Gardner wrote:
> I don't know either, but this device is becoming rare enough that
> I'm not gonna lose any sleep over it.
Ok, so defer any change since we see a bug report (I hope
we do not see, patch was tested by 2 users, it fix bug
and device work for them).
Stanislaw
On 01/17/2011 06:05 AM, Stanislaw Gruszka wrote:
> commit 4e5518ca53be29c1ec3c00089c97bef36bfed515 upstream.
>
> pcmcia_request_irq() and pcmcia_enable_device() are intended
> to be called from process context (first function allocate memory
> with GFP_KERNEL, second take a mutex). We can not take spin lock
> and call them.
>
> It's safe to move spin lock after pcmcia_enable_device() as we
> still hold off IRQ until dev->base_addr is 0 and driver will
> not proceed with interrupts when is not ready.
>
> Patch resolves:
> https://bugzilla.redhat.com/show_bug.cgi?id=643758
>
> Reported-and-tested-by: [email protected]
> Signed-off-by: Stanislaw Gruszka<[email protected]>
> ---
> drivers/net/wireless/hostap/hostap_cs.c | 10 ++--------
> 1 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/hostap/hostap_cs.c b/drivers/net/wireless/hostap/hostap_cs.c
> index b4ff1dc..6992f8f 100644
> --- a/drivers/net/wireless/hostap/hostap_cs.c
> +++ b/drivers/net/wireless/hostap/hostap_cs.c
> @@ -662,12 +662,6 @@ static int prism2_config(struct pcmcia_device *link)
> link->dev_node =&hw_priv->node;
>
> /*
> - * Make sure the IRQ handler cannot proceed until at least
> - * dev->base_addr is initialized.
> - */
> - spin_lock_irqsave(&local->irq_init_lock, flags);
> -
> - /*
> * Allocate an interrupt line. Note that this does not assign a
> * handler to the interrupt, unless the 'Handler' member of the
> * irq structure is initialized.
> @@ -690,9 +684,10 @@ static int prism2_config(struct pcmcia_device *link)
> CS_CHECK(RequestConfiguration,
> pcmcia_request_configuration(link,&link->conf));
>
> + /* IRQ handler cannot proceed until at dev->base_addr is initialized */
> + spin_lock_irqsave(&local->irq_init_lock, flags);
> dev->irq = link->irq.AssignedIRQ;
> dev->base_addr = link->io.BasePort1;
> -
> spin_unlock_irqrestore(&local->irq_init_lock, flags);
>
> /* Finally, report what we've done */
> @@ -724,7 +719,6 @@ static int prism2_config(struct pcmcia_device *link)
> return ret;
>
> cs_failed:
> - spin_unlock_irqrestore(&local->irq_init_lock, flags);
> cs_error(link, last_fn, last_ret);
>
> failed:
Yes - I think this patch is correct. I didn't drill deep enough to
notice the GFP_KERNEL memory allocation. However, I think there is still
a problem with the interrupt handler which will only be noticed if there
is another active device on the same shared interrupt. Shouldn't it
return IRQ_NONE? See attached.
rtg
--
Tim Gardner [email protected]
On Tue, Jan 18, 2011 at 07:49:33AM -0700, Tim Gardner wrote:
> Yes - I think this patch is correct. I didn't drill deep enough to
> notice the GFP_KERNEL memory allocation. However, I think there is
> still a problem with the interrupt handler which will only be
> noticed if there is another active device on the same shared
> interrupt. Shouldn't it return IRQ_NONE? See attached.
I'm not sure. I think kernel could disable interrupt line when IRQ_NONE
is returned, but line is not shared.
Generally hostap pcmcia initialization procedure does not look correct.
It should be rahter rearranged to request irq when we are ready to
receive it, like that:
ret = pcmcia_enable_device(link);
if (ret)
goto failed;
dev->irq = link->irq;
dev->base_addr = link->resource[0]->start;
ret = pcmcia_request_irq(link, prism2_interrupt);
if (ret)
goto failed;
However I'm not sure if pcmcia_enable_device() does not require
to have pcmcia_request_irq() before?
Stanislaw
On 01/18/2011 08:43 AM, Stanislaw Gruszka wrote:
> On Tue, Jan 18, 2011 at 07:49:33AM -0700, Tim Gardner wrote:
>> Yes - I think this patch is correct. I didn't drill deep enough to
>> notice the GFP_KERNEL memory allocation. However, I think there is
>> still a problem with the interrupt handler which will only be
>> noticed if there is another active device on the same shared
>> interrupt. Shouldn't it return IRQ_NONE? See attached.
>
> I'm not sure. I think kernel could disable interrupt line when IRQ_NONE
> is returned, but line is not shared.
>
I believe it is a shared IRQ, but its been so long since I've worked
with the PCMCIA version of the prism device that I can't remember for
sure. The PCI flavor definitely requests a shared IRQ, and both PCI and
PCMCIA use the same interrupt handler function.
> Generally hostap pcmcia initialization procedure does not look correct.
> It should be rahter rearranged to request irq when we are ready to
> receive it, like that:
>
> ret = pcmcia_enable_device(link);
> if (ret)
> goto failed;
>
> dev->irq = link->irq;
> dev->base_addr = link->resource[0]->start;
>
> ret = pcmcia_request_irq(link, prism2_interrupt);
> if (ret)
> goto failed;
>
> However I'm not sure if pcmcia_enable_device() does not require
> to have pcmcia_request_irq() before?
>
I don't know either, but this device is becoming rare enough that I'm
not gonna lose any sleep over it.
rtg
--
Tim Gardner [email protected]