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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BANLkTinaH0yCg+uFOZL5MiHhFqXNxd4sAw@mail.gmail.com>
Date:	Mon, 27 Jun 2011 22:31:23 -0500
From:	Jon Mason <mason@...i.com>
To:	Joe Perches <joe@...ches.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	Andrew Gallatin <gallatin@...i.com>
Subject: Re: [PATCH 3/9 v2] myri10ge: rework parity error check and cleanup

On Mon, Jun 27, 2011 at 9:17 PM, Joe Perches <joe@...ches.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.

>
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html