Messages in this thread | | | Date | Wed, 22 Nov 2023 22:09:54 +0100 | From | Christian Marangi <> | Subject | Re: [net-next PATCH] net: phy: aquantia: drop wrong endianness conversion for addr and CRC |
| |
On Wed, Nov 22, 2023 at 08:25:16PM +0000, Russell King (Oracle) wrote: > On Wed, Nov 22, 2023 at 08:55:17PM +0100, Christian Marangi wrote: > > On Wed, Nov 22, 2023 at 06:53:50PM +0000, Russell King (Oracle) wrote: > > > On Wed, Nov 22, 2023 at 06:53:39PM +0100, Christian Marangi wrote: > > > > On Wed, Nov 22, 2023 at 05:24:33PM +0000, Russell King (Oracle) wrote: > > > > > On Wed, Nov 22, 2023 at 06:08:13PM +0100, Christian Marangi wrote: > > > > > > On further testing on BE target with kernel test robot, it was notice > > > > > > that the endianness conversion for addr and CRC in fw_load_memory was > > > > > > wrong and actually not needed. Values in define doesn't get converted > > > > > > and are passed as is and hardcoded values are already in what the PHY > > > > > > require, that is LE. > > > > > > > > > > > > Also drop the cpu_to_be32 for CRC calculation as it's wrong and use > > > > > > _swab32 instead, the word is taked from firmware and is always LE, the > > > > > > > > > > taken > > > > > > > > > > > mailbox will emit a BE CRC hence the word needs to be always swapped and > > > > > > the endianness of the host needs to be ignored. > > > > > > > > > > I'm not convinced. If the firmware is a bytestream (as most "files" are) > > > > > then for val = get_unaligned((u32 *)ptr), where ptr is an array of u8: > > > > > > > > > > ptr[0] ptr[1] ptr[2] ptr[3] val on LE val on BE > > > > > 0x01 0x02 0x03 0x04 0x04030201 0x01020304 > > > > > > > > > > So, endianness matters here, and I think as Jakub already suggested, you > > > > > need to use get_unaligned_le32(). > > > > > > > > > > > > > So they DO get converted to the HOST endian on reading the firmware from > > > > an nvmem cell or a filesystem? > > > > > > I don't like "converted". It's *not* a conversion. It's a fundamental > > > property of accessing memory using different sizes of access. > > > > > > As I attempted to explain above, if you have a file, and byte 0 > > > contains 0xAA, byte 1 of the file contains 0xBB, byte 2 contains > > > 0xCC, and byte 3 contains 0xDD, then if you read that file byte by > > > byte, you will get 0xAA, then 0xBB, then 0xCC and then 0xDD. > > > > > > If you map that file into memory, e.g. in userspace, using mmap(), > > > or allocating memory and reading four bytes into memory, and access > > > it using bytes, then at offset 0, you will find 0xAA, offset 1 will > > > be 0xBB, etc. > > > > > > The problems with endianness start when you move away from byte > > > access. > > > > > > If you use 16-bit accessors, then, a little endian machine is defined > > > that a 16-bit load from memory will result in the first byte being put > > > into the LSB of the 16-bit value, and the second byte will be put into > > > the MSB of the 16-bit value. So that would be 0xBBAA. However, on a big > > > endian machine, a 16-bit load will result in the first byte being put > > > into the MSB of the 16-bit value, and the second byte will be put into > > > the LSB of that value - meaning the 16-bit value will be 0xAABB. > > > > > > The second 16-bit value uses the next two bytes, and the order at which > > > these two bytes are placed into the 16-bit value reflects the same as > > > the first two bytes. So LE will be 0xDDCC and BE would be 0xCCDD. > > > > > > The same "swapping" happens with 32-bit, but of course instead of just > > > two bytes, it covers four bytes. On LE, a 32-bit access will give > > > 0xDDCCBBAA. On BE, that will be 0xAABBCCDD. > > > > > > Again, this is not to do with any kind of "conversion" happening in > > > software. It's a property of how the memory subsystem inside the CPU > > > works. > > > > > > > Again this is really dumping raw data from the read file directly to the > > > > mailbox. Unless phy_write does some conversion internally, but in that > > > > case how does it know what endian is the PHY internally? > > > > > > phy_write() does *no* conversion. The MDIO bus defines that a 16-bit > > > register value will be transferred, and the MDIO bus specifies that > > > bit 15 will be sent first, followed by subsequent bits down to bit 0. > > > > > > The access to the hardware to make this happen is required to ensure > > > that the value passed to phy_write() and read using phy_read() will > > > reflect this. So, if one does this: > > > > > > val = phy_read(phydev, 0); > > > > > > for (i = 15; i >= 0; i--) > > > printk("%u", !!(val & BIT(i))); > > > > > > printk("\n"); > > > > > > This will give you the stream of bits in the _order_ that they appeared > > > on the MDIO bus when phy_read() accessed. Doing the same with a value > > > to be written will produce the bits in the same value that they will > > > be placed on the MDIO bus. > > > > > > So, this means that if the BMCR contains 0x1234 in the PHY, phy_read() > > > will return 0x1234. Passing 0x1234 into phy_write() will write 0x1234 > > > in that register. The host endian is entirely irrelevant here. > > > > > > > Thanks a lot for the clarification. And sorry for misusing the word > > conversion. > > > > > > > I would make this explicit: > > > > > > > > > > u8 crc_data[4]; > > > > > > > > > > ... > > > > > > > > > > /* CRC is calculated using BE order */ > > > > > crc_data[0] = word >> 24; > > > > > crc_data[1] = word >> 16; > > > > > crc_data[2] = word >> 8; > > > > > crc_data[3] = word; > > > > > > > > > > crc = crc_ccitt_false(crc, crc_data, sizeof(crc_data)); > > > > > > > > > > which will be (a) completely unambiguous, and (b) completely > > > > > independent of the host endianness. > > > > > > > > But isn't this exactly what is done with ___constant_swab32 ? > > > > __swab32 should not change if the HOST is BE or LE. > > > > > > Let try again to make this clear. If one has this code: > > > > > > u32 word = 0x01020304; > > > u8 *ptr; > > > int i; > > > > > > ptr = (u8 *)&word; > > > > > > for (i = 0; i < 4; i++) > > > printk(" %02x", ptr[i]); > > > printk("\n"); > > > > > > Then, on a: > > > - LE machine, this will print " 04 03 02 01" > > > - BE machine, this will print " 01 02 03 04" > > > > > > Now, if you look at the definition of crc_ccitt_false(), it is > > > defined to do: > > > > > > while (len--) > > > crc = crc_ccitt_false_byte(crc, *buffer++); > > > > > > So, on a LE machine, this will feed the above bytes in the order of > > > 0x04, 0x03, 0x02, 0x01 in a LE machine, and 0x01, 0x02, 0x03, 0x04 > > > on a BE machine. > > > > > > > So it's really a problem of setting u8 in word and the order they are > > read in the system. > > Correct. > > > The first get_unaligned has to be changed to get_unaligned_le32 based on > > how the data are treated from passing from an u8 to u32. > > Yes. > > I'm going to use the term "bytestream", abbreviated to just stream, to > represent the firmware that you are going to upload, because that's > essentially what all files are. > > the first byte of the stream to appear in bits 7:0 of > VEND1_GLOBAL_MAILBOX_INTERFACE6 > > the second byte of the stream to appear in bits 15:8 of > VEND1_GLOBAL_MAILBOX_INTERFACE6 > > the third byte of the stream to appear in bits 7:0 of > VEND1_GLOBAL_MAILBOX_INTERFACE5 > > the forth byte of the stream to appear in bits 15:8 of > VEND1_GLOBAL_MAILBOX_INTERFACE5 > > and this to repeat over subsequent groups of four bytes in the stream. > > This will be achieved by reading the stream using 32-bit little endian > accesses using get_unaligned_le32(), and then as you are already doing, > splitting them up into two 16-bit quantities. > > > For LE this doesn't matter but for BE they needs to be swapped as this > > is what mailbox expect. > > Correct. > > > For CRC. Would something like this work? > > > > Define u8 crc_data[4]; > > > > *crc_data = (__force u32)cpu_to_be32(word); > > That won't do what you want, it will only write the first byte. >
Right I'm stupid...
Just an example, the correct way would have been...
u8 crc_data[4]; u32 *crc_word; u32 word;
crc_word = (u32 *)crc_data; *crc_word = (__force u32)cpu_to_be32(word);
crc = crc_ccitt_false(crc, crc_data, sizeof(word));
> > crc = crc_ccitt_false(crc, crc_data, sizeof(word)); > > The point of explicitly assigning each byte is to ensure that it's > obvious that we'll get the right result. If we try to write a 32-bit > value, then we're getting right back into the "how does _this_ CPU > map a 32-bit value to indivudual bytes" endianness problem. > > The advantage of writing it out as bytes into a u8 array is that from > a code readability point of view, it's all laid out in plain sight > exactly which part of the 32-bit value ends up where and the order in > which the crc function is going to read those bytes - and it is > independent of whatever the endianess of the host architecture. >
It's just that I would really love to have a way to skip the shift maddness. If the code above is correct really don't know what is better... probably yours... just I don't like those shift.
I wonder... can't I reuse get_unaligned_be32((const u32 *)(data + pos)); ???
> > Using u8 array should keep the correct order no matter the endian and > > cpu_to_be32 should correctly swap the word if needed. (in a BE HOST data > > should already be in the right order and in LE has to be swapped right?) > > If you are absolutely certain that each group of four bytes in the > source bytestream need to be provided to the CRC function in the > reverse order to which they appear in the file. >
-- Ansuel
| |