linux-kernel - RE: [PATCH 1/2] I/OAT: Add watchdog/reset functionality to ioatdma driver

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: <7F38996F7185A24AB9071ED4950AD8C101D2C62A@swsmsx413.ger.corp.intel.com>
Date:	Fri, 18 Jul 2008 16:53:03 +0100
From:	"Sosnowski, Maciej" <maciej.sosnowski@...el.com>
To:	"Williams, Dan J" <dan.j.williams@...el.com>
Cc:	"Nelson, Shannon" <shannon.nelson@...el.com>,
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/2] I/OAT: Add watchdog/reset functionality to ioatdma driver

Dan Williams wrote:
> On Fri, Jul 11, 2008 at 4:36 AM, Maciej Sosnowski
> <maciej.sosnowski@...el.com> wrote:
>> Due to occasional DMA channel hangs observed for I/OAT versions 1.2
>> and 2.0 a watchdog has been introduced to check every 2 seconds
>> if all channels progress normally.
>> If stuck channel is detected, driver resets it.
>> The reset is done in two parts. The second part is scheduled
>> by the first one to reinitialize the channel after the restart.
>> 
> 
> checkpatch had some valid suggestions, please give it a run.

Hi Dan,
Thanks a lot for your review.
I have just sent v2 of this patch series updated according to your
suggestions.
This includes resolving checkpatch issues.

> A general comment about this change it seems to add a bit of
> complexity.  The driver now has two workqueue routines and a tasklet.
> Might some of these be combined?  In other words, do we need a free
> running watchdog when no descriptors are in flight?  It seems that
> using a timer to kick the cleanup tasklet could be used in place of
> the ioat_dma_chan_watchdog workqueue.
 
The idea was to run watchdog not too frequently 
in order to minimize its impact on the performance.
That's why it is called every 2 secs.
Cleanup is a differnt story and it is called more frequently,
so it would be difficult to combine these two things.
BTW - cleanup tasklet is not used at the moment
(TCP does not use interrupts but polling for cleanups).

>> Sleep time in self_test has been increased to 10ms
>> since in some cases 1ms appeared not to be enough.
> 
> This should be broken out into a separate patch.  Perhaps with an
> explanation of why it needs extra time for this particular
> transaction.

It has been separated in v2.

> 
> Regards,
> Dan

Thanks,
Maciej
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/