Re: [PATCH] e100: Fix inconsistency in bad frames handling

On 06/06/2011 01:29 PM, Eric Dumazet wrote:

Le lundi 06 juin 2011 à 21:15 +0100, Ben Hutchings a écrit :

quoted

On Mon, 2011-06-06 at 10:56 -0700, Ben Greear wrote:

quoted

On 06/06/2011 10:49 AM, Brandeburg, Jesse wrote:

quoted

<added netdev>, removed other useless lists.

On Sat, 4 Jun 2011, Andrea Merello wrote:

quoted

In e100 driver it seems that the intention was to accept bad frames in
promiscuous mode and loopback mode.
I think this is evident because of the following code in the driver:

if (nic->flags&   promiscuous || nic->loopback) {
		config->rx_save_bad_frames = 0x1;	/* 1=save, 0=discard */
		config->rx_discard_short_frames = 0x0;	/* 1=discard, 0=save */
		config->promiscuous_mode = 0x1;		/* 1=on, 0=off */
	}

Hi, thanks for your work on e100.

quoted

However this intention is not really realized because bad frames are
discarded later by SW check.
This patch finally honors the above intention, making the RX code to
let bad frames to pass when the NIC is in promiscuous or loopback
mode.

I think this may be a mistake by the authors of the software developers
manual.  The manual suggests that save bad frames and save short frames
should be enabled in promisc mode, but all of our other drivers *do not*
save bad frames when in promiscuous mode (by design).  This is intentional
because a bad frame is just that, bad, and with no hope of knowing if the
data in it is okay/malicious/other.  I understand your reasoning above,
but realistically the rx_save_bad_frames should NOT be set.  I'd ack a
patch to comment that line out.

quoted

This helped me a lot to debug an FPGA ethernet core.
Maybe it can be also useful to someone else..

I think this patch is just that, debug only. As a developer I understand
why this is useful, but there is no reason any normal user would be able
to benefit from this, so for now, sorry:

NACK.

I think anyone sniffing a funky network would have benefit in
receiving all frames.  So, while it shouldn't be enabled by default,
it would be nice to have an ethtool command to turn on receiving
bad-crc frames, as well as receiving the 4-byte CRC on the end of
the packets.

It just so happens I have such a patch, in case others agree :)

How would a received skb be flagged as having a CRC error?

maybe some skb->pkt_type = PACKET_INVALID; or something...

That looks good to me.  pkt_type is passed up through some of the pf_socket
interfaces, so capture tools could easily be modified to pay attention to it.

We might also need to add a flag 'crc-included' so that tools could know
that the last 4 bytes of the packet are ethernet CRC, for NICs that support
that.

Thanks,
Ben

-- 
Ben Greear [off-list ref]
Candela Technologies Inc  http://www.candelatech.com


------------------------------------------------------------------------------
EditLive Enterprise is the world's most technically advanced content
authoring tool. Experience the power of Track Changes, Inline Image
Editing and ensure content is compliant with Accessibility Checking.
http://p.sf.net/sfu/ephox-dev2dev
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired