hme(4) hardware RX checksum

[Izumi Tsutsui]

In hme.c:hme_get(), m_pkthdr.csum_flags is always cleared after
a goto label, so hardware RX checksum on hme(4) seems unused at all.
(no one has checked hwcsum stats by options TCP_CSUM_COUNTERS etc?)
There is also a wrong pointer arith in VLAN case.
Is it okay to commit this fix?

Please do!

Now I notice gem.c has the similar code and hme.c needs to
have one more `else' for swcsum cases. Which is better?

Index: hme.c
===================================================================
RCS file: /cvsroot/src/sys/dev/ic/hme.c,v
retrieving revision 1.68
diff -u -r1.68 hme.c
--- hme.c 16 Dec 2008 22:35:31 -0000 1.68
+++ hme.c 6 Mar 2009 17:45:55 -0000
@@ -836,7 +836,7 @@
}

m0->m_pkthdr.csum_flags |= M_CSUM_DATA | M_CSUM_NO_PSEUDOHDR;
- }
+ } else
swcsum:
m0->m_pkthdr.csum_flags = 0;
#endif


BTW, does hardware checksum work even for VLAN packets?

At least it looks wrong to check sc_ethercom.ec_capenable
without checking eh->ether_type == ETHERTYPE_VLAN.

---
if (sc->sc_ethercom.ec_capenable & ETHERCAP_VLAN_MTU) {
pktlen = m0->m_pkthdr.len - ETHER_HDR_LEN -
ETHER_VLAN_ENCAP_LEN;
eh = (struct ether_header *) mtod(m0, void *) +
ETHER_VLAN_ENCAP_LEN;
} else {
pktlen = m0->m_pkthdr.len - ETHER_HDR_LEN;
eh = mtod(m0, struct ether_header *);
}
if (ntohs(eh->ether_type) != ETHERTYPE_IP)
goto swcsum;
---
Izumi Tsutsui

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Christos Zoulas]

On Mar 7, 2:49am, ***@ceres.dti.ne.jp (Izumi Tsutsui) wrote:
-- Subject: Re: hme(4) hardware RX checksum

| ***@astron.com wrote:
|
| > >In hme.c:hme_get(), m_pkthdr.csum_flags is always cleared after
| > >a goto label, so hardware RX checksum on hme(4) seems unused at all.
| > >(no one has checked hwcsum stats by options TCP_CSUM_COUNTERS etc?)
| > >
| > >There is also a wrong pointer arith in VLAN case.
| > >
| > >Is it okay to commit this fix?
| >
| > Please do!
|
| Now I notice gem.c has the similar code and hme.c needs to
| have one more `else' for swcsum cases. Which is better?

should be fixed too I guess.

| Index: hme.c
| ===================================================================
| RCS file: /cvsroot/src/sys/dev/ic/hme.c,v
| retrieving revision 1.68
| diff -u -r1.68 hme.c
| --- hme.c 16 Dec 2008 22:35:31 -0000 1.68
| +++ hme.c 6 Mar 2009 17:45:55 -0000
| @@ -836,7 +836,7 @@
| }
|
| m0->m_pkthdr.csum_flags |= M_CSUM_DATA | M_CSUM_NO_PSEUDOHDR;
| - }
| + } else
| swcsum:
| m0->m_pkthdr.csum_flags = 0;
| #endif

right the else needs to be there.

|
| BTW, does hardware checksum work even for VLAN packets?

It must, because there is vlan stuff inside a hw capabilities if statement
(I am just guessing).

| At least it looks wrong to check sc_ethercom.ec_capenable
| without checking eh->ether_type == ETHERTYPE_VLAN.

You are suggesting:

if (eh->ether_type == ETHERTYPE_VLAN &&
sc->sc_ethercom.ec_capenable & ETHERCAP_VLAN_MTU) {
?

christos

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[FUKAUMI Naoki]

hi

I'm sorry this is another topic but I remembered suddenly...

sys/dev/ic/hme.c has ether_cmp(), it returns 1 if two ethernet addresses
are identical. it is (only) used in hme_setladrf().

it should be eliminated and replaced by memcmp(), otherwise IFF_ALLMULTI
is set unexpectedly.

--
FUKAUMI Naoki

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Christos Zoulas]

Post by FUKAUMI Naoki
hi
I'm sorry this is another topic but I remembered suddenly...
sys/dev/ic/hme.c has ether_cmp(), it returns 1 if two ethernet addresses
are identical. it is (only) used in hme_setladrf().
it should be eliminated and replaced by memcmp(), otherwise IFF_ALLMULTI
is set unexpectedly.

Please fix it.

christos


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Darren Reed]

Have you read this:
http://gdamore.blogspot.com/2007/08/hme-checksum-limitations.html

You might want to follow up with Garrett directly, to see
if he has any tips/comments on the hardware itself.

Izumi Tsutsui wrote:
...

hme(4) seems to provide a dumb checksum for data after the IP header,
so I'm not sure if it can provide proper checksum even for
ETHERTYPE_VLAN packets. With a quick test, VLAN packets are
also swcsum'ed.
Currently hme(4) and gem(4) have the similar code which decodes
RX packet headers to adjust (deducting IP option sum etc.)
a dumb checksum data provided by hardware.
fxp(4) (i82559) also has the similar RX checksum hardware so
I wonder how we can share a common function to decode RX packets
for such dumb RX M_CSUM_DATA checksums.

...

With respect to this question, yes, it seems that it might
be a worthwhile idea for this functionality to bubble up
into the common ethernet code.

Darren

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de