This is the mail archive of the ecos-patches@sources.redhat.com mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [ECOS] Re: Bug in crc32 routine?


On Fri, Oct 18, 2002 at 03:14:24AM +0100, Jonathan Larmour wrote:
> Andrew Lunn wrote:
> >I checked around and found a few more CRC calculations. In summary
> >
> >1) zlib
> >2) jffs2
> >3) gdb stubs
> >4) cygmon
> >5) ks32c5000 net driver
> >6) Redboot - already using cyg_crc32()
> [snip]
> 
> Belated reply from me (what's new!), but just a few comments:
> 
> First of all, we aren't the master for jffs2. If there's a problem with 
> the crc32 calc for jffs2 we should tell <dwmw2@redhat.com>. But then I'm 
> not convinced a mismatch with zlib counts as a problem. If you agree, then 
> this is a nop :).

I say its a NOP. There is nothing wrong with using a different
algorithm. It makes little difference to code size since they (now)
shares the same table of coefficients, which is the dominant factor.

> 
> >--- packages/devs/eth/arm/ks32c5000/current/cdl/ks32c5000_eth.cdl	23 
> >May 2002 23:00:40 -0000	1.2
> >+++ packages/devs/eth/arm/ks32c5000/current/cdl/ks32c5000_eth.cdl	11 
> >Oct 2002 11:32:31 -0000
> >@@ -58,6 +58,8 @@
> > 	active_if     CYGPKG_IO_ETH_DRIVERS
> >     implements    CYGHWR_NET_DRIVERS
> >     implements    CYGHWR_NET_DRIVER_ETH0
> >+    requires      (CYGPKG_CRC || (!(CYG_HAL_CPUTYPE == \"KS32C5000A\" )))
> >+
> 
> More out of interest than anything, CDL has an implies operator which 
> allows you to use:
> 
>  requires { (CYG_HAL_CPUTYPE == \"KS32C5000A\" ) implies CYGPKG_CRC }
> 
> which is more readable.

Yep, forgot about that.

Although, in the past i've had problems with implies. I think implies
implies the CDL conflict engine can modify these options to try to
resolve the conflict. As you point out below, CYCPKG_CRC may not be
know, so it may actually try to resolve this conflict by changing the
CPUTYPE! This is all theoretical, i've not tried it.

> One thing I'm concerned about though is that the "default" requirements of 
> the eth driver include a package that is not included by default. It shows 
> up two long-standing CDL issues:
> 
> - only hardware packages can be listed in ecos.db targets
> - there is no way for the user to request automatically loading required 
> packages not currently in the config
> 
> the former would be easy to fix, but would be a band-aid for the latter. 
> Bart's on holiday for a week, but I'd be interested in his feedback. A 
> bugzilla bug should probably be submitted to track this (depending on what 
> the correct value of "this" is ;-)).

This is a deficiency i noted when testing. I was in two minds if to
add the CRC package to the net template, but decided not to since its
only required by one driver.

I'll leave bugzilla to you. 
 
> >+++ packages/services/crc/current/doc/crc.sgml	11 Oct 2002 11:32:33 
> >-0000
> >@@ -5,7 +5,7 @@
> > The CRC package provides implementation of CRC algorithms. This
> > includes the POSIX CRC calculation which produces the same result as
> > the cksum command on Linux, another 32 bit CRC by Gary S. Brown and a
> >-16bit CRC.
> >+16bit CRC. The CRC used for Ethernet FCS is also implemented.
> > </PARA>
> > </PARTINTRO>
> > <CHAPTER id="crc-functions">
> >@@ -35,17 +35,45 @@
> > <sect2 id="services-crc-api-cyg-crc32">
> > <title>cyg_crc32</title>
> > <para>
> >-This function implements a 32 bit CRC by Gary S. Brown. It uses the
> >+These functions implements a 32 bit CRC by Gary S. Brown. It uses the
> 
> implement :). I've just gone ahead and tweaked this in the repo, and a few 
> other trivial speeling typos.

No problem. I explicitly said i cannot spell! Well, its more likely
the words are spelled correctly, its just the wrong word!

> >+  if (1667500021 != cyg_ether_crc32(license_txt,sizeof(license_txt)-1)) {
> 
> Mumble, mumble, 32-bit reliance, mumble ;-). Really, that's what the type 
> defs in <cyg/infra/cyg_type.h> are for - rather than using unsigned longs. 
> (Although in due course we should use the C99 <inttypes.h> when GCC gets 
> round to implementing it).

OK, maybe, if i can find some free time with nothing more exciting to
do...

        Andrew


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]