Aren/haiku
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

generic/tty: restore lock sharing between master and slave TTYs

Browse Source 
The code in this module was derived from the one in driver/tty. However,
the driver uses a shared lock between the master and slave side of a
TTY, and this was changed to use two separate locks. The approach with
two locks does not work. It seems the change was unfinished and the
second TTY was never locked. But attempting to lock it will result in
lock inversion problems, unless we do complicated things (try to find
which of the two TTY is the master side, and lock that first, for
example). It is simpler to restore the shared lock as used in the
driver.

To set up the shared lock, I modified the tty_create function to take a
pointer to the master TTY when creating the slave. Maybe it makes more
sense to create both sides in the same call, create_tty_pair?

However, this does not work as easily as I wanted, because there is some
recursion going on: at least in one case, the tty_control function is
calling the driver's tty_service function, which in turns attempts to
call back into tty_control for the "other side" TTY. To handle this
case, replace the mutex with a recursive_lock.

Fixes #17091, where the root problem was access to
other_tty->select_pool without locking. This was also made unconvenient
to debug because select_pool objects are self-deleting, when the last
item in the pool is removed. As a result, the code accessing it without
log would suddenly find out that the data it was accessing had been
freed and erased.

This also makes the TTY code in driver/tty and generic/tty a bit more
similar than it was before, and brings us one step closer to merging the
two together. There are still two main differences and I don't know
enough about TTY to decide if they are important, and which version
should be kept:
- The driver has extra code for "background" read and write. I don't
  know what this is used for.
- The driver has a single "settings" instance shared by a master and
  slave TTY, while the module has two separate instances, but seems to
  copy one to the other. I'm not sure which approach is correct.

Change-Id: Ie2daddd027859ce32ba395af76b4f109f8b984b0
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4604
Reviewed-by: waddlesplash <waddlesplash@gmail.com>
This commit is contained in:
Adrien Destugues 2021-10-18 14:19:36 +02:00 committed by waddlesplash
parent c0ec37dcbc
commit 0f6f5adb50
6 changed files with 47 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
headers/os/drivers/tty/tty_module.h
View File

@ -46,7 +46,7 @@ typedef struct tty_module_info tty_module_info;
struct tty_module_info {
module_info mi;
struct tty *(*tty_create)(tty_service_func serviceFunction, bool isMaster);
struct tty *(*tty_create)(tty_service_func serviceFunction, struct tty* master);
void (*tty_destroy)(struct tty *tty);
struct tty_cookie *

4
src/add-ons/kernel/drivers/ports/pc_serial/SerialDevice.cpp
View File

@ -545,13 +545,13 @@ SerialDevice::Open(uint32 flags)
if (fDeviceRemoved)
return B_DEV_NOT_READY;
fMasterTTY = gTTYModule->tty_create(pc_serial_service, true);
fMasterTTY = gTTYModule->tty_create(pc_serial_service, NULL);
if (fMasterTTY == NULL) {
TRACE_ALWAYS("open: failed to init master tty\n");
return B_NO_MEMORY;
}
fSlaveTTY = gTTYModule->tty_create(pc_serial_service, false);
fSlaveTTY = gTTYModule->tty_create(pc_serial_service, fMasterTTY);
if (fSlaveTTY == NULL) {
TRACE_ALWAYS("open: failed to init slave tty\n");
gTTYModule->tty_destroy(fMasterTTY);

4
src/add-ons/kernel/drivers/ports/usb_serial/SerialDevice.cpp
View File

@ -292,13 +292,13 @@ SerialDevice::Open(uint32 flags)
if (fDeviceRemoved)
return B_DEV_NOT_READY;
fMasterTTY = gTTYModule->tty_create(usb_serial_service, true);
fMasterTTY = gTTYModule->tty_create(usb_serial_service, NULL);
if (fMasterTTY == NULL) {
TRACE_ALWAYS("open: failed to init master tty\n");
return B_NO_MEMORY;
}
fSlaveTTY = gTTYModule->tty_create(usb_serial_service, false);
fSlaveTTY = gTTYModule->tty_create(usb_serial_service, fMasterTTY);
if (fSlaveTTY == NULL) {
TRACE_ALWAYS("open: failed to init slave tty\n");
gTTYModule->tty_destroy(fMasterTTY);

10
src/add-ons/kernel/drivers/tty/tty.cpp
View File

@ -1477,13 +1477,11 @@ tty_close_cookie(struct tty_cookie* cookie)
// finally close the tty
tty_close(cookie->tty);
// notify a select write event on the other tty, if we've closed this tty
if (cookie->other_tty->open_count > 0)
tty_notify_select_event(cookie->other_tty, B_SELECT_WRITE);
}
// notify pending select()s and cleanup the select sync pool
// notify a select write event on the other tty, if we've closed this tty
if (cookie->tty->open_count == 0 && cookie->other_tty->open_count > 0)
tty_notify_select_event(cookie->other_tty, B_SELECT_WRITE);
}

48
src/add-ons/kernel/generic/tty/tty.cpp
View File

@ -96,9 +96,9 @@ public:
protected:
void Lock()
{ mutex_lock(&fCookie->tty->lock); }
{ recursive_lock_lock(fCookie->tty->lock); }
void Unlock()
{ mutex_unlock(&fCookie->tty->lock); }
{ recursive_lock_unlock(fCookie->tty->lock); }
tty_cookie* fCookie;
size_t fBytes;
@ -834,6 +834,13 @@ tty_readable(struct tty* tty)
}
/** \brief Notify anyone waiting on events for this TTY.
*
* The TTY lock must be held.
*
* Otherwise, the select_pool may be modified or deleted (which happens automatically when the last
* item is removed from it).
*/
static void
tty_notify_select_event(struct tty* tty, uint8 event)
{
@ -1266,25 +1273,39 @@ tty_write_to_tty_slave(tty_cookie* sourceCookie, const void* _buffer,
struct tty*
tty_create(tty_service_func func, bool isMaster)
tty_create(tty_service_func func, struct tty* master)
{
struct tty* tty = new(std::nothrow) (struct tty);
if (tty == NULL)
return NULL;
mutex_init(&tty->lock, "tty lock");
if (master == NULL) {
tty->is_master = true;
tty->lock = new(std::nothrow) recursive_lock;
if (tty->lock == NULL) {
delete tty;
return NULL;
}
recursive_lock_init(tty->lock, "tty lock");
} else {
tty->is_master = false;
tty->lock = master->lock;
}
tty->ref_count = 0;
tty->open_count = 0;
tty->opened_count = 0;
tty->select_pool = NULL;
tty->is_master = isMaster;
tty->pending_eof = 0;
tty->hardware_bits = 0;
reset_tty_settings(tty->settings);
if (init_line_buffer(tty->input_buffer, TTY_BUFFER_SIZE) < B_OK) {
if (tty->is_master) {
recursive_lock_destroy(tty->lock);
delete tty->lock;
}
delete tty;
return NULL;
}
@ -1301,7 +1322,10 @@ tty_destroy(struct tty* tty)
TRACE(("tty_destroy(%p)\n", tty));
uninit_line_buffer(tty->input_buffer);
delete_select_sync_pool(tty->select_pool);
mutex_destroy(&tty->lock);
if (tty->is_master) {
recursive_lock_destroy(tty->lock);
delete tty->lock;
}
delete tty;
}
@ -1327,7 +1351,7 @@ tty_create_cookie(struct tty* tty, struct tty* otherTTY, uint32 openMode)
cookie->closed = false;
MutexLocker locker(cookie->tty->lock);
RecursiveLocker locker(cookie->tty->lock);
// add to the TTY's cookie list
tty->cookies.Add(cookie);
@ -1379,7 +1403,7 @@ tty_close_cookie(tty_cookie* cookie)
// For the removal of the cookie acquire the TTY's lock. This ensures, that
// cookies will not be removed from a TTY (or added -- cf. add_tty_cookie())
// as long as the TTY's lock is being held.
MutexLocker ttyLocker(cookie->tty->lock);
RecursiveLocker ttyLocker(cookie->tty->lock);
// remove the cookie from the TTY's cookie list
cookie->tty->cookies.Remove(cookie);
@ -1439,7 +1463,7 @@ tty_close_cookie(tty_cookie* cookie)
void
tty_destroy_cookie(tty_cookie* cookie)
{
MutexLocker locker(cookie->tty->lock);
RecursiveLocker locker(cookie->tty->lock);
cookie->tty->ref_count--;
locker.Unlock();
@ -1582,7 +1606,7 @@ tty_control(tty_cookie* cookie, uint32 op, void* buffer, size_t length)
TRACE(("tty_ioctl: tty %p, op %" B_PRIu32 ", buffer %p, length %"
B_PRIuSIZE "\n", tty, op, buffer, length));
MutexLocker locker(tty->lock);
RecursiveLocker locker(tty->lock);
switch (op) {
// blocking/non-blocking mode
@ -1798,7 +1822,7 @@ tty_select(tty_cookie* cookie, uint8 event, uint32 ref, selectsync* sync)
// lock the TTY (allows us to freely access the cookie lists of this and
// the other TTY)
MutexLocker ttyLocker(tty->lock);
RecursiveLocker ttyLocker(tty->lock);
// get the other TTY -- needed for `write' events
struct tty* otherTTY = cookie->other_tty;
@ -1871,7 +1895,7 @@ tty_deselect(tty_cookie* cookie, uint8 event, selectsync* sync)
return B_BAD_VALUE;
// lock the TTY (guards the select sync pool, among other things)
MutexLocker ttyLocker(tty->lock);
RecursiveLocker ttyLocker(tty->lock);
return remove_select_sync_pool_entry(&tty->select_pool, sync, event);
}

4
src/add-ons/kernel/generic/tty/tty_private.h
View File

@ -137,7 +137,7 @@ struct tty {
tty_service_func service_func;
uint32 pending_eof;
bool is_master;
struct mutex lock;
recursive_lock* lock;
tty_settings settings;
uint8 hardware_bits;
};
@ -146,7 +146,7 @@ struct tty {
extern struct mutex gTTYCookieLock;
extern struct recursive_lock gTTYRequestLock;
extern struct tty *tty_create(tty_service_func func, bool isMaster);
extern struct tty *tty_create(tty_service_func func, struct tty* masterTTY);
extern void tty_destroy(struct tty *tty);
extern tty_cookie *tty_create_cookie(struct tty *tty, struct tty *otherTTY,