Re: [PATCH v2 1/1] tty: serial: owl: Add support for kernel debugger
On Fri, Jan 08, 2021 at 08:58:38AM +0100, Jiri Slaby wrote:
> On 07. 01. 21, 19:16, Cristian Ciocaltea wrote:
> > Hi Greg,
> >
> > Thank you for the review!
> >
> > On Thu, Jan 07, 2021 at 04:20:55PM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Jan 05, 2021 at 07:02:02PM +0200, Cristian Ciocaltea wrote:
> > > > Implement 'poll_put_char' and 'poll_get_char' callbacks in struct
> > > > 'owl_uart_ops' that enables OWL UART to be used for kernel debugging
> > > > over serial line.
> > > >
> > > > Signed-off-by: Cristian Ciocaltea
> >
> > [...]
> >
> > > > +
> > > > +static void owl_uart_poll_put_char(struct uart_port *port, unsigned
> > > > char ch)
> > > > +{
> > > > + while (owl_uart_read(port, OWL_UART_STAT) & OWL_UART_STAT_TFFU)
> > > > + cpu_relax();
> > >
> > > Unbounded loops? What could possibly go wrong?
> > >
> > > :(
> > >
> > > Please don't do that in the kernel, put a max bound on this.
> >
> > I didn't realize the issue since I had encountered this pattern in many
> > other serial drivers, as well: altera_uart, arc_uart, atmel_serial, etc.
> >
> > > And are you _SURE_ that cpu_relax() is what you want to call here?
> >
> > I'm thinking of replacing the loop with 'readl_poll_timeout_atomic()',
> > if that would be a better approach.
>
> It might be better, yes. Either way, if you add a bound to the loop, you
> definitely need a more precise timing, so ndelay/udelay instead of
> cpu_relax.
I will use 1-5 us for the timing, but I'm not quite sure about the
overall timeout - 10 ms would suffice?
Thanks,
Cristi
> thanks,
> --
> js
Re: [PATCH v2 1/1] tty: serial: owl: Add support for kernel debugger
On 07. 01. 21, 19:16, Cristian Ciocaltea wrote:
Hi Greg,
Thank you for the review!
On Thu, Jan 07, 2021 at 04:20:55PM +0100, Greg Kroah-Hartman wrote:
On Tue, Jan 05, 2021 at 07:02:02PM +0200, Cristian Ciocaltea wrote:
Implement 'poll_put_char' and 'poll_get_char' callbacks in struct
'owl_uart_ops' that enables OWL UART to be used for kernel debugging
over serial line.
Signed-off-by: Cristian Ciocaltea
[...]
+
+static void owl_uart_poll_put_char(struct uart_port *port, unsigned char ch)
+{
+ while (owl_uart_read(port, OWL_UART_STAT) & OWL_UART_STAT_TFFU)
+ cpu_relax();
Unbounded loops? What could possibly go wrong?
:(
Please don't do that in the kernel, put a max bound on this.
I didn't realize the issue since I had encountered this pattern in many
other serial drivers, as well: altera_uart, arc_uart, atmel_serial, etc.
And are you _SURE_ that cpu_relax() is what you want to call here?
I'm thinking of replacing the loop with 'readl_poll_timeout_atomic()',
if that would be a better approach.
It might be better, yes. Either way, if you add a bound to the loop, you
definitely need a more precise timing, so ndelay/udelay instead of
cpu_relax.
thanks,
--
js
Re: [PATCH v2 1/1] tty: serial: owl: Add support for kernel debugger
Hi Greg,
Thank you for the review!
On Thu, Jan 07, 2021 at 04:20:55PM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 05, 2021 at 07:02:02PM +0200, Cristian Ciocaltea wrote:
> > Implement 'poll_put_char' and 'poll_get_char' callbacks in struct
> > 'owl_uart_ops' that enables OWL UART to be used for kernel debugging
> > over serial line.
> >
> > Signed-off-by: Cristian Ciocaltea
[...]
> > +
> > +static void owl_uart_poll_put_char(struct uart_port *port, unsigned char
> > ch)
> > +{
> > + while (owl_uart_read(port, OWL_UART_STAT) & OWL_UART_STAT_TFFU)
> > + cpu_relax();
>
> Unbounded loops? What could possibly go wrong?
>
> :(
>
> Please don't do that in the kernel, put a max bound on this.
I didn't realize the issue since I had encountered this pattern in many
other serial drivers, as well: altera_uart, arc_uart, atmel_serial, etc.
> And are you _SURE_ that cpu_relax() is what you want to call here?
I'm thinking of replacing the loop with 'readl_poll_timeout_atomic()',
if that would be a better approach.
Kind regards,
Cristi
> thanks,
>
> greg k-h
Re: [PATCH v2 1/1] tty: serial: owl: Add support for kernel debugger
On Tue, Jan 05, 2021 at 07:02:02PM +0200, Cristian Ciocaltea wrote:
> Implement 'poll_put_char' and 'poll_get_char' callbacks in struct
> 'owl_uart_ops' that enables OWL UART to be used for kernel debugging
> over serial line.
>
> Signed-off-by: Cristian Ciocaltea
> ---
> Changes in v2:
> - Reverted unnecessary changes per Andreas feedback
> - Optimized implementation for 'owl_uart_poll_get_char()'
>and 'owl_uart_poll_put_char()' callbacks
>
> drivers/tty/serial/owl-uart.c | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/tty/serial/owl-uart.c b/drivers/tty/serial/owl-uart.c
> index c149f8c30007..54b24669ebc5 100644
> --- a/drivers/tty/serial/owl-uart.c
> +++ b/drivers/tty/serial/owl-uart.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -461,6 +462,26 @@ static void owl_uart_config_port(struct uart_port *port,
> int flags)
> }
> }
>
> +#ifdef CONFIG_CONSOLE_POLL
> +
> +static int owl_uart_poll_get_char(struct uart_port *port)
> +{
> + if (owl_uart_read(port, OWL_UART_STAT) & OWL_UART_STAT_RFEM)
> + return NO_POLL_CHAR;
> +
> + return owl_uart_read(port, OWL_UART_RXDAT);
> +}
> +
> +static void owl_uart_poll_put_char(struct uart_port *port, unsigned char ch)
> +{
> + while (owl_uart_read(port, OWL_UART_STAT) & OWL_UART_STAT_TFFU)
> + cpu_relax();
Unbounded loops? What could possibly go wrong?
:(
Please don't do that in the kernel, put a max bound on this.
And are you _SURE_ that cpu_relax() is what you want to call here?
thanks,
greg k-h
[PATCH v2 1/1] tty: serial: owl: Add support for kernel debugger
Implement 'poll_put_char' and 'poll_get_char' callbacks in struct
'owl_uart_ops' that enables OWL UART to be used for kernel debugging
over serial line.
Signed-off-by: Cristian Ciocaltea
---
Changes in v2:
- Reverted unnecessary changes per Andreas feedback
- Optimized implementation for 'owl_uart_poll_get_char()'
and 'owl_uart_poll_put_char()' callbacks
drivers/tty/serial/owl-uart.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/drivers/tty/serial/owl-uart.c b/drivers/tty/serial/owl-uart.c
index c149f8c30007..54b24669ebc5 100644
--- a/drivers/tty/serial/owl-uart.c
+++ b/drivers/tty/serial/owl-uart.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -461,6 +462,26 @@ static void owl_uart_config_port(struct uart_port *port,
int flags)
}
}
+#ifdef CONFIG_CONSOLE_POLL
+
+static int owl_uart_poll_get_char(struct uart_port *port)
+{
+ if (owl_uart_read(port, OWL_UART_STAT) & OWL_UART_STAT_RFEM)
+ return NO_POLL_CHAR;
+
+ return owl_uart_read(port, OWL_UART_RXDAT);
+}
+
+static void owl_uart_poll_put_char(struct uart_port *port, unsigned char ch)
+{
+ while (owl_uart_read(port, OWL_UART_STAT) & OWL_UART_STAT_TFFU)
+ cpu_relax();
+
+ owl_uart_write(port, ch, OWL_UART_TXDAT);
+}
+
+#endif /* CONFIG_CONSOLE_POLL */
+
static const struct uart_ops owl_uart_ops = {
.set_mctrl = owl_uart_set_mctrl,
.get_mctrl = owl_uart_get_mctrl,
@@ -476,6 +497,10 @@ static const struct uart_ops owl_uart_ops = {
.request_port = owl_uart_request_port,
.release_port = owl_uart_release_port,
.verify_port = owl_uart_verify_port,
+#ifdef CONFIG_CONSOLE_POLL
+ .poll_get_char = owl_uart_poll_get_char,
+ .poll_put_char = owl_uart_poll_put_char,
+#endif
};
#ifdef CONFIG_SERIAL_OWL_CONSOLE
--
2.30.0
