The kernel now contains library routines to establish crc8 tables and
to calculate the appropriate sums. Use them for ssb.
Signed-off-by: Larry Finger <[email protected]>
---
John,
This is -next material.
This has been tested on both and little- and big-endian platforms.
Larry
---
Kconfig | 1
pci.c | 85 ++++++++++++++++++----------------------------------------------
2 files changed, 26 insertions(+), 60 deletions(-)
Index: wireless-testing-new/drivers/ssb/pci.c
===================================================================
--- wireless-testing-new.orig/drivers/ssb/pci.c
+++ wireless-testing-new/drivers/ssb/pci.c
@@ -20,6 +20,7 @@
#include <linux/slab.h>
#include <linux/pci.h>
#include <linux/delay.h>
+#include <linux/crc8.h>
#include "ssb_private.h"
@@ -179,71 +180,29 @@ err_pci:
SPEX16(_outvar, _offset, _mask, _shift)
-static inline u8 ssb_crc8(u8 crc, u8 data)
+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)
{
- /* Polynomial: x^8 + x^7 + x^6 + x^4 + x^2 + 1 */
- static const u8 t[] = {
- 0x00, 0xF7, 0xB9, 0x4E, 0x25, 0xD2, 0x9C, 0x6B,
- 0x4A, 0xBD, 0xF3, 0x04, 0x6F, 0x98, 0xD6, 0x21,
- 0x94, 0x63, 0x2D, 0xDA, 0xB1, 0x46, 0x08, 0xFF,
- 0xDE, 0x29, 0x67, 0x90, 0xFB, 0x0C, 0x42, 0xB5,
- 0x7F, 0x88, 0xC6, 0x31, 0x5A, 0xAD, 0xE3, 0x14,
- 0x35, 0xC2, 0x8C, 0x7B, 0x10, 0xE7, 0xA9, 0x5E,
- 0xEB, 0x1C, 0x52, 0xA5, 0xCE, 0x39, 0x77, 0x80,
- 0xA1, 0x56, 0x18, 0xEF, 0x84, 0x73, 0x3D, 0xCA,
- 0xFE, 0x09, 0x47, 0xB0, 0xDB, 0x2C, 0x62, 0x95,
- 0xB4, 0x43, 0x0D, 0xFA, 0x91, 0x66, 0x28, 0xDF,
- 0x6A, 0x9D, 0xD3, 0x24, 0x4F, 0xB8, 0xF6, 0x01,
- 0x20, 0xD7, 0x99, 0x6E, 0x05, 0xF2, 0xBC, 0x4B,
- 0x81, 0x76, 0x38, 0xCF, 0xA4, 0x53, 0x1D, 0xEA,
- 0xCB, 0x3C, 0x72, 0x85, 0xEE, 0x19, 0x57, 0xA0,
- 0x15, 0xE2, 0xAC, 0x5B, 0x30, 0xC7, 0x89, 0x7E,
- 0x5F, 0xA8, 0xE6, 0x11, 0x7A, 0x8D, 0xC3, 0x34,
- 0xAB, 0x5C, 0x12, 0xE5, 0x8E, 0x79, 0x37, 0xC0,
- 0xE1, 0x16, 0x58, 0xAF, 0xC4, 0x33, 0x7D, 0x8A,
- 0x3F, 0xC8, 0x86, 0x71, 0x1A, 0xED, 0xA3, 0x54,
- 0x75, 0x82, 0xCC, 0x3B, 0x50, 0xA7, 0xE9, 0x1E,
- 0xD4, 0x23, 0x6D, 0x9A, 0xF1, 0x06, 0x48, 0xBF,
- 0x9E, 0x69, 0x27, 0xD0, 0xBB, 0x4C, 0x02, 0xF5,
- 0x40, 0xB7, 0xF9, 0x0E, 0x65, 0x92, 0xDC, 0x2B,
- 0x0A, 0xFD, 0xB3, 0x44, 0x2F, 0xD8, 0x96, 0x61,
- 0x55, 0xA2, 0xEC, 0x1B, 0x70, 0x87, 0xC9, 0x3E,
- 0x1F, 0xE8, 0xA6, 0x51, 0x3A, 0xCD, 0x83, 0x74,
- 0xC1, 0x36, 0x78, 0x8F, 0xE4, 0x13, 0x5D, 0xAA,
- 0x8B, 0x7C, 0x32, 0xC5, 0xAE, 0x59, 0x17, 0xE0,
- 0x2A, 0xDD, 0x93, 0x64, 0x0F, 0xF8, 0xB6, 0x41,
- 0x60, 0x97, 0xD9, 0x2E, 0x45, 0xB2, 0xFC, 0x0B,
- 0xBE, 0x49, 0x07, 0xF0, 0x9B, 0x6C, 0x22, 0xD5,
- 0xF4, 0x03, 0x4D, 0xBA, 0xD1, 0x26, 0x68, 0x9F,
- };
- return t[crc ^ data];
-}
-
-static u8 ssb_sprom_crc(const u16 *sprom, u16 size)
-{
- int word;
- u8 crc = 0xFF;
-
- for (word = 0; word < size - 1; word++) {
- crc = ssb_crc8(crc, sprom[word] & 0x00FF);
- crc = ssb_crc8(crc, (sprom[word] & 0xFF00) >> 8);
- }
- crc = ssb_crc8(crc, sprom[size - 1] & 0x00FF);
- crc ^= 0xFF;
+ 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));
}
static int sprom_check_crc(const u16 *sprom, size_t size)
{
- u8 crc;
- u8 expected_crc;
- u16 tmp;
-
- crc = ssb_sprom_crc(sprom, size);
- tmp = sprom[size - 1] & SSB_SPROM_REVISION_CRC;
- expected_crc = tmp >> SSB_SPROM_REVISION_CRC_SHIFT;
- if (crc != expected_crc)
+ if (crc8(srom_crc8_table, (u8 *)sprom, 2 * size, CRC8_INIT_VALUE) !=
+ CRC8_GOOD_VALUE(srom_crc8_table))
return -EPROTO;
return 0;
@@ -690,8 +649,11 @@ static int ssb_pci_sprom_get(struct ssb_
buf = kcalloc(SSB_SPROMSIZE_WORDS_R123, sizeof(u16), GFP_KERNEL);
if (!buf)
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);
err = sprom_check_crc(buf, bus->sprom_size);
if (err) {
/* try for a 440 byte SPROM - revision 4 and higher */
@@ -702,6 +664,7 @@ static int ssb_pci_sprom_get(struct ssb_
return -ENOMEM;
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);
if (err) {
/* All CRC attempts failed.
@@ -721,9 +684,11 @@ static int ssb_pci_sprom_get(struct ssb_
goto out_free;
}
ssb_printk(KERN_WARNING PFX "WARNING: Invalid"
- " SPROM CRC (corrupt SPROM)\n");
+ " SPROM CRC (corrupt SPROM)\n");
}
}
+ /* restore endianess */
+ ltoh16_buf(buf, 2 * bus->sprom_size);
err = sprom_extract(bus, sprom, buf, bus->sprom_size);
out_free:
Index: wireless-testing-new/drivers/ssb/Kconfig
===================================================================
--- wireless-testing-new.orig/drivers/ssb/Kconfig
+++ wireless-testing-new/drivers/ssb/Kconfig
@@ -24,6 +24,7 @@ config SSB
# Common SPROM support routines
config SSB_SPROM
bool
+ select CRC8
# Support for Block-I/O. SELECT this from the driver that needs it.
config SSB_BLOCKIO
On 10/09/2011 12:51 AM, Michael B?sch wrote:
> On Sat, 08 Oct 2011 17:28:42 -0500
> Larry Finger <[email protected]> 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
On Sat, 15 Oct 2011 08:29:05 -0500
Larry Finger <[email protected]> wrote:
> > 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.
--
Greetings, Michael.
On Sat, 2011-10-08 at 17:28 -0500, Larry Finger wrote:
> 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)
Perhaps a rename and use le16_to_cpup?
> +static inline void htol16_buf(u16 *buf, unsigned int size)
and cpu_to_le16p?
On 10/15/2011 03:27 AM, Arend van Spriel wrote:
>
> 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).
That sounds like a good plan. When that is posted, I'll study if that kind of
change makes sense for ssb.
Larry
On 10/09/2011 03:48 AM, Rafał Miłecki wrote:
> 2011/10/9 Michael Büsch<[email protected]>:
>> On Sat, 08 Oct 2011 17:28:42 -0500
>> Larry Finger<[email protected]> 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.
>> 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ł:
>> 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.
John,
Please drop this patch. The amount of code churning is not worth the 100 byte
saving in the size of the generated code.
Larry
On Sat, 08 Oct 2011 17:28:42 -0500
Larry Finger <[email protected]> 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.
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.
--
Greetings, Michael.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 10/09/2011 10:48 AM, Rafał Miłecki wrote:
> 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ł:
>> 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.
>
Hi Rafał,
"not perfect" == "hacky" ;-)
I also just replied with a less hacky approach. Reading the sprom as
bytes will read the sprom content as is (little-endian) and you can do
the crc8 check without any conversions. After the check there is only
one conversion needed to move to a word array.
Gr. AvS
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQIcBAEBAgAGBQJOkXheAAoJELcEx/G14aEWnHwP+wZRWLladazO/zZnuMrnEuRk
xhMzbZcIy8gJwkGs/GwuWqnHBZu+Qx/k/BC3S1CNNmLPOf7qLNbS4hgKRZn+2Tze
FHYzXb6IPXkCE6MS5BFRi2qxnzrxam7gL00SB5NuGceS4b3LQL+wNHzPx5yPlPEW
KsJnecyFTabclun2zxucZPB19w2xrS84Xcl2Db+2nV3wSn700REr5mr3+pmVXmvd
ptLuWsc5ZoAnvuTQq/PUjKvQb/tA7CpDFs9+uvCeKa93Rb3JIRwTk5tAVadLEWn7
6IWyN0VMK+pdL2LT14qc1LWJa192tn8qbmOg21lKsQFepP4egJyRGWaU7aKo9bdG
PVSXfqme0azRl6vd+G28Q/SVdt//w5HBoPgdWlORhNdo/sov07QVl8gf80QkTn/A
Ij3M1+LMlUFu7weuIwgKBIA+Bi6CxTR/3ozK+S3ItpVqt6gCnMlDYtEUM6pk2vVj
8ppl0LtGHyrogFgwOrcYbxMKJGbk8Po82T+Um7wchcHuRxGLD3NuErndMiRwemMk
zALNLYNCLLftCSypCgkM5kyfDkDf7uZMxJZbPXyIIFCP4BXm5xrgx39sYQAVx4if
bIjW0W3OmIbBcQScZ4+KRZ5YN1g45VFRQ3+4BPyusApYqzwSmjxUj8aomfD2QhEJ
Fh/5uq3JIrpQazyEsILz
=JjZ0
-----END PGP SIGNATURE-----
On 10/14/2011 06:30 PM, Larry Finger wrote:
> On 10/14/2011 10:11 AM, Pavel Roskin wrote:
>> On Sat, 08 Oct 2011 17:28:42 -0500
>> Larry Finger<[email protected]> wrote:
>>
>>> +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.
>
> The patch was dropped. Even so, as this routine is found in brcmsmac, your
> comments warrant further discussion.
Following the thread over here ;-)
> 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.
Agree.
> 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).
Gr. AvS
On 10/15/2011 08:53 AM, Michael B?sch wrote:
> On Sat, 15 Oct 2011 08:29:05 -0500
> Larry Finger<[email protected]> wrote:
>
>>> 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.
Two of my devices are a BCM4303 (14e4:4301), and an early BCM4306. There are not
likely to be any older than that.
Larry
On 10/14/2011 05:11 PM, Pavel Roskin wrote:
> On Sat, 08 Oct 2011 17:28:42 -0500
> Larry Finger <[email protected]> wrote:
>
>> +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 think this patch was already dropped.
Gr. AvS
On 10/08/2011 05:38 PM, Joe Perches wrote:
> On Sat, 2011-10-08 at 17:28 -0500, Larry Finger wrote:
>> 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)
>
> Perhaps a rename and use le16_to_cpup?
>
>> +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.
Thanks,
Larry
2011/10/9 Michael Büsch <[email protected]>:
> On Sat, 08 Oct 2011 17:28:42 -0500
> Larry Finger <[email protected]> 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.
> 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ł:
> 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.
--
Rafał
On Sat, 2011-10-08 at 18:00 -0500, Larry Finger wrote:
> On 10/08/2011 05:38 PM, Joe Perches wrote:
> > On Sat, 2011-10-08 at 17:28 -0500, Larry Finger wrote:
> >> 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)
> >
> > Perhaps a rename and use le16_to_cpup?
> >
> >> +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.
It's just a naming consistency thing,
you already are using a pointer version,
and maybe this should go into byteorder.h
inline void array_cpu_to_le16p(u16 *p, size_t size)
{
while (size) {
cpu_to_le16p(p++);
size--;
}
}
On Sat, 08 Oct 2011 17:28:42 -0500
Larry Finger <[email protected]> wrote:
> +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.
--
Regards,
Pavel Roskin
On Fri, 14 Oct 2011 11:30:10 -0500
Larry Finger <[email protected]> wrote:
> 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
(There also is cpu_to_le16s(). Just for the record.)
--
Greetings, Michael.
On 10/14/2011 10:11 AM, Pavel Roskin wrote:
> On Sat, 08 Oct 2011 17:28:42 -0500
> Larry Finger<[email protected]> wrote:
>
>> +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.
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