[PATCH 1/2] wl1271: handle HW watchdog interrupt

[PATCH 1/2] wl1271: handle HW watchdog interrupt

[Eliad Peller]

unmask the WL1271_ACX_INTR_WATCHDOG interrupt.
when getting it - enqueue a recovery work and bail out.

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/wl1271_acx.h  |    3 ++-
 drivers/net/wireless/wl12xx/wl1271_main.c |    8 ++++++++
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/wl1271_acx.h b/drivers/net/wireless/wl12xx/wl1271_acx.h
index 7589167..b7c4908 100644
--- a/drivers/net/wireless/wl12xx/wl1271_acx.h
+++ b/drivers/net/wireless/wl12xx/wl1271_acx.h
@@ -61,7 +61,8 @@
 					    WL1271_ACX_INTR_HW_AVAILABLE  | \
 					    WL1271_ACX_INTR_DATA)
 
-#define WL1271_INTR_MASK                   (WL1271_ACX_INTR_EVENT_A      | \
+#define WL1271_INTR_MASK                   (WL1271_ACX_INTR_WATCHDOG     | \
+					    WL1271_ACX_INTR_EVENT_A      | \
 					    WL1271_ACX_INTR_EVENT_B      | \
 					    WL1271_ACX_INTR_HW_AVAILABLE | \
 					    WL1271_ACX_INTR_DATA)
diff --git a/drivers/net/wireless/wl12xx/wl1271_main.c b/drivers/net/wireless/wl12xx/wl1271_main.c
index c54887c..c9b51f4 100644
--- a/drivers/net/wireless/wl12xx/wl1271_main.c
+++ b/drivers/net/wireless/wl12xx/wl1271_main.c
@@ -529,6 +529,14 @@ static void wl1271_irq_work(struct work_struct *work)
 
 		intr &= WL1271_INTR_MASK;
 
+		if (unlikely(intr & WL1271_ACX_INTR_WATCHDOG)) {
+			wl1271_error("watchdog interrupt received! starting recovery.");
+			ieee80211_queue_work(wl->hw, &wl->recovery_work);
+
+			/* we are going to restart the chip. ignore any other interrupt. */
+			goto out;
+		}
+
 		if (intr & WL1271_ACX_INTR_DATA) {
 			wl1271_debug(DEBUG_IRQ, "WL1271_ACX_INTR_DATA");
 
-- 
1.7.0.4

[PATCH 2/2] wl1271: add recover testmode command

[Eliad Peller]

add RECOVER testmode command.
this command triggers a recovery sequence (by enqueueing a recovery_work).

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 drivers/net/wireless/wl12xx/wl1271_testmode.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/wl12xx/wl1271_testmode.c b/drivers/net/wireless/wl12xx/wl1271_testmode.c
index a3aa843..55ec442 100644
--- a/drivers/net/wireless/wl12xx/wl1271_testmode.c
+++ b/drivers/net/wireless/wl12xx/wl1271_testmode.c
@@ -37,6 +37,7 @@ enum wl1271_tm_commands {
 	WL1271_TM_CMD_CONFIGURE,
 	WL1271_TM_CMD_NVS_PUSH,
 	WL1271_TM_CMD_SET_PLT_MODE,
+	WL1271_TM_CMD_RECOVER,
 
 	__WL1271_TM_CMD_AFTER_LAST
 };
@@ -248,6 +249,15 @@ static int wl1271_tm_cmd_set_plt_mode(struct wl1271 *wl, struct nlattr *tb[])
 	return ret;
 }
 
+static int wl1271_tm_cmd_recover(struct wl1271 *wl, struct nlattr *tb[])
+{
+	wl1271_debug(DEBUG_TESTMODE, "testmode cmd recover");
+
+	ieee80211_queue_work(wl->hw, &wl->recovery_work);
+
+	return 0;
+}
+
 int wl1271_tm_cmd(struct ieee80211_hw *hw, void *data, int len)
 {
 	struct wl1271 *wl = hw->priv;
@@ -272,6 +282,8 @@ int wl1271_tm_cmd(struct ieee80211_hw *hw, void *data, int len)
 		return wl1271_tm_cmd_nvs_push(wl, tb);
 	case WL1271_TM_CMD_SET_PLT_MODE:
 		return wl1271_tm_cmd_set_plt_mode(wl, tb);
+	case WL1271_TM_CMD_RECOVER:
+		return wl1271_tm_cmd_recover(wl, tb);
 	default:
 		return -EOPNOTSUPP;
 	}
-- 
1.7.0.4

Re: [PATCH 2/2] wl1271: add recover testmode command

[Luciano Coelho]

On Wed, 2010-10-27 at 14:09 +0200, ext Eliad Peller wrote:
> add RECOVER testmode command.
> this command triggers a recovery sequence (by enqueueing a recovery_work).
> 
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

So, the point of this patch is to allow hardware recovery from the
userspace? In what kind of scenario do you need this?


-- 
Cheers,
Luca.

Re: [PATCH 2/2] wl1271: add recover testmode command

[Eliad Peller]

>> add RECOVER testmode command.
>> this command triggers a recovery sequence (by enqueueing a recovery_work).
>>
>> Signed-off-by: Eliad Peller <eliad@wizery.com>
>> ---
>
> So, the point of this patch is to allow hardware recovery from the
> userspace? In what kind of scenario do you need this?
>

yes.
it's mainly for testing (or if we want to recover manually for some reason).
this is why i implemented it as a testmode command.

i added it after observing some recovery issues (until applying
Juuso's "wl1271: Check interface state in op_* functions" :) )

Eliad.

Re: [PATCH 2/2] wl1271: add recover testmode command

[Gábor Stefanik]

On Mon, Nov 1, 2010 at 9:59 PM, Eliad Peller <eliad@wizery.com> wrote:
>>> add RECOVER testmode command.
>>> this command triggers a recovery sequence (by enqueueing a recovery_work).
>>>
>>> Signed-off-by: Eliad Peller <eliad@wizery.com>
>>> ---
>>
>> So, the point of this patch is to allow hardware recovery from the
>> userspace? In what kind of scenario do you need this?
>>
>
> yes.
> it's mainly for testing (or if we want to recover manually for some reason).
> this is why i implemented it as a testmode command.

Isn't testmode for features used during regulatory testing, that the
user should be unable to access to ensure further regulatory
compliance?

>
> i added it after observing some recovery issues (until applying
> Juuso's "wl1271: Check interface state in op_* functions" :) )
>
> Eliad.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Vista: [V]iruses, [I]ntruders, [S]pyware, [T]rojans and [A]dware. :-)

Re: [PATCH 2/2] wl1271: add recover testmode command

[Luciano Coelho]

On Mon, 2010-11-01 at 22:21 +0100, ext Gábor Stefanik wrote:
> On Mon, Nov 1, 2010 at 9:59 PM, Eliad Peller <eliad@wizery.com> wrote:
> >>> add RECOVER testmode command.
> >>> this command triggers a recovery sequence (by enqueueing a recovery_work).
> >>>
> >>> Signed-off-by: Eliad Peller <eliad@wizery.com>
> >>> ---
> >>
> >> So, the point of this patch is to allow hardware recovery from the
> >> userspace? In what kind of scenario do you need this?
> >>
> >
> > yes.
> > it's mainly for testing (or if we want to recover manually for some reason).
> > this is why i implemented it as a testmode command.
> 
> Isn't testmode for features used during regulatory testing, that the
> user should be unable to access to ensure further regulatory
> compliance?

Not exactly, it's just an interface to be used during production (and
possibly R&D) tests.  In the specific case of wl12xx, we use it for
production-line testing and for calibration and testing during R&D.

A normal user shouldn't use this by default (all distros and device
manufacturers should have it disabled in the kernel), but there's no way
to prevent a hacker from enabling it in a custom kernel.

In any case, I'm not sure I really like this idea of a HW recovery test
command.  This should really be done automatically by the driver and,
with Juuso's implementation plus the watchdog, we shouldn't really need
it.

-- 
Cheers,
Luca.

Re: [PATCH 2/2] wl1271: add recover testmode command

[Eliad Peller]

On Mon, Nov 1, 2010 at 11:42 PM, Luciano Coelho
<luciano.coelho@nokia.com> wrote:
>
> In any case, I'm not sure I really like this idea of a HW recovery test
> command.  This should really be done automatically by the driver and,
> with Juuso's implementation plus the watchdog, we shouldn't really need
> it.
>
as i wrote before - it's mainly for testing purposes.
that's why adding it as a testcmd made sense to me (i guess we do want
some easy way to trigger and test the recovery sequence).

Eliad.

Re: [PATCH 2/2] wl1271: add recover testmode command

[Luciano Coelho]

On Tue, 2010-11-02 at 08:05 +0100, ext Eliad Peller wrote:
> On Mon, Nov 1, 2010 at 11:42 PM, Luciano Coelho
> <luciano.coelho@nokia.com> wrote:
> >
> > In any case, I'm not sure I really like this idea of a HW recovery test
> > command.  This should really be done automatically by the driver and,
> > with Juuso's implementation plus the watchdog, we shouldn't really need
> > it.
> >
> as i wrote before - it's mainly for testing purposes.
> that's why adding it as a testcmd made sense to me (i guess we do want
> some easy way to trigger and test the recovery sequence).

Okay, so it's for testing the actual recovery sequence... That makes
sense.  I'll apply your patch.

-- 
Cheers,
Luca.

Re: [PATCH 2/2] wl1271: add recover testmode command

[Luciano Coelho]

On Wed, 2010-10-27 at 14:09 +0200, ext Eliad Peller wrote:
> add RECOVER testmode command.
> this command triggers a recovery sequence (by enqueueing a recovery_work).
> 
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

Applied, thank you!


-- 
Cheers,
Luca.

Re: [PATCH 1/2] wl1271: handle HW watchdog interrupt

[Luciano Coelho]

Hi Eliad,

On Wed, 2010-10-27 at 14:09 +0200, ext Eliad Peller wrote:
> unmask the WL1271_ACX_INTR_WATCHDOG interrupt.
> when getting it - enqueue a recovery work and bail out.
> 
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

This looks good.  Just to clarify, the hardware is going to send this
event when something goes wrong? What kind of situation triggers this
event?


-- 
Cheers,
Luca.

Re: [PATCH 1/2] wl1271: handle HW watchdog interrupt

[Eliad Peller]

hi Luca,

On Mon, Nov 1, 2010 at 3:22 PM, Luciano Coelho <luciano.coelho@nokia.com> wrote:
>> unmask the WL1271_ACX_INTR_WATCHDOG interrupt.
>> when getting it - enqueue a recovery work and bail out.
>>
>> Signed-off-by: Eliad Peller <eliad@wizery.com>
>> ---
>
> This looks good.  Just to clarify, the hardware is going to send this
> event when something goes wrong? What kind of situation triggers this
> event?
>

the WD is a simple timer that runs backward. when it gets to 0, the WD
interrupt is triggered.

normally, the firmware resets the timer when it's idle (every 500 msec
in the worst case).
thus, the WD will be triggered if the firmware is stuck (ASSERT /
endless loop) for about 2 seconds.

Eliad.

Re: [PATCH 1/2] wl1271: handle HW watchdog interrupt

[Luciano Coelho]

On Wed, 2010-10-27 at 14:09 +0200, ext Eliad Peller wrote:
> unmask the WL1271_ACX_INTR_WATCHDOG interrupt.
> when getting it - enqueue a recovery work and bail out.
> 
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

Applied and fixed a couple of checkpatch "line over 80 characters"
warnings.

Please remember to *always* run checkpatch and fix any eventual
warnings/errors before submitting.


-- 
Cheers,
Luca.

Re: [PATCH 1/2] wl1271: handle HW watchdog interrupt

[Eliad Peller]

On Tue, Nov 2, 2010 at 12:11 PM, Luciano Coelho
<luciano.coelho@nokia.com> wrote:
> On Wed, 2010-10-27 at 14:09 +0200, ext Eliad Peller wrote:
>> unmask the WL1271_ACX_INTR_WATCHDOG interrupt.
>> when getting it - enqueue a recovery work and bail out.
>>
>> Signed-off-by: Eliad Peller <eliad@wizery.com>
>> ---
>
> Applied and fixed a couple of checkpatch "line over 80 characters"
> warnings.
>
> Please remember to *always* run checkpatch and fix any eventual
> warnings/errors before submitting.
>
will do.
thanks!