ssb: Convert to use crc8 code in kernel library

The kernel now contains library routines to establish crc8 tables and to calculate the appropriate sums. Use them for ssb.

--- wireless-testing-new.orig/drivers/ssb/pci.c

+static inline void ltoh16_buf(u16 *buf, unsigned int size)

+static inline void htol16_buf(u16 *buf, unsigned int size)

Quoted 2 lines · Larry Finger

The kernel now contains library routines to establish crc8 tables and to calculate the appropriate sums. Use them for ssb.

[]

Quoted 1 line

--- wireless-testing-new.orig/drivers/ssb/pci.c

[]

Quoted 1 line

+static inline void ltoh16_buf(u16 *buf, unsigned int size)

Perhaps a rename and use le16_to_cpup?

Quoted 1 line

+static inline void htol16_buf(u16 *buf, unsigned int size)

and cpu_to_le16p?

Quoted 8 lines · Joe Perches

Quoted 2 lines · Larry Finger

The kernel now contains library routines to establish crc8 tables and to calculate the appropriate sums. Use them for ssb.

[]

Quoted 1 line

--- wireless-testing-new.orig/drivers/ssb/pci.c

[]

Quoted 1 line

+static inline void ltoh16_buf(u16 *buf, unsigned int size)

Quoted 5 lines

Perhaps a rename and use le16_to_cpup?

Quoted 1 line

+static inline void htol16_buf(u16 *buf, unsigned int size)

and cpu_to_le16p?

I'm sorry, but I don't see any advantage to using a pointer version here. Please enlighten me.

The kernel now contains library routines to establish crc8 tables and to calculate the appropriate sums. Use them for ssb.

drivers/ssb/pci.c +17 -1

+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 */

Quoted 30 lines · On Sat, 08 Oct 2011 17:28:42 -0500 Larry Finger <[email protected]>…

The kernel now contains library routines to establish crc8 tables and to calculate the appropriate sums. Use them for ssb.

drivers/ssb/pci.c +6 -0

+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));

drivers/ssb/pci.c +6 -1

+}
 
- ? ? 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);

Quoted 8 lines

? ? ? ? ? ? ? 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.
Does this patch decrease the code size, at least? I'll almost doubt it. If it doesn't, why are we actually doing this?
It doesn't even decrease the .data size. Worse, it converts a .const table to a .data table.

Just my 2 cents.

Agree. In brcm80211 we convert the entire sprom, calculate, and convert it back. Also not perfect I think as it loops over de sprom data twice.

Agree. I already tried converting bcma to use crc8:
[RFC][WORTH IT?][PATCH] bcma: make use of crc8 lib
http://lists.infradead.org/pipermail/b43-dev/2011-June/001466.html

But resigned, it was introducing some hacks or not optimal ops, I decided it's not worth it.

Even Arend said their brcm80211 is hacky about crc8 usage:

W dniu 15 czerwca 2011 21:26 użytkownik Arend van Spriel

Quoted 2 lines · <[email protected]> napisał:

Agree. In brcm80211 we convert the entire sprom, calculate, and convert it back. Also not perfect I think as it loops over de sprom data twice.

2011/10/9 Michael B?sch<[email protected]>:

Quoted 42 lines

Quoted 28 lines · On Sat, 08 Oct 2011 17:28:42 -0500 Larry Finger<[email protected]>…

The kernel now contains library routines to establish crc8 tables and to calculate the appropriate sums. Use them for ssb.

drivers/ssb/pci.c +17 -1

+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 */

drivers/ssb/pci.c err_pci:

Quoted 10 lines

drivers/ssb/pci.c +4 -0

+     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);

Quoted 7 lines

This endianness stuff is _really_ ugly.
Does this patch decrease the code size, at least? I'll almost doubt it. If it doesn't, why are we actually doing this?
It doesn't even decrease the .data size. Worse, it converts a .const table to a .data table.

Just my 2 cents.

Agree. I already tried converting bcma to use crc8:
[RFC][WORTH IT?][PATCH] bcma: make use of crc8 lib
http://lists.infradead.org/pipermail/b43-dev/2011-June/001466.html

But resigned, it was introducing some hacks or not optimal ops, I decided it's not worth it.

Even Arend said their brcm80211 is hacky about crc8 usage:

W dniu 15 czerwca 2011 21:26 u?ytkownik Arend van Spriel <[email protected]> napisa?:

Quoted 2 lines

Agree. In brcm80211 we convert the entire sprom, calculate, and convert it back. Also not perfect I think as it loops over de sprom data twice.

Quoted 34 lines · On Sat, 08 Oct 2011 17:28:42 -0500 Larry Finger <[email protected]>…

The kernel now contains library routines to establish crc8 tables and to calculate the appropriate sums. Use them for ssb.

drivers/ssb/pci.c +19 -1

+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);

drivers/ssb/pci.c static int ssb_pci_sprom_get(struct ssb_ · line 692

Quoted 3 lines

drivers/ssb/pci.c +2 -0

+	/* restore endianess */
+	ltoh16_buf(buf, 2 * bus->sprom_size);
 	err = sprom_extract(bus, sprom, buf, bus->sprom_size);

This endianness stuff is _really_ ugly.

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

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

Just my 2 cents.

drivers/ssb/pci.c err_pci: · line 200

Quoted 6 lines · On Sat, 08 Oct 2011 17:28:42 -0500 Larry Finger <[email protected]>…

drivers/ssb/pci.c +5 -0

+static inline void htol16_buf(u16 *buf, unsigned int size)
+{
+	size /= 2;
+	while (size--)
+		*(__le16 *)(buf + size) = cpu_to_le16(*(buf + size));
 }

I'm not not sure compilers would optimize it out on little-endian systems. Perhaps you want a define that uses this code on big-endian systems and does nothing on little endian systems.

Also, it would be nice to have a compile-time check that size is even. Or maybe size should be the number of 16-bit words, but then it would be better to call the argument "count" or something like that.

drivers/ssb/pci.c err_pci: · line 200

Quoted 6 lines · On Sat, 08 Oct 2011 17:28:42 -0500 Larry Finger<[email protected]>…

drivers/ssb/pci.c +5 -0

+static inline void htol16_buf(u16 *buf, unsigned int size)
+{
+	size /= 2;
+	while (size--)
+		*(__le16 *)(buf + size) = cpu_to_le16(*(buf + size));
  }

I'm not not sure compilers would optimize it out on little-endian systems. Perhaps you want a define that uses this code on big-endian systems and does nothing on little endian systems.

Also, it would be nice to have a compile-time check that size is even. Or maybe size should be the number of 16-bit words, but then it would be better to call the argument "count" or something like that.

I am pretty sure that the compiler would optimize out the entire htol16_buf routine. After substitution for cpu_to_le16() on a little-endian system, the statement in the while loop becomes '*(buf + size) = *(buf + size)', which is

Quoted 14 lines · Pavel Roskin

drivers/ssb/pci.c err_pci: · line 200

Quoted 6 lines · On Sat, 08 Oct 2011 17:28:42 -0500 Larry Finger<[email protected]>…

drivers/ssb/pci.c +5 -0

+static inline void htol16_buf(u16 *buf, unsigned int size)
+{
+	size /= 2;
+	while (size--)
+		*(__le16 *)(buf + size) = cpu_to_le16(*(buf + size));
  }

I'm not not sure compilers would optimize it out on little-endian systems. Perhaps you want a define that uses this code on big-endian systems and does nothing on little endian systems.

Quoted 3 lines

Also, it would be nice to have a compile-time check that size is even. Or maybe size should be the number of 16-bit words, but then it would be better to call the argument "count" or something like that.

The patch was dropped. Even so, as this routine is found in brcmsmac, your comments warrant further discussion.

I am pretty sure that the compiler would optimize out the entire htol16_buf routine. After substitution for cpu_to_le16() on a little-endian system, the statement in the while loop becomes '*(buf + size) = *(buf + size)', which is certainly optimized away, as will the now empty while loop. The entire routine is reduced to 'size /= 2'. As this will have no effect on the external world, it will also be dropped leaving an empty htol16_buf(). I don't think any "#ifdef __BIG_ENDIAN ... #endif" statements are needed.

Your suggestion that the argument be renamed is good, but there is no need to check for an even number as the data in question come from 16-bit reads of the SPROM on the b43 device. That number of 16-bit quantities was multiplied by 2 to get the byte count before calling this routine. Of course, the routine should have been passed the number of 16-bit words, not the byte count. My second version would have done this.

Larry

Feedback on the renaming is indeed valid. Passing the word count is better here. For brcmsmac I plan to fill the buffer using 8-bit reads from SPROM, verify the crc8, and perform the endianess conversion from le16 to cpu when crc is ok (actually under review internally).

Quoted 1 line

I plan to fill the buffer using 8-bit reads from SPROM,

That sounds like a good plan. When that is posted, I'll study if that kind of change makes sense for ssb.

Quoted 4 lines · On Sat, 15 Oct 2011 08:29:05 -0500 Larry Finger<[email protected]>…

Quoted 1 line

I plan to fill the buffer using 8-bit reads from SPROM,

That sounds like a good plan. When that is posted, I'll study if that kind of change makes sense for ssb.

If this change is made, it has to be tested on all supported devices, IMO. I would not be surprised if there are older devices that don't like that.