Sebastian Huber commented: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1492#note_159740 Thanks, the goal is right, but the implementation does not reach it in the case which matters most. ### The release is a no-op for a caller which used `RTEMS_IODEV_IOCTL_OBTAIN` `iodev->mutex` is a recursive mutex (`rtems_recursive_mutex_init()` in `rtems_iodev_do_init()`), and `RTEMS_IODEV_IOCTL_OBTAIN` exists so that a client can hold the device across several `ioctl()` calls. | Step | Nesting count | | ---- | ------------- | | `ioctl(OBTAIN)`: obtain on entry, obtain in the case, release on exit | 1 | | `ioctl(EVENT_WAIT)`: obtain on entry | 2 | | the new `rtems_iodev_release()` | 1 | The count never reaches 0, so the mutex stays owned and every other thread still blocks in `rtems_iodev_ioctl()`. The fix works only for a client which holds no `OBTAIN`. For a client which does, the new sentence "The iodev lock is not held when event_wait is called" is false. An unlock loop is no answer, because the nesting depth belongs to the caller. A condition variable fixes this. RTEMS offers one in `<rtems/thread.h>`: `rtems_condition_variable_wait()` releases the mutex, blocks, and takes the mutex again on wake, and it does so atomically against `rtems_condition_variable_signal()`. That is exactly the release and re-obtain which the two added lines attempt by hand. It takes an `rtems_mutex`, not a recursive one, which is the point. Turn the device lock into a plain `rtems_mutex`, add a condition variable and an owner field to `struct rtems_iodev`, and carry the nesting of `RTEMS_IODEV_IOCTL_OBTAIN` in that field instead of in the mutex: - `RTEMS_IODEV_IOCTL_OBTAIN` waits on the condition variable until the device has no owner, then records the calling task. - `RTEMS_IODEV_IOCTL_RELEASE` clears the owner and signals. - `RTEMS_IODEV_IOCTL_EVENT_WAIT` clears the owner, signals, calls the `event_wait` handler with the mutex released, and claims the ownership again when the handler returns. The ownership then leaves the device for the full duration of the wait, whether or not the client called `RTEMS_IODEV_IOCTL_OBTAIN` before. The condition variable also gives `RTEMS_IODEV_IOCTL_RELEASE` a defined meaning for a task which is not the owner, which the recursive mutex does not. A smaller step is to handle `RTEMS_IODEV_IOCTL_EVENT_WAIT` outside the obtain/release region of `rtems_iodev_ioctl()` and to document that a client which holds the device through `OBTAIN` keeps it locked across the wait. That constraint then belongs in the `RTEMS_IODEV_IOCTL_OBTAIN` and `RTEMS_IODEV_IOCTL_EVENT_WAIT` blocks, not only in the `event_wait` member block. ### The new duty of the driver is not documented Before the change the mutex serialised every call into the driver. After it, two threads can sit inside the same `event_wait` at the same time, on the same event index, and a waiting thread runs concurrently with `get_event_info`, `get_regions` and any driver private state. The header states that the lock is absent. It does not state that the driver now protects its own state. The in-tree driver shows the hazard: `test_iodev_event_wait()` writes and reads the file-scope `tiodev` and calls `rtems_event_receive()` on the calling task. ### No test covers the new behaviour `iodev01` never has two tasks in the device at once, so nothing in the tree fails if the release goes away again. A cheap case: task A waits on event index 1 with no timeout, task B then runs `RTEMS_IODEV_IOCTL_EVENT_INFO` and must return. Repeat it with a preceding `RTEMS_IODEV_IOCTL_OBTAIN` in task A, which is the case which deadlocks B today. ### Doxygen form Every other `@brief` in `iodev.h` is one sentence. The two added sentences extend the brief instead of forming a detail paragraph: ```c /** * @brief Call to the device driver to wait on event. * * The iodev lock is not held during the call. The device driver * protects its own state. * * @param[in] iodev Pointer to iodev device. ``` ### Commit message The subject names `dev/io/iodev.c`, but the commit also changes `cpukit/include/dev/io/iodev.h`. The body opens with the edit instead of the problem. State the wrong behaviour, then the solution in the imperative: ``` dev/io: Do not hold the device lock during an event wait A thread which waited for an event held the device lock. Every other ioctl() call to the device blocked until the event arrived. Release the lock around the call to the event_wait handler of the driver. ``` ### Pre-existing, for a separate commit `rtems_iodev_event_wait_on()` returns -1 with `errno` set on its `EINVAL` and `ENODEV` paths, but `rtems_iodev_ioctl()` treats a non-zero return as an `errno` value and calls `rtems_set_errno_and_return_minus_one( err )`, so `errno` ends up as -1. `rtems_iodev_get_event_info()` and `rtems_iodev_get_region()` carry the same mismatch. -- View it on GitLab: https://gitlab.rtems.org/rtems/rtos/rtems/-/merge_requests/1492#note_159740 You're receiving this email because of your account on gitlab.rtems.org. Unsubscribe from this thread: https://gitlab.rtems.org/-/namespace/49/sent_notifications/5-cljrs19rbri1q9pabadmdgr2w-1d/unsubscribe | Manage all notifications: https://gitlab.rtems.org/-/profile/notifications | Help: https://gitlab.rtems.org/help
_______________________________________________ bugs mailing list [email protected] http://lists.rtems.org/mailman/listinfo/bugs
