This is the mail archive of the
ecos-patches@sources.redhat.com
mailing list for the eCos project.
Re: Bug fix in cl_8900a ethernet driver
- From: Jonathan Larmour <jifl at eCosCentric dot com>
- To: Laurent Gonzalez <laurent dot gonzalez at silicomp dot fr>
- Cc: ecos-patches at sources dot redhat dot com
- Date: Fri, 11 Apr 2003 03:56:32 +0100
- Subject: Re: Bug fix in cl_8900a ethernet driver
- References: <1049981791.22016.29.camel@mishima.ri.silicomp.fr>
Laurent Gonzalez wrote:
hi folks,
the file attached contains a patch to fix this bug.
In cs8900a_send, the cast from data to sdata is not safe. If data is not
aligned on a short boundary (even address), the 16bit read with the
pointer sdata, may return an unpredictible value or raise an alignement
error (depending the hardware).
The bug will happen if the variable odd_byte becomes true, and the next
packet has an aligned value for data. the first byte is joined with
saved_data, and the pointer data becomes odd.
Thanks for spotting this. This indeed looks like a problem, although I'm
slightly concerned that the patch is endian-specific.... but looking at
the driver, it looks to be endian-specific in its treatment of odd_byte
too. Nevertheless, I thought I may as well fix it just in case since even
if the CS8900 is only ever used with ARMs which it may be, ARMs have been
known to be big-endian.
However it's somewhat sub-optimal to _always_ resort to byte fiddling.
Most times the amount of data to be spent will be large enough that
optimising the aligned case is worth it. So I've created the attached
patch. Since I don't have cs8900 hardware, please let me know if it works
for you and I'll check it in!
Jifl
--
eCosCentric http://www.eCosCentric.com/ The eCos and RedBoot experts
--[ "You can complain because roses have thorns, or you ]--
--[ can rejoice because thorns have roses." -Lincoln ]-- Opinions==mine
Index: src/if_cs8900a.c
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/eth/cl/cs8900a/current/src/if_cs8900a.c,v
retrieving revision 1.6
diff -u -5 -p -r1.6 if_cs8900a.c
--- src/if_cs8900a.c 14 Jun 2002 22:01:17 -0000 1.6
+++ src/if_cs8900a.c 11 Apr 2003 02:56:23 -0000
@@ -449,24 +449,42 @@ cs8900a_send(struct eth_drv_sc *sc, stru
len = sg_list[i].len;
if (len > 0) {
/* Finish the last word. */
if (odd_byte) {
+#if CYG_BYTEORDER == CYG_MSBFIRST
+ saved_data = ((cyg_uint16)*data++) | (saved_data << 8);
+#else
saved_data |= ((cyg_uint16)*data++) << 8;
+#endif
HAL_WRITE_UINT16(cpd->base+CS8900A_RTDATA, saved_data);
len--;
odd_byte = false;
}
- /* Output contiguous words. */
- sdata = (cyg_uint16 *)data;
- while (len > 1) {
- HAL_WRITE_UINT16(cpd->base+CS8900A_RTDATA, *sdata++);
- len -= sizeof(cyg_uint16);
+ if ((CYG_ADDRESS)data & 0x1 == 0) {
+ /* Aligned on 16-bit boundary, so output contiguous words. */
+ sdata = (cyg_uint16 *)data;
+ while (len > 1) {
+ HAL_WRITE_UINT16(cpd->base+CS8900A_RTDATA, *sdata++);
+ len -= sizeof(cyg_uint16);
+ }
+ data = (cyg_uint8 *)sdata;
+ } else {
+ /* Not 16-bit aligned, so byte copy */
+ while (len > 1) {
+ saved_data = (cyg_uint16)*data++; // reuse saved_data
+#if CYG_BYTEORDER == CYG_MSBFIRST
+ saved_data = ((cyg_uint16)*data++) | (saved_data << 8);
+#else
+ saved_data |= ((cyg_uint16)*data++) << 8;
+#endif
+ HAL_WRITE_UINT16(cpd->base+CS8900A_RTDATA, saved_data);
+ len -= sizeof(cyg_uint16);
+ }
}
/* Save last byte, if necessary. */
if (len == 1) {
- data = (cyg_uint8 *)sdata;
saved_data = (cyg_uint16)*data;
odd_byte = true;
}
}
}