Re: [PATCH] ssb: Convert to use crc8 code in kernel library

["Arend van Spriel" <arend@broadcom.com>]

On 10/09/2011 12:51 AM, Michael Büsch wrote:
> On Sat, 08 Oct 2011 17:28:42 -0500
> Larry Finger <Larry.Finger@lwfinger.net> wrote:
> 
>> The kernel now contains library routines to establish crc8 tables and
>> to calculate the appropriate sums. Use them for ssb.
> 
>> +static u8 srom_crc8_table[CRC8_TABLE_SIZE];
>> +
>> +/* Polynomial:   x^8 + x^7 + x^6 + x^4 + x^2 + 1   */
>> +#define SROM_CRC8_POLY	0xAB
>> +
>> +static inline void ltoh16_buf(u16 *buf, unsigned int size)
>>  {
>> +	size /= 2;
>> +	while (size--)
>> +		*(buf + size) = le16_to_cpu(*(__le16 *)(buf + size));
>> +}
>>  
>> -	return crc;
>> +static inline void htol16_buf(u16 *buf, unsigned int size)
>> +{
>> +	size /= 2;
>> +	while (size--)
>> +		*(__le16 *)(buf + size) = cpu_to_le16(*(buf + size));
>>  }
> 
>>  		return -ENOMEM;
>> +	crc8_populate_lsb(srom_crc8_table, SROM_CRC8_POLY);
>>  	bus->sprom_size = SSB_SPROMSIZE_WORDS_R123;
>>  	sprom_do_read(bus, buf);
>> +	/* convert to le */
>> +	htol16_buf(buf, 2 * bus->sprom_size);
> 
>>  		bus->sprom_size = SSB_SPROMSIZE_WORDS_R4;
>>  		sprom_do_read(bus, buf);
>> +		htol16_buf(buf, 2 * bus->sprom_size);
>>  		err = sprom_check_crc(buf, bus->sprom_size);
> 
>> +	/* restore endianess */
>> +	ltoh16_buf(buf, 2 * bus->sprom_size);
>>  	err = sprom_extract(bus, sprom, buf, bus->sprom_size);
> 
> This endianness stuff is _really_ ugly.

It may seem ugly, but is not new. Choosing a 8-bit crc to check a 16-bit
array is not very efficient considering host endianess. The endianess
was also dealt with in the old version:

-	for (word = 0; word < size - 1; word++) {
-		crc = ssb_crc8(crc, sprom[word] & 0x00FF);
-		crc = ssb_crc8(crc, (sprom[word] & 0xFF00) >> 8);
-	}

It is a bit easier on the eye. I guess the ugliness comes from the fact
that there are two calls to htol16_buf.

A better approach would be to read sprom as bytes and run the crc8 over
the byte array. When ok do ltoh16_buf once.

> Does this patch decrease the code size, at least? I'll almost doubt it.
> If it doesn't, why are we actually doing this?

Probably for the same reason why struct list_head and related functions
are used. Trying to use what is commonly available in the kernel.

> It doesn't even decrease the .data size. Worse, it converts a .const
> table to a .data table.

True. .code size became .data size, because of the flexibility that the
table is generated for a given polynomial. Every 'bility' comes with a
price and this seems not too pricy.

> Just my 2 cents.
> 

Gr. AvS