| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| hverkuil | arnd: ping | [12:24] |
| arnd | hi hverkuil | [12:25] |
| hverkuil | Just a quick question w.r.t. struct timespec. | [12:25] |
| arnd | I'm still trying to come up with a nice way to handle VIDIOC_DQEVENT, I saw multiple problems in my patch from yesterday, but will have something later today I hope | [12:25] |
| hverkuil | arnd: don't do anything, I'm working on this. | [12:26] |
| arnd | ok
what about timespec then? | [12:26] |
| hverkuil | Never mind :-) I just found the answer myself.
I was wondering whether tv_nsec was 32 or 64 bit when it is converted to y2038 for 32 bit systems. But it is a long. only tv_sec changes type. | [12:27] |
| arnd | right, that is the tricky part, in particular when we want to use the same code for native and compat mode on 64-bit kernels
in compat mode, you should assume that there is uninitialized padding in the upper 32 bits this means to if we ever pass a timespec into the kernel, we have to either clear the upper bits in a compat handler (since the in-kernel representation has a 64-bit nanosecond on a 64-bit kernel), but have to test them for being zero in native mode. AFAICT, v4l never needs that though, it only passed timespec from kernel to user space, and then the correct behavior is to pretend that it is a 64-bit member and set the upper bits to zero before copy_to_user, so we don't leak kernel stack data | [12:28] |
| ...................... (idle for 1h46mn) | ||
| hverkuil | arnd: I'm looking at drivers/media/v4l2-core/v4l2-compat-ioctl32.c, put_v4l2_event32()
struct v4l2_event32 uses struct compat_timespec. There does not appear to be a struct compat_timespec64, so I assume I need to use struct timespec64? | [14:17] |
| arnd | hverkuil: the version of v4l2_event with 64-bit timestamps should not need any compat handling at all | [14:20] |
| hverkuil | Ah, that's true. | [14:21] |
| arnd | struct timespec64 must not be used in uapi files, since the nanosecond field is in the wrong place on 32-bit architectures. open-coding a __s64/__s64 structure was the best I could come up with here for the in-kernel usage | [14:21] |
| hverkuil | But I will need to do this for v4l2_buffer which has a compat_timeval. | [14:22] |
| arnd | right
looking again, I do see a problem for x86: alignof(struct v4l2_event_ctrl) is '4' on x86-32, but '8' on all other architectures (both 32-bit and 64-bit) I guess we can work around that by adding a 'char __pad[sizeof(time_t) - sizeof(long)];' between v4l2_event::type and v4l2_event::u, but that's really ugly | [14:23] |
| hverkuil | Argh, this is confusing. So yes, I do need compat32 code for v4l2_event using a y2038 timespec. | [14:26] |
| arnd | only on x86, and only if you don't want to include the __pad hack | [14:27] |
| hverkuil | So if I have to make a struct v4l2_event32_y2038 that has a layout according to the new y2038-capable timespec, then what should I use to replace "struct compat timespec"? | [14:28] |
| arnd | and only if we can guarantee that all applications that are build against a latest glibc use a linux/videodev2.h that is new enough. This works if they get the one from the system headers (glibc will require a minimal kernel header version), but not if they ship their own copy
in my patch, I just use __s32/__s32 for the old layout , and __s64/__s64 for the new layout inside of #ifdef __KERNEL__ | [14:28] |
| hverkuil | You lost me now. | [14:30] |
| arnd | sorry, I was still talking about the padding question above, let me start over. This is what I'd do in the header file:
https://www.irccloud.com/pastebin/dbJUnWPU/ | [14:30] |
| hverkuil | (I'm not planning to mess around with padding, it's already complex enough without doing hacks) | [14:31] |
| arnd | the kernel now sees two structures that are always defined in terms of __u32, __s32, __s64 etc, so they are fixed size, while user space sees the same structure as before (defined in terms of timeval from glibc) | [14:33] |
| hverkuil | Isn't there a third layout? Identical to struct v4l2_event32, except tv_sec is __s64. I.e. a 32 bit application that uses the new y2038-safe timespec and that calls a 64 bit kernel. | [14:33] |
| arnd | ideally there are only two possible layouts (ignoring the x86-32 special case for now) | [14:35] |
| hverkuil | but I can't ignore that special case. It's there.
Am I missing something? | [14:35] |
| arnd | I just want to make sure we have the same understanding for the simple architectures at first, and then decide on how to solve the x86 case
(still thinking about it here, after realizing I messed up the alignment in v4l2_event32) | [14:36] |
| hverkuil | I'm close to posting my first version of this. It might help to wait until you see my approach. | [14:43] |
| arnd | do you want to discuss it on the list then, or send me that patch in private first and keep talking here?
here is a new version of the struct definition that I think should cover all cases: https://www.irccloud.com/pastebin/z3ahDsWO/ the __attribute__((aligned(sizeof(time_t)))) makes sure that the definition seen by user space is either the traditional layout with 32-bit time_t, or it has both thet 64-bit time_t and the alignment we see on 64-bit architectures hverkuil: that way we only need two structure layouts: one for compat mode and 32-bit kernels with the old layout, and one for native mode on 64-bit architectures and 32-bit with the new layout | [14:44] |
| hverkuil | I remember that there was a wiki or LWN article about y2038. Do you have links handy? | [15:02] |
| arnd | hverkuil: it's all fairly old now, https://kernelnewbies.org/y2038 is the best entry point we have, it links to a few other things
https://sourceware.org/glibc/wiki/Y2038ProofnessDesign has some more information from the user space perspective | [15:09] |
| hverkuil | thanks | [15:10] |
| arnd: so to be 100% certain I understand this: there are three variants of struct timespec and struct timeval (I'm speaking from point of view of userspace here):
For a 64 bit application both fields are 64-bit values, so no change there. For existing 32 bit applications both fields are 32-bit value. For the future glibc y2038-safe version the seconds field will be 64 bit and the nsecs/usecs field is 32 bit. Is that correct? | [15:21] | |
| arnd | yes, but it's worth pointing out here that the nsecs/usecs field on 32-bit architectures is in the same bit position as the lower 32 bits of the 64-bit nsecs/usecs field on 64-bit architectures. The difference between the first and the third format is whether the upper bits are defined to zeroes or have undefined contents
in almost all cases we can pretend they are the same | [15:24] |
| hverkuil | that's true for 64-bit little endian architectures, not for big-endian, correct? | [15:26] |
| arnd | no, for both
see https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#struct_timespec the definition for big-endian has some extra twists here, but the intention is to make it compatible with the kernel's view | [15:28] |
| hverkuil | oh, they add padding in the right place to make it work. Got it.
Is there a guarantee that the high 32 bits of nsecs/usecs are 0? | [15:30] |
| arnd | the kernel must guarantee that they are zero for data it passes to user space | [15:32] |
| hverkuil | and in the other direction? (to the kernel) | [15:32] |
| arnd | glibc won't guarantee that they are zero for data passed into the kernel, so the one place that the kernel cares about the difference is the function to copy a timespec from user space into kernel space | [15:32] |
| hverkuil | Ah, got it. | [15:33] |
| arnd | fortunately, very few drivers do it | [15:33] |
| hverkuil | That situation is not relevant for v4l2_event, but it is for v4l2_buffer. | [15:33] |
| arnd | ok, I see | [15:33] |
| hverkuil | Thank you for your help. I need to rework my patches a bit since I hadn't realized this fact about the padding. | [15:34] |
| arnd | strictly speaking, if you interpret a timeval from user space, the driver should check if tv_usec is at most 999999 microseconds and positive
so on 64-bit user space, you must do that check before clearing the upper bits, while for 32-bit user space we must clear the upper bits and then check deepa has proposed a get_timespec64() function that does this correctly, but it hasn't made it into the 4.15 merge window if you don't check the tv_usec value on input today, you can probably get away with always clearing the upper bits | [15:34] |
| hverkuil | There are actually just two use-cases for the timestamp in v4l2_buffer: either it is set by the kernel, or for memory-to-memory devices it is just copied from source to destination and it is not actually used inside the driver. | [15:37] |
| arnd | I see. so you don't check the validity of the timestamp in the kernel because the kernel never uses it? | [15:38] |
| hverkuil | right. | [15:38] |
| .... (idle for 19mn) | ||
| I'm going home. I'll see if I can work on this a bit more this weekend. Thank you for your help, this was very informative. | [15:57] | |
| arnd | thank you for working on it! | [15:58] |
| *** | benjiG has left
prabhakarlad has left | [16:01] |
| .................................................................... (idle for 5h37mn) | ||
| pinky | anyone into DVB-s? wondering what "the best" USB receiver is these days for use with linux, something with hardware blind scan | [21:39] |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |