Thread (6 messages) 6 messages, 2 authors, 2012-09-28

Re: [net-next PATCH 4/5] be2net: get rid of AMAP_SET/GET macros in TX path

From: David Miller <davem@davemloft.net>
Date: 2012-09-28 02:29:02

From: Sathya Perla <redacted>
Date: Thu, 27 Sep 2012 12:02:47 +0530
The AMAP macros are used in be2net for setting and parsing bits in HW
descriptors. The macros do this by calculating the mask & offset of each
field from the AMAP structure definition.
In the TX patch, replace the usage of these macros with code to explicitly
shift & mask each field. Doing this reduces instructions and improves
readability.

Signed-off-by: Sathya Perla <redacted>
Now you have endianness bugs.  The previous code worked with 8-bit
struct members and as such was endian neutral.

Now you're working with words, so you thus have to take endianness
into consideration.

The readability aspect is also extremely questionable, here's why.
The old code accessed struct members with _NAMES_ which described what
the values are and what they do.

This new code puts values into opaque "word" array members.  That's
about as crappy as it comes wrt. readability.  What in the world
does word[0] do?  I can't tell from it's name.  Yet with the existing
"struct amap_eth_hdr_wrb" there is none of that kind of confusion.

So don't pretend this new code even looks better, it looks like opaque
garbage to me.

Just admit this is an optimization that broke things on the endianness
you did not test.

be2net patches have been of a very low quality lately.  So I suggest
you improve things or else your submissions will be processed with an
extremely low priority.

Thanks.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help