linux-kernel - Re: [PATCH v5 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

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: <20140503114029.GA19315@electric-eye.fr.zoreil.com>
Date:	Sat, 3 May 2014 13:40:29 +0200
From:	Francois Romieu <romieu@...zoreil.com>
To:	Darek Marcinkiewicz <reksio@...term.pl>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

Darek Marcinkiewicz <reksio@...term.pl> :
[...]
> This driver adds support for EtherCAT master module located on CCAT
> FPGA found on Beckhoff CX series industrial PCs. The driver exposes
> EtherCAT master as an ethernet interface.
> 
> EtherCAT is a filedbus protocol defined on top of ethernet and Beckhoff

s/filedbus/fieldbus/ :o)

> CX5020 PCs come with built-in EtherCAT master module located on a FPGA,
> which in turn is connected to a PCI bus.
> 
> Signed-off-by: Dariusz Marcinkiewicz <reksio@...term.pl>

I have attached four patches. Patch #3 / 4 contains a bugfix. Remaining
patches are small nits.

The points below are not showstoppers. Your answer will be welcome though.

If the tx_desc_free list can turn empty, I don't see much point for not
using an array as descriptor ring. It's quite common for linux ethernet
drivers.

Be it a list or an array, sizing it as the max count of descriptors that
could fit in the memory window through which packet descriptors _and_
ethernet data are copied seems overkill. It may be an upper bound though.
If you are not sure about a sane default value, ethtool API allows to
change the descriptor ring size.

Given the amount of debug messages, ethtool registers dump support could
be of some value.

I guess that the ASIC and the host CPU need some external polling to
communicate (no IRQ, really ?), whence the hrtimer. It should naturally
fit with NAPI. hrtimer could thus be disabled as soon as some ASIC event
is detected. It should save some hrtimer work when load increases and
the usual Tx lock avoidance patterns would kick in (current locking is
gross).

Did Beckhoff document their FPGA PCI interface ?

-- 
Ueimor

View attachment "0001-ec_bhf-inverted-xmas-tree.patch" of type "text/plain" (1446 bytes)

View attachment "0002-ec_bhf-line-breaks-and-indent.patch" of type "text/plain" (4203 bytes)

View attachment "0003-ec_bhf-rx_desc-vs-tx_desc-confusion.patch" of type "text/plain" (1249 bytes)

View attachment "0004-ec_bhf-use-once-variable-removal.patch" of type "text/plain" (1548 bytes)