Re: [PATCH 3/9 v2] myri10ge: rework parity error check and cleanup

[Jon Mason <mason@myri.com>]

On Mon, Jun 27, 2011 at 9:17 PM, Joe Perches <joe@perches.com> wrote:
> On Mon, 2011-06-27 at 15:54 -0500, Jon Mason wrote:
>> Clean up watchdog reset code:
>>  - move code that checks for stuck slice to a common routine
>>  - unless there is a confirmed h/w fault, verify that a stuck
>>    slice is still stuck in the watchdog worker; if the slice is no
>>    longer stuck, abort the reset.
>>  - this removes an egregious 2000ms pause in the watchdog worker that
>>    was a diagnostic aid (to look for spurious resets) the snuck into
>>    production code.
>> v2 includes corrections from Ben Hutchings and Joe Perches
>
> Here's some more trivia:
>
>> diff --git a/drivers/net/myri10ge/myri10ge.c b/drivers/net/myri10ge/myri10ge.c
> []
>> @@ -3442,6 +3443,42 @@ static u32 myri10ge_read_reboot(struct myri10ge_priv *mgp)
>>       return reboot;
>>  }
>>
>> +static void
>> +myri10ge_check_slice(struct myri10ge_slice_state *ss, int *reset_needed,
>> +                  int *busy_slice_cnt, u32 rx_pause_cnt)
>> +{
> []
>> +             /* nic seems like it might be stuck.. */
>> +             if (rx_pause_cnt != mgp->watchdog_pause) {
>> +                     if (net_ratelimit())
>> +                             netdev_warn(mgp->dev, "slice %d: TX paused, "
>> +                                         "check link partner\n", slice);
>
> I think this would be better if the format weren't split.
>
>                                netdev_warn(mgp->dev, "slice %d: TX paused, check link partner\n",
>                                            slice);
> or
>                                netdev_warn(mgp->dev,
>                                            "slice %d: TX paused, check link partner\n",
>                                            slice);
> or if you really must split it because exceeding 80 columns
> makes you itchy:
>                                netdev_warn(mgp->dev, "slice %d: "
>                                            "TX paused, check link partner\n",
>                                            slice);

Naa, I prefer it this way.

>> @@ -3465,8 +3504,7 @@ static void myri10ge_watchdog(struct work_struct *work)
>>                * For now, just report it */
>>               reboot = myri10ge_read_reboot(mgp);
>>               netdev_err(mgp->dev, "NIC rebooted (0x%x),%s resetting\n",
>> -                        reboot,
>> -                        myri10ge_reset_recover ? "" : " not");
>> +                        reboot, myri10ge_reset_recover ? " " : " not");
>
> I think this was correct before you changed it.
>
> Maybe:
>                           reboot, myri10ge_reset_recover ? "" : " not");

Yes, I believe this was the intent.

>
>
>