| Message ID | 20090801-patch-01.tilman@imap.cc |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, 1 Aug 2009 14:18:51 +0200 (CEST) Tilman Schmidt <tilman@imap.cc> wrote: > This patch corrects the problem noted but left unfixed by commit > 23198fda7182969b619613a555f8645fdc3dc334 "tty: fix chars_in_buffers". > The tty_operation chars_in_buffer() should return zero, not a > negative value, in the error case. Is that correct. If you take a signal so the mutex_lock_interruptible takes the error path we'll exit any waits for characters to be flushed from buffers and lose the bytes in some cases ? Thats why I didn't convert it - I didn't understand why it was using _interruptible at all ? Alan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alan, thanks a lot for getting in contact with me, and for your explanation of the rationale behind your patch. On Sat, 1 Aug 2009 17:11:41 +0100, Alan Cox wrote: > Is that correct. If you take a signal so the mutex_lock_interruptible > takes the error path we'll exit any waits for characters to be flushed > from buffers and lose the bytes in some cases ? You are right. I'll have to change it to use non-interruptible mutex_lock. I'll prepare a new patch. > Thats why I didn't > convert it - I didn't understand why it was using _interruptible at all ? No specific reason. just the general rule to prefer sleeping interuptibly whenever possible, because unkillable processes are so annoying. Seeing that chars_in_buffer was declared as returning int, I assumed (wrongly as it now turns out) that it would be allowed to return the customary negative values for errors, so that it could use the _interruptible variant. When I saw your patch I read into it that zero would be the appropriate return value for all error cases, including a signal while waiting for the mutex. I guess there was a bit of wishful thinking in that. Well, thanks again for your help. I'll follow up with a revised patch. Regards, Tilman
diff --git a/drivers/isdn/gigaset/interface.c b/drivers/isdn/gigaset/interface.c index 8b62df8..dfeec68 100644 --- a/drivers/isdn/gigaset/interface.c +++ b/drivers/isdn/gigaset/interface.c @@ -408,33 +408,29 @@ static int if_write_room(struct tty_struct *tty) return retval; } -/* FIXME: This function does not have error returns */ - static int if_chars_in_buffer(struct tty_struct *tty) { struct cardstate *cs; - int retval = -ENODEV; + int retval = 0; cs = (struct cardstate *) tty->driver_data; if (!cs) { pr_err("%s: no cardstate\n", __func__); - return -ENODEV; + return 0; } gig_dbg(DEBUG_IF, "%u: %s()", cs->minor_index, __func__); if (mutex_lock_interruptible(&cs->mutex)) - return -ERESTARTSYS; // FIXME -EINTR? + return 0; - if (!cs->connected) { + if (!cs->connected) gig_dbg(DEBUG_IF, "not connected"); - retval = -ENODEV; - } else if (!cs->open_count) + else if (!cs->open_count) dev_warn(cs->dev, "%s: device not opened\n", __func__); - else if (cs->mstate != MS_LOCKED) { + else if (cs->mstate != MS_LOCKED) dev_warn(cs->dev, "can't write to unlocked device\n"); - retval = -EBUSY; - } else + else retval = cs->ops->chars_in_buffer(cs); mutex_unlock(&cs->mutex);
This patch corrects the problem noted but left unfixed by commit 23198fda7182969b619613a555f8645fdc3dc334 "tty: fix chars_in_buffers". The tty_operation chars_in_buffer() should return zero, not a negative value, in the error case. Impact: error handling bugfix Signed-off-by: Tilman Schmidt <tilman@imap.cc> --- I only just noticed the "tty: fix chars_in_buffers" patch going in. Please merge this follow-up fix for 2.6.31, too. Thanks. drivers/isdn/gigaset/interface.c | 18 +++++++----------- 1 files changed, 7 insertions(+), 11 deletions(-)