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

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

[Mike Frysinger]

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 <michael.hennerich@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 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

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

[Marcel Holtmann]

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

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

[Mike Frysinger]

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 <michael.hennerich@analog.com>
Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
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

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

[Mike Frysinger]

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

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

[Marcel Holtmann]

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

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

[Robert Hancock]

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<michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Mike Frysinger<vapier-aBrp7R+bbdUdnm+yROfE0A@public.gmane.org>
> ---
>   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;
>

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

[Mike Frysinger]

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