> On Thu, Mar 07, 2024 at 05:02:29PM +0200, Peter Pentchev wrote: >> On Thu, Mar 07, 2024 at 03:25:01PM +0100, Manuel Traut wrote: >>> On 7 Feb 2024, at 18:27, Dima Kogan <dko...@debian.org> wrote: >>> >>> Hi. Thanks for your contribution. I looked at the upstream code a tiny >>> bit, and it looks like it might have portability bug, at least on >>> big-endian architectures. For instance: >>> >>> https://github.com/missinglinkelectronics/libuio/blob/6ef3d8d096a641686bfdd112035aa04aa16fe81a/irq.c#L78 >>> >>> This assumes that sizeof(long)==4. Maybe this is benign, but it would be >>> nice to fix. Are you upstream or do you know upstream? Can yall fix >>> these? >>> >> The kernel expects a 4 byte write here, since unsigned long is defined as at >> least 32 bit this shall work on all architectures. >> >> If your concern is about endianess this is not in the current scope of >> libuio and needs to be addressed by a higher layer. >> >> I am very familiar with library and in close contact with upstream. >> > In the case of uio_disable_irq() the bug is nicely hidden by the fact > that tmp is guaranteed to be all-zeroes. However, consider the previous > function in the file, uio_enable_irq(). If sizeof(unsigned long) == 8, > then the "tmp" variable's value of 1 will be encoded in memory as > 8 bytes containing the values 0, 0, 0, 0, 0, 0, 0, and 1 respectively. > When uio_enable_irq() calls write(..., &tmp, 4), it will only send > the first four bytes to the kernel - and they are 0, 0, 0, and... 0. > So it turns out that uio_disable_irq() and uio_enable_irq() do > exactly the same - send a 32-bit zero value to the kernel. > > ...of course, this will only happen on a big-endian system, as > Dima Kogan was concerned about. On a little-endian system, this > will work by chance. > > Is this the expected behavior indeed?
No it is not :) Thanks for the detailed explanation. Looks this fix sane to you? https://github.com/manut/libuio/commit/1edcf262fbfaf0b7f8519875ed5b357964dd1e55 I am not sure if users also expect an automatic conversion for the read/write functions: https://github.com/manut/libuio/commit/d0319169359bffc73374f1011e942702e9bafb1e It will be somehow inconsistent since a user could also access the device memory by pointer: https://github.com/manut/libuio/blob/add-ci/mem.c#L93 and than needs to take care on the endianess by its own anyway.
