| Message ID | 1480953058-4958-1-git-send-email-alexander.stein@systec-electronic.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
Alexander Stein wrote: > - schedule_timeout(2); > + schedule_timeout(msecs_to_jiffies(2)); NACK. So I don't remember why I wrote this code, but I don't think I was expecting it to be 2ms. Instead, I think I just wanted it to be some delay, but I believed that schedule_timeout(1) was too short or would be "optimized" out somehow. Note that right below this, I do: if (qe_port->wait_closing) { /* Wait a bit longer */ set_current_state(TASK_UNINTERRUPTIBLE); schedule_timeout(qe_port->wait_closing); } And wait_closing is a number of jiffies, so I knew that schedule_timeout() took jiffies as a parameter. So I think I'm going to NACK this patch, since I believe I knew what I was doing when I wrote it five years ago.
On Monday 05 December 2016 10:04:27, Timur Tabi wrote: > Alexander Stein wrote: > > - schedule_timeout(2); > > + schedule_timeout(msecs_to_jiffies(2)); > > NACK. > > So I don't remember why I wrote this code, but I don't think I was > expecting it to be 2ms. Instead, I think I just wanted it to be some > delay, but I believed that schedule_timeout(1) was too short or would be > "optimized" out somehow. > > Note that right below this, I do: > > if (qe_port->wait_closing) { > /* Wait a bit longer */ > set_current_state(TASK_UNINTERRUPTIBLE); > schedule_timeout(qe_port->wait_closing); > } > > And wait_closing is a number of jiffies, so I knew that > schedule_timeout() took jiffies as a parameter. > > So I think I'm going to NACK this patch, since I believe I knew what I > was doing when I wrote it five years ago. Okay, I was just wondering why the timeout is dependant on the timer tick. That didn't seem obvious to me. Rethinking about this, I would rather replace those lines with msleep instead. Best regards, Alexander
Alexander Stein wrote: > Okay, I was just wondering why the timeout is dependant on the timer tick. > That didn't seem obvious to me. > Rethinking about this, I would rather replace those lines with msleep instead. What's wrong with leaving it as-is? The code is five years old, and Freescale/NXP barely uses the QE any more. I don't have access to any hardware to test any changes you would propose.
diff --git a/drivers/tty/serial/ucc_uart.c b/drivers/tty/serial/ucc_uart.c index 481eb29..c6c01a4 100644 --- a/drivers/tty/serial/ucc_uart.c +++ b/drivers/tty/serial/ucc_uart.c @@ -827,7 +827,7 @@ static void qe_uart_shutdown(struct uart_port *port) break; } set_current_state(TASK_UNINTERRUPTIBLE); - schedule_timeout(2); + schedule_timeout(msecs_to_jiffies(2)); } if (qe_port->wait_closing) {
schedule_timeout takes a timeout in jiffies resolution. So pass 2ms as a converted jiffies value. This makes the timeout independent of CONFIG_HZ. Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com> --- drivers/tty/serial/ucc_uart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)