[rt2x00-users] [PATCH 4/4] rt2x00: Implement watchdog monitoring

Ivo Van Doorn ivdoorn at gmail.com
Mon Jul 5 21:56:19 AEST 2010


On Mon, Jul 5, 2010 at 8:30 AM, Helmut Schaa
<helmut.schaa at googlemail.com> wrote:
> Am Sonntag 04 Juli 2010 schrieb Ivo van Doorn:
>> Implement watchdog monitoring for USB devices (PCI support can
>> be added later). This will determine if URBs being uploaded to
>> the hardware are actually returning. Both rt2500usb and rt2800usb
>> have shown that URBs being uploaded can remain hanging without
>> being released by the hardware.
>> By using this watchdog, a queue can be reset when this occurs.
>> For rt2800usb it has been tested that the connection is preserved
>> even though this interruption.
>
> Yeah, I like that idea but see some comments below ...
>
> [...]
>
>> diff --git a/drivers/net/wireless/rt2x00/rt2x00link.c b/drivers/net/wireless/rt2x00/rt2x00link.c
>> index d1948ea..38995d2 100644
>> --- a/drivers/net/wireless/rt2x00/rt2x00link.c
>> +++ b/drivers/net/wireless/rt2x00/rt2x00link.c
>> @@ -385,6 +385,12 @@ static void rt2x00link_tuner(struct work_struct *work)
>>               rt2x00dev->ops->lib->link_tuner(rt2x00dev, qual, link->count);
>>
>>       /*
>> +      * Perform watchdog checkings to determine the health of the hardware.
>> +      */
>> +     if (test_bit(DRIVER_SUPPORT_WATCHDOG, &rt2x00dev->flags))
>> +             rt2x00dev->ops->lib->watchdog(rt2x00dev);
>> +
>> +     /*
>>        * Send a signal to the led to update the led signal strength.
>>        */
>>       rt2x00leds_led_quality(rt2x00dev, qual->rssi);
>
> Could we move the watchdog into a different periodic function? If you'd like
> to keep it here rt2x00link_tuner could be renamed to do_periodic_stuff (or
> something more appropriate but you get the idea ;) ).
>
> One more thing: since link tuning is disabled in AP mode (see patch "rt2x00:
> Disable link tuning in AP mode") we should move the AP mode check into the
> periodic function mode instead of having it refuse to start the periodic work
> in rt2x00link_start_tuner (oh, and maybe rt2x00link_start_tuner should be
> renamed too). Because I think the watchdog functionality would be useful in
> AP and AdHoc mode as well.

Good point, I'll move the watchdog in a separate work structure. That way
it can always work independently of any link tuning.

>> diff --git a/drivers/net/wireless/rt2x00/rt2x00usb.c b/drivers/net/wireless/rt2x00/rt2x00usb.c
>> index a22837c..a630a26 100644
>> --- a/drivers/net/wireless/rt2x00/rt2x00usb.c
>> +++ b/drivers/net/wireless/rt2x00/rt2x00usb.c
>> @@ -292,6 +292,37 @@ void rt2x00usb_kill_tx_queue(struct rt2x00_dev *rt2x00dev,
>>  }
>>  EXPORT_SYMBOL_GPL(rt2x00usb_kill_tx_queue);
>>
>> +void rt2x00usb_watchdog(struct rt2x00_dev *rt2x00dev)
>> +{
>> +     struct data_queue *queue;
>> +     struct queue_entry_priv_usb *entry_priv;
>> +     u32 i;
>> +     u32 limit;
>> +
>> +     tx_queue_for_each(rt2x00dev, queue) {
>> +             if (rt2x00queue_timeout(queue)) {
>> +                     WARNING(rt2x00dev, "TX queue %d timed out, invoke reset", queue->qid);
>
> The warning is only printed when compiled with debug, right? Would it make
> sense to attract more attention if the watchdog triggers? Not sure though.

For the moment I want to keep it as warning, as long as the device is
able to recover
I don't think we should always print errors about it. We might make it
a bit smarter in
the future where it complains when the warning is triggered for the
same queue twice in a row.
at which point there really is a problem which is hard to restore.

Please note that during current testing with rt2800usb I can get this
warning 5 times in half an hour time,
and depending on some changes in the link tuner, I can get it dozens
of times per minute.

Ivo




More information about the users mailing list