[PATCH 2.6.32.y] hostap_cs: fix sleeping function called from invalid context

[PATCH 2.6.32.y] hostap_cs: fix sleeping function called from invalid context

[Stanislaw Gruszka]

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: rbugz@biobind.com
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 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

Re: [PATCH 2.6.32.y] hostap_cs: fix sleeping function called from invalid context

[Tim Gardner]

[-- Attachment #1: Type: text/plain, Size: 2577 bytes --]

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: rbugz@biobind.com
> Signed-off-by: Stanislaw Gruszka<sgruszka@redhat.com>
> ---
>   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 tim.gardner@canonical.com

[-- Attachment #2: 0001-hostap-Return-IRQ_NONE-if-the-device-has-not-been-co.patch --]
[-- Type: text/x-patch, Size: 888 bytes --]

>From 638d3ea93e2a9cccb860d9e31c84e5d3a3eb38bd Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@canonical.com>
Date: Tue, 18 Jan 2011 07:47:45 -0700
Subject: [PATCH] 2.6.32.y, hostap: Return IRQ_NONE if the device has not been configured

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 drivers/net/wireless/hostap/hostap_hw.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/hostap/hostap_hw.c b/drivers/net/wireless/hostap/hostap_hw.c
index 8721850..789a503 100644
--- a/drivers/net/wireless/hostap/hostap_hw.c
+++ b/drivers/net/wireless/hostap/hostap_hw.c
@@ -2624,7 +2624,7 @@ static irqreturn_t prism2_interrupt(int irq, void *dev_id)
 			printk(KERN_DEBUG "%s: Interrupt, but dev not configured\n",
 			       dev->name);
 		}
-		return IRQ_HANDLED;
+		return IRQ_NONE;
 	}
 
 	iface = netdev_priv(dev);
-- 
1.7.0.4

Re: [PATCH 2.6.32.y] hostap_cs: fix sleeping function called from invalid context

[Stanislaw Gruszka]

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

Re: [PATCH 2.6.32.y] hostap_cs: fix sleeping function called from invalid context

[Tim Gardner]

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 tim.gardner@canonical.com

Re: [PATCH 2.6.32.y] hostap_cs: fix sleeping function called from invalid context

[Stanislaw Gruszka]

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