On Wed, Mar 13, 2024 at 10:40:51AM +0000, Traut Manuel LCPF-CH wrote: > > 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 <dkogan@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 Yeah, after I sent the e-mails I realized that I forgot to mention that using uint32_t would be one of the best ways to fix that. Thanks for the fast reaction! > 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. ...and this part I have no opinion about. I don't have any realistic idea about how people actually use libuio (or how they will use it), and I do not think that I will use it soon, so this should be left for others to decide. Thanks again for paying attention to the comments! G'luck, Peter -- Peter Pentchev roam@ringlet.net roam@debian.org pp@storpool.com PGP key: http://people.FreeBSD.org/~roam/roam.key.asc Key fingerprint 2EE7 A7A5 17FC 124C F115 C354 651E EFB0 2527 DF13
Attachment:
signature.asc
Description: PGP signature