2009-09-14 05:18:24

by Mike Frysinger

Subject: [PATCH] Revert "[Bluetooth] Eliminate checks for impossible conditions in IRQ handler"

Commit ac019360fe3 changed the irq handler logic to BUG_ON rather than
returning IRQ_NONE when the incoming argument is invalid. While this
works in most cases, it doesn't work when the IRQ is shared with other
devices (or when DEBUG_SHIRQ is enabled).

So revert the previous change and replace the warning message with a
comment.

Signed-off-by: Michael Hennerich <[email protected]>
Signed-off-by: Mike Frysinger <[email protected]>
---
drivers/bluetooth/bluecard_cs.c | 4 +++-
drivers/bluetooth/bt3c_cs.c | 4 +++-
drivers/bluetooth/btuart_cs.c | 4 +++-
drivers/bluetooth/dtl1_cs.c | 4 +++-
4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/bluecard_cs.c b/drivers/bluetooth/bluecard_cs.c
index b0e569b..a3a8f4d 100644
--- a/drivers/bluetooth/bluecard_cs.c
+++ b/drivers/bluetooth/bluecard_cs.c
@@ -503,7 +503,9 @@ static irqreturn_t bluecard_interrupt(int irq, void *dev_inst)
unsigned int iobase;
unsigned char reg;

- BUG_ON(!info->hdev);
+ if (!info || !info->hdev)
+ /* our irq handler is shared */
+ return IRQ_NONE;

if (!test_bit(CARD_READY, &(info->hw_state)))
return IRQ_HANDLED;
diff --git a/drivers/bluetooth/bt3c_cs.c b/drivers/bluetooth/bt3c_cs.c
index d58e22b..9a7578e 100644
--- a/drivers/bluetooth/bt3c_cs.c
+++ b/drivers/bluetooth/bt3c_cs.c
@@ -345,7 +345,9 @@ static irqreturn_t bt3c_interrupt(int irq, void *dev_inst)
int iir;
irqreturn_t r = IRQ_NONE;

- BUG_ON(!info->hdev);
+ if (!info || !info->hdev)
+ /* our irq handler is shared */
+ return IRQ_NONE;

iobase = info->p_dev->io.BasePort1;

diff --git a/drivers/bluetooth/btuart_cs.c b/drivers/bluetooth/btuart_cs.c
index efd689a..cd9cb6c 100644
--- a/drivers/bluetooth/btuart_cs.c
+++ b/drivers/bluetooth/btuart_cs.c
@@ -295,7 +295,9 @@ static irqreturn_t btuart_interrupt(int irq, void *dev_inst)
int iir, lsr;
irqreturn_t r = IRQ_NONE;

- BUG_ON(!info->hdev);
+ if (!info || !info->hdev)
+ /* our irq handler is shared */
+ return IRQ_NONE;

iobase = info->p_dev->io.BasePort1;

diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
index 2cc7b32..588678a 100644
--- a/drivers/bluetooth/dtl1_cs.c
+++ b/drivers/bluetooth/dtl1_cs.c
@@ -299,7 +299,9 @@ static irqreturn_t dtl1_interrupt(int irq, void *dev_inst)
int iir, lsr;
irqreturn_t r = IRQ_NONE;

- BUG_ON(!info->hdev);
+ if (!info || !info->hdev)
+ /* our irq handler is shared */
+ return IRQ_NONE;

iobase = info->p_dev->io.BasePort1;

--
1.6.4.2

2009-09-14 09:58:39

by Marcel Holtmann

Subject: Re: [PATCH] Revert "[Bluetooth] Eliminate checks for impossible conditions in IRQ handler"

Hi Mike,

> Commit ac019360fe3 changed the irq handler logic to BUG_ON rather than
> returning IRQ_NONE when the incoming argument is invalid. While this
> works in most cases, it doesn't work when the IRQ is shared with other
> devices (or when DEBUG_SHIRQ is enabled).
>
> So revert the previous change and replace the warning message with a
> comment.

please don't use git revert here. Send an updated patch that fixes this
issue correct and please send it with a proper subject line.

Regards

Marcel

2009-09-14 17:43:50

by Mike Frysinger

Subject: [PATCH v2] [Bluetooth] redo checks in IRQ handler for shared IRQ support

Commit ac019360fe3 changed the irq handler logic to BUG_ON rather than
returning IRQ_NONE when the incoming argument is invalid. While this
works in most cases, it doesn't work when the IRQ is shared with other
devices (or when DEBUG_SHIRQ is enabled).

So revert the previous change and replace the warning message with a
comment explaining that we want this behavior.

Signed-off-by: Michael Hennerich <[email protected]>
Signed-off-by: Mike Frysinger <[email protected]>
---
v2
- tweak commit log to not use git-revert

drivers/bluetooth/bluecard_cs.c | 4 +++-
drivers/bluetooth/bt3c_cs.c | 4 +++-
drivers/bluetooth/btuart_cs.c | 4 +++-
drivers/bluetooth/dtl1_cs.c | 4 +++-
4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/bluecard_cs.c b/drivers/bluetooth/bluecard_cs.c
index b0e569b..a3a8f4d 100644
--- a/drivers/bluetooth/bluecard_cs.c
+++ b/drivers/bluetooth/bluecard_cs.c
@@ -503,7 +503,9 @@ static irqreturn_t bluecard_interrupt(int irq, void *dev_inst)
unsigned int iobase;
unsigned char reg;

- BUG_ON(!info->hdev);
+ if (!info || !info->hdev)
+ /* our irq handler is shared */
+ return IRQ_NONE;

if (!test_bit(CARD_READY, &(info->hw_state)))
return IRQ_HANDLED;
diff --git a/drivers/bluetooth/bt3c_cs.c b/drivers/bluetooth/bt3c_cs.c
index d58e22b..9a7578e 100644
--- a/drivers/bluetooth/bt3c_cs.c
+++ b/drivers/bluetooth/bt3c_cs.c
@@ -345,7 +345,9 @@ static irqreturn_t bt3c_interrupt(int irq, void *dev_inst)
int iir;
irqreturn_t r = IRQ_NONE;

- BUG_ON(!info->hdev);
+ if (!info || !info->hdev)
+ /* our irq handler is shared */
+ return IRQ_NONE;

iobase = info->p_dev->io.BasePort1;

diff --git a/drivers/bluetooth/btuart_cs.c b/drivers/bluetooth/btuart_cs.c
index efd689a..cd9cb6c 100644
--- a/drivers/bluetooth/btuart_cs.c
+++ b/drivers/bluetooth/btuart_cs.c
@@ -295,7 +295,9 @@ static irqreturn_t btuart_interrupt(int irq, void *dev_inst)
int iir, lsr;
irqreturn_t r = IRQ_NONE;

- BUG_ON(!info->hdev);
+ if (!info || !info->hdev)
+ /* our irq handler is shared */
+ return IRQ_NONE;

iobase = info->p_dev->io.BasePort1;

diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
index 2cc7b32..588678a 100644
--- a/drivers/bluetooth/dtl1_cs.c
+++ b/drivers/bluetooth/dtl1_cs.c
@@ -299,7 +299,9 @@ static irqreturn_t dtl1_interrupt(int irq, void *dev_inst)
int iir, lsr;
irqreturn_t r = IRQ_NONE;

- BUG_ON(!info->hdev);
+ if (!info || !info->hdev)
+ /* our irq handler is shared */
+ return IRQ_NONE;

iobase = info->p_dev->io.BasePort1;

--
1.6.4.2

2009-09-15 00:24:28

by Robert Hancock

Subject: Re: [PATCH] Revert "[Bluetooth] Eliminate checks for impossible conditions in IRQ handler"

On 09/13/2009 11:18 PM, Mike Frysinger wrote:
> Commit ac019360fe3 changed the irq handler logic to BUG_ON rather than
> returning IRQ_NONE when the incoming argument is invalid. While this
> works in most cases, it doesn't work when the IRQ is shared with other
> devices (or when DEBUG_SHIRQ is enabled).

Something doesn't add up here. It shouldn't be possible for the incoming
argument to be invalid. I'd think that if it is it means that the IRQ
handler is being registered too soon, before the data structures it
requires are set up fully. If that's the case, reverting the change just
partially papers over the bug.

>
> So revert the previous change and replace the warning message with a
> comment.
>
> Signed-off-by: Michael Hennerich<[email protected]>
> Signed-off-by: Mike Frysinger<[email protected]>
> ---
> drivers/bluetooth/bluecard_cs.c | 4 +++-
> drivers/bluetooth/bt3c_cs.c | 4 +++-
> drivers/bluetooth/btuart_cs.c | 4 +++-
> drivers/bluetooth/dtl1_cs.c | 4 +++-
> 4 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bluetooth/bluecard_cs.c b/drivers/bluetooth/bluecard_cs.c
> index b0e569b..a3a8f4d 100644
> --- a/drivers/bluetooth/bluecard_cs.c
> +++ b/drivers/bluetooth/bluecard_cs.c
> @@ -503,7 +503,9 @@ static irqreturn_t bluecard_interrupt(int irq, void *dev_inst)
> unsigned int iobase;
> unsigned char reg;
>
> - BUG_ON(!info->hdev);
> + if (!info || !info->hdev)
> + /* our irq handler is shared */
> + return IRQ_NONE;
>
> if (!test_bit(CARD_READY,&(info->hw_state)))
> return IRQ_HANDLED;
> diff --git a/drivers/bluetooth/bt3c_cs.c b/drivers/bluetooth/bt3c_cs.c
> index d58e22b..9a7578e 100644
> --- a/drivers/bluetooth/bt3c_cs.c
> +++ b/drivers/bluetooth/bt3c_cs.c
> @@ -345,7 +345,9 @@ static irqreturn_t bt3c_interrupt(int irq, void *dev_inst)
> int iir;
> irqreturn_t r = IRQ_NONE;
>
> - BUG_ON(!info->hdev);
> + if (!info || !info->hdev)
> + /* our irq handler is shared */
> + return IRQ_NONE;
>
> iobase = info->p_dev->io.BasePort1;
>
> diff --git a/drivers/bluetooth/btuart_cs.c b/drivers/bluetooth/btuart_cs.c
> index efd689a..cd9cb6c 100644
> --- a/drivers/bluetooth/btuart_cs.c
> +++ b/drivers/bluetooth/btuart_cs.c
> @@ -295,7 +295,9 @@ static irqreturn_t btuart_interrupt(int irq, void *dev_inst)
> int iir, lsr;
> irqreturn_t r = IRQ_NONE;
>
> - BUG_ON(!info->hdev);
> + if (!info || !info->hdev)
> + /* our irq handler is shared */
> + return IRQ_NONE;
>
> iobase = info->p_dev->io.BasePort1;
>
> diff --git a/drivers/bluetooth/dtl1_cs.c b/drivers/bluetooth/dtl1_cs.c
> index 2cc7b32..588678a 100644
> --- a/drivers/bluetooth/dtl1_cs.c
> +++ b/drivers/bluetooth/dtl1_cs.c
> @@ -299,7 +299,9 @@ static irqreturn_t dtl1_interrupt(int irq, void *dev_inst)
> int iir, lsr;
> irqreturn_t r = IRQ_NONE;
>
> - BUG_ON(!info->hdev);
> + if (!info || !info->hdev)
> + /* our irq handler is shared */
> + return IRQ_NONE;
>
> iobase = info->p_dev->io.BasePort1;
>

2009-09-15 11:28:58

by Mike Frysinger

Subject: Re: [PATCH] Revert "[Bluetooth] Eliminate checks for impossible conditions in IRQ handler"

On Mon, Sep 14, 2009 at 19:17, Robert Hancock wrote:
> On 09/13/2009 11:18 PM, Mike Frysinger wrote:
>> Commit ac019360fe3 changed the irq handler logic to BUG_ON rather than
>> returning IRQ_NONE when the incoming argument is invalid.  While this
>> works in most cases, it doesn't work when the IRQ is shared with other
>> devices (or when DEBUG_SHIRQ is enabled).
>
> Something doesn't add up here. It shouldn't be possible for the incoming
> argument to be invalid. I'd think that if it is it means that the IRQ
> handler is being registered too soon, before the data structures it requires
> are set up fully. If that's the case, reverting the change just partially
> papers over the bug.

it's a shared irq. so there is no way to guarantee that the incoming
interrupt is from the bluetooth device.

aaaand there's the issue of registering too soon, but the pcmcia
framework doesnt seem to provide a way to fix this. enable
DEBUG_SHIRQ and you too will see the kernel crash (since this causes
the IRQ to "fire" as soon as it's generated).

i dont know anything about these devices, but another fix may be to
remove the shareable flag from the irq registration.
-mike

2010-01-19 11:16:29

by Mike Frysinger

Subject: Re: [PATCH v2] [Bluetooth] redo checks in IRQ handler for shared IRQ support

On Mon, Sep 14, 2009 at 12:43, Mike Frysinger wrote:
> Commit ac019360fe3 changed the irq handler logic to BUG_ON rather than
> returning IRQ_NONE when the incoming argument is invalid.  While this
> works in most cases, it doesn't work when the IRQ is shared with other
> devices (or when DEBUG_SHIRQ is enabled).
>
> So revert the previous change and replace the warning message with a
> comment explaining that we want this behavior.

was this picked up ?
-mike

2010-01-19 18:22:13

by Marcel Holtmann

Subject: Re: [PATCH v2] [Bluetooth] redo checks in IRQ handler for shared IRQ support

Hi Mike,

> > Commit ac019360fe3 changed the irq handler logic to BUG_ON rather than
> > returning IRQ_NONE when the incoming argument is invalid. While this
> > works in most cases, it doesn't work when the IRQ is shared with other
> > devices (or when DEBUG_SHIRQ is enabled).
> >
> > So revert the previous change and replace the warning message with a
> > comment explaining that we want this behavior.
>
> was this picked up ?

not it was not, but now it has been applied to my trees. Thanks.

Regards

Marcel