16550: Fix crossed timeouts in rt_16550_write
PATCH 1/1 16550: Fix crossed timeouts in rt_16550_write
rt_16550_write currently initializes its timeout sequence using rx_timeout. Because xnsynch_acquire ignores the value of timeout (apart from making sure it's positive) when it is passed a non NULL timeout sequence, this means that serial writes are using the read timeout, except for when the user sets a zero tx timeout. (Should xnsynch_acquire be changed to (timeout + current time) to *timeout_seq to decide which will lead to timing out sooner? Seems more consistent but might break existing code...)
This changes the initialization of the timeout sequence to use tx_timeout. I also changed the first call to rtdm_mutex_timedlock to use tx_timeout, since this more accurately reflects what is happening.
*******************
ksrc/drivers/serial/16550A.c +2 -2
@@ -979,10 +979,10 @@ ssize_t rt_16550_write(struct rtdm_dev_context * context, ctx = (struct rt_16550_context *)context->dev_private; -rtdm_toseq_init (&timeout_seq, ctx->config.rx_timeout); +rtdm_toseq_init (&timeout_seq, ctx->config.tx_timeout);/* Make write operation atomic. */ - ret =rtdm_mutex_timedlock (&ctx->out_lock, ctx->config.rx_timeout, + ret =rtdm_mutex_timedlock (&ctx->out_lock, ctx->config.tx_timeout, &timeout_seq);if (ret)return ret;
*******************
Signature · 1 line
Doug Brunner
Quoted 5 lines · Doug Brunner
rt_16550_writecurrently initializes its timeout sequence usingrx_timeout. Becausexnsynch_acquireignores the value of timeout (apart from making sure it's positive) when it is passed a non NULL timeout sequence, this means that serial writes are using the read timeout, except for when the user sets a zero tx timeout.
I would make the description shorter: We accidentally use rx_timeout instead of tx_timeout in parts of rt_dev_write. This fixes the obvious copy&paste mistake.
Quoted 4 lines
(Should
xnsynch_acquire
be changed to (timeout + current time) to*timeout_seqto decide which will lead to timing out sooner? Seems more consistent but might break existing code...)
That's controlled by timeout_mode (XN_RELATIVE vs. XN_ABSOLUTE).
Quoted 6 lines
This changes the initialization of the timeout sequence to use
tx_timeout. I also changed the first call tortdm_mutex_timedlockto usetx_timeout, since this more accurately reflects what is happening.*******************
Use "---" as separator - or git format-patch.
Quoted 20 lines
ksrc/drivers/serial/16550A.c +2
-2@@ -979,10 +979,10 @@ ssize_t rt_16550_write(struct rtdm_dev_context * context, ctx = (struct rt_16550_context *)context->dev_private; -rtdm_toseq_init (&timeout_seq, ctx->config.rx_timeout); +rtdm_toseq_init (&timeout_seq, ctx->config.tx_timeout);/* Make write operation atomic. */ - ret =rtdm_mutex_timedlock (&ctx->out_lock, ctx->config.rx_timeout, + ret =rtdm_mutex_timedlock (&ctx->out_lock, ctx->config.tx_timeout, &timeout_seq);if (ret)return ret;
Quoted 1 line
*******************
This line may confuse "git am" while applying, but I didn't test.
Anyway, the patch content is fine.
Thanks,
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Quoted 42 lines · Jan Kiszka
Quoted 5 lines · Doug Brunner
rt_16550_writecurrently initializes its timeout sequence usingrx_timeout. Becausexnsynch_acquireignores the value of timeout (apart from making sure it's positive) when it is passed a non NULL timeout sequence, this means that serial writes are using the read timeout, except for when the user sets a zero tx timeout.I would make the description shorter: We accidentally use
rx_timeoutinstead oftx_timeoutin parts ofrt_dev_write. This fixes the obvious copy&paste mistake.Quoted 4 lines
(Should
xnsynch_acquire
be changed to (timeout + current time) to*timeout_seqto decide which will lead to timing out sooner? Seems more consistent but might break existing code...)That's controlled by
timeout_mode(XN_RELATIVEvs.XN_ABSOLUTE).Quoted 5 lines
This changes the initialization of the timeout sequence to use
tx_timeout. I also changed the first call tortdm_mutex_timedlockto usetx_timeout, since this more accurately reflects what is happening.*******************
Use "---" as separator - or git format-patch.
Quoted 19 lines
ksrc/drivers/serial/16550A.c +2
-2@@ -979,10 +979,10 @@ ssize_t rt_16550_write(struct rtdm_dev_context * context, ctx = (struct rt_16550_context *)context->dev_private; -rtdm_toseq_init (&timeout_seq, ctx->config.rx_timeout); +rtdm_toseq_init (&timeout_seq, ctx->config.tx_timeout);/* Make write operation atomic. */ - ret =rtdm_mutex_timedlock (&ctx->out_lock, ctx->config.rx_timeout, + ret =rtdm_mutex_timedlock (&ctx->out_lock, ctx->config.tx_timeout, &timeout_seq);if (ret)return ret;
Quoted 8 lines
Quoted 1 line
*******************
This line may confuse "git am" while applying, but I didn't test.
Anyway, the patch content is fine.
Thanks,
Jan
Thanks for the info on formatting Jan. Next time I will get it right! :)
I meant to say, should rtdm_mutex_timedlock compare (timeout + now) to *timeout_seq to decide which one times out sooner, or require that either (a) timeout == 0 && timeout_seq != NULL (to use timeout_seq) or (b) timeout_seq == NULL (to use timeout? This would make the meaning of a call to rtdm_mutex_timedlock more obvious (and might have led to catching this bug earlier), but would break code that has relied on the non-obvious behavior that currently exists.
--Doug Brunner