* [RESEND PATCH 04/14] serial: lantiq: Change ltq_w32_mask to asc_update_bits
From: Songjun Wu @ 2018-10-16 9:19 UTC (permalink / raw)
To: yixin.zhu, chuanhua.lei, hauke.mehrtens
Cc: gregkh, paul.burton, jslaby, Songjun Wu, linux-kernel,
linux-serial
In-Reply-To: <20181016091915.19909-1-songjun.wu@linux.intel.com>
ltq prefix is platform specific function, asc prefix
is more generic.
Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
---
drivers/tty/serial/lantiq.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
index 66c671677761..4c14608b8ef8 100644
--- a/drivers/tty/serial/lantiq.c
+++ b/drivers/tty/serial/lantiq.c
@@ -113,6 +113,13 @@ struct ltq_uart_port {
unsigned int err_irq;
};
+static inline void asc_update_bits(u32 clear, u32 set, void __iomem *reg)
+{
+ u32 tmp = readl(reg);
+
+ writel((tmp & ~clear) | set, reg);
+}
+
static inline struct
ltq_uart_port *to_ltq_uart_port(struct uart_port *port)
{
@@ -163,16 +170,16 @@ lqasc_rx_chars(struct uart_port *port)
if (rsr & ASCSTATE_ANY) {
if (rsr & ASCSTATE_PE) {
port->icount.parity++;
- ltq_w32_mask(0, ASCWHBSTATE_CLRPE,
+ asc_update_bits(0, ASCWHBSTATE_CLRPE,
port->membase + LTQ_ASC_WHBSTATE);
} else if (rsr & ASCSTATE_FE) {
port->icount.frame++;
- ltq_w32_mask(0, ASCWHBSTATE_CLRFE,
+ asc_update_bits(0, ASCWHBSTATE_CLRFE,
port->membase + LTQ_ASC_WHBSTATE);
}
if (rsr & ASCSTATE_ROE) {
port->icount.overrun++;
- ltq_w32_mask(0, ASCWHBSTATE_CLRROE,
+ asc_update_bits(0, ASCWHBSTATE_CLRROE,
port->membase + LTQ_ASC_WHBSTATE);
}
@@ -252,7 +259,7 @@ lqasc_err_int(int irq, void *_port)
struct uart_port *port = (struct uart_port *)_port;
spin_lock_irqsave(<q_asc_lock, flags);
/* clear any pending interrupts */
- ltq_w32_mask(0, ASCWHBSTATE_CLRPE | ASCWHBSTATE_CLRFE |
+ asc_update_bits(0, ASCWHBSTATE_CLRPE | ASCWHBSTATE_CLRFE |
ASCWHBSTATE_CLRROE, port->membase + LTQ_ASC_WHBSTATE);
spin_unlock_irqrestore(<q_asc_lock, flags);
return IRQ_HANDLED;
@@ -304,7 +311,7 @@ lqasc_startup(struct uart_port *port)
clk_enable(ltq_port->clk);
port->uartclk = clk_get_rate(ltq_port->fpiclk);
- ltq_w32_mask(ASCCLC_DISS | ASCCLC_RMCMASK, (1 << ASCCLC_RMCOFFSET),
+ asc_update_bits(ASCCLC_DISS | ASCCLC_RMCMASK, (1 << ASCCLC_RMCOFFSET),
port->membase + LTQ_ASC_CLC);
ltq_w32(0, port->membase + LTQ_ASC_PISEL);
@@ -320,7 +327,7 @@ lqasc_startup(struct uart_port *port)
* setting enable bits
*/
wmb();
- ltq_w32_mask(0, ASCCON_M_8ASYNC | ASCCON_FEN | ASCCON_TOEN |
+ asc_update_bits(0, ASCCON_M_8ASYNC | ASCCON_FEN | ASCCON_TOEN |
ASCCON_ROEN, port->membase + LTQ_ASC_CON);
retval = request_irq(ltq_port->tx_irq, lqasc_tx_int,
@@ -364,9 +371,9 @@ lqasc_shutdown(struct uart_port *port)
free_irq(ltq_port->err_irq, port);
ltq_w32(0, port->membase + LTQ_ASC_CON);
- ltq_w32_mask(ASCRXFCON_RXFEN, ASCRXFCON_RXFFLU,
+ asc_update_bits(ASCRXFCON_RXFEN, ASCRXFCON_RXFFLU,
port->membase + LTQ_ASC_RXFCON);
- ltq_w32_mask(ASCTXFCON_TXFEN, ASCTXFCON_TXFFLU,
+ asc_update_bits(ASCTXFCON_TXFEN, ASCTXFCON_TXFFLU,
port->membase + LTQ_ASC_TXFCON);
if (!IS_ERR(ltq_port->clk))
clk_disable(ltq_port->clk);
@@ -438,7 +445,7 @@ lqasc_set_termios(struct uart_port *port,
spin_lock_irqsave(<q_asc_lock, flags);
/* set up CON */
- ltq_w32_mask(0, con, port->membase + LTQ_ASC_CON);
+ asc_update_bits(0, con, port->membase + LTQ_ASC_CON);
/* Set baud rate - take a divider of 2 into account */
baud = uart_get_baud_rate(port, new, old, 0, port->uartclk / 16);
@@ -446,19 +453,19 @@ lqasc_set_termios(struct uart_port *port,
divisor = divisor / 2 - 1;
/* disable the baudrate generator */
- ltq_w32_mask(ASCCON_R, 0, port->membase + LTQ_ASC_CON);
+ asc_update_bits(ASCCON_R, 0, port->membase + LTQ_ASC_CON);
/* make sure the fractional divider is off */
- ltq_w32_mask(ASCCON_FDE, 0, port->membase + LTQ_ASC_CON);
+ asc_update_bits(ASCCON_FDE, 0, port->membase + LTQ_ASC_CON);
/* set up to use divisor of 2 */
- ltq_w32_mask(ASCCON_BRS, 0, port->membase + LTQ_ASC_CON);
+ asc_update_bits(ASCCON_BRS, 0, port->membase + LTQ_ASC_CON);
/* now we can write the new baudrate into the register */
ltq_w32(divisor, port->membase + LTQ_ASC_BG);
/* turn the baudrate generator back on */
- ltq_w32_mask(0, ASCCON_R, port->membase + LTQ_ASC_CON);
+ asc_update_bits(0, ASCCON_R, port->membase + LTQ_ASC_CON);
/* enable rx */
ltq_w32(ASCWHBSTATE_SETREN, port->membase + LTQ_ASC_WHBSTATE);
--
2.11.0
^ permalink raw reply related
* [RESEND PATCH 03/14] serial: lantiq: Get serial id from dts
From: Songjun Wu @ 2018-10-16 9:19 UTC (permalink / raw)
To: yixin.zhu, chuanhua.lei, hauke.mehrtens
Cc: gregkh, paul.burton, jslaby, Songjun Wu, linux-kernel,
linux-serial
In-Reply-To: <20181016091915.19909-1-songjun.wu@linux.intel.com>
Get serial id from dts, also keep backward compatible when dts is not
updated.
Signed-off-by: Songjun Wu <songjun.wu@linux.intel.com>
---
drivers/tty/serial/lantiq.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/lantiq.c b/drivers/tty/serial/lantiq.c
index 044128277248..66c671677761 100644
--- a/drivers/tty/serial/lantiq.c
+++ b/drivers/tty/serial/lantiq.c
@@ -688,7 +688,7 @@ lqasc_probe(struct platform_device *pdev)
struct ltq_uart_port *ltq_port;
struct uart_port *port;
struct resource *mmres, irqres[3];
- int line = 0;
+ int line;
int ret;
mmres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -699,9 +699,20 @@ lqasc_probe(struct platform_device *pdev)
return -ENODEV;
}
- /* check if this is the console port */
- if (mmres->start != CPHYSADDR(LTQ_EARLY_ASC))
- line = 1;
+ /* get serial id */
+ line = of_alias_get_id(node, "serial");
+ if (line < 0) {
+ if (IS_ENABLED(CONFIG_LANTIQ)) {
+ if (mmres->start == CPHYSADDR(LTQ_EARLY_ASC))
+ line = 0;
+ else
+ line = 1;
+ } else {
+ dev_err(&pdev->dev, "failed to get alias id, errno %d\n",
+ line);
+ return line;
+ }
+ }
if (lqasc_port[line]) {
dev_err(&pdev->dev, "port %d already allocated\n", line);
--
2.11.0
^ permalink raw reply related
* [RESEND PATCH 00/14] serial: lantiq: Add CCF suppport
From: Songjun Wu @ 2018-10-16 9:19 UTC (permalink / raw)
To: yixin.zhu, chuanhua.lei, hauke.mehrtens
Cc: gregkh, paul.burton, jslaby, Songjun Wu, devicetree, linux-serial,
James Hogan, linux-mips, linux-kernel, Thomas Gleixner,
Philippe Ombredanne, Rob Herring, Kate Stewart, Mark Rutland,
Ralf Baechle
This patch series is for adding common clock framework support
for lantiq serial driver, mainly includes:
1) Add common clock framework support.
2) Modify the dts file according to the DT conventions.
3) Replace the platform dependent functions with kernel functions
Songjun Wu (14):
MIPS: dts: Change upper case to lower case
MIPS: dts: Add aliases node for lantiq danube serial
serial: lantiq: Get serial id from dts
serial: lantiq: Change ltq_w32_mask to asc_update_bits
MIPS: lantiq: Unselect SWAP_IO_SPACE when LANTIQ is selected
serial: lantiq: Use readl/writel instead of ltq_r32/ltq_w32
serial: lantiq: Rename fpiclk to freqclk
serial: lantiq: Replace clk_enable/clk_disable with clk generic API
serial: lantiq: Add CCF support
serial: lantiq: Reorder the head files
include: Add lantiq.h in include/linux/
serial: lantiq: Replace lantiq_soc.h with lantiq.h
serial: lantiq: Change init_lqasc to static declaration
dt-bindings: serial: lantiq: Add optional properties for CCF
.../devicetree/bindings/serial/lantiq_asc.txt | 15 +++
arch/mips/Kconfig | 1 -
arch/mips/boot/dts/lantiq/danube.dtsi | 42 +++---
arch/mips/boot/dts/lantiq/easy50712.dts | 18 ++-
drivers/tty/serial/lantiq.c | 145 ++++++++++++---------
include/linux/lantiq.h | 23 ++++
6 files changed, 155 insertions(+), 89 deletions(-)
create mode 100644 include/linux/lantiq.h
--
2.11.0
^ permalink raw reply
* Re: [PATCH 00/14] serial: langtiq: Add CCF suppport
From: Wu, Songjun @ 2018-10-16 9:05 UTC (permalink / raw)
To: Paul Burton
Cc: yixin.zhu@linux.intel.com, chuanhua.lei@linux.intel.com,
hauke.mehrtens@intel.com, devicetree@vger.kernel.org,
linux-serial@vger.kernel.org, James Hogan,
linux-mips@linux-mips.org, Jiri Slaby,
linux-kernel@vger.kernel.org, Thomas Gleixner,
Philippe Ombredanne, Rob Herring, Kate Stewart,
Greg Kroah-Hartman, Mark Rutland, Ralf Baechle
In-Reply-To: <20181015215845.5t7bkyks6hsxqwsb@pburton-laptop>
On 10/16/2018 5:58 AM, Paul Burton wrote:
> Hi Songjun,
>
> On Mon, Sep 24, 2018 at 06:27:49PM +0800, Songjun Wu wrote:
>> This patch series is for adding common clock framework support
>> for langtiq serial driver, mainly includes:
> s/langtiq/lantiq/ ...
Thanks, it will be fixed.
>> 1) Add common clock framework support.
>> 2) Modify the dts file according to the DT conventions.
>> 3) Replace the platform dependent functions with kernel functions
>>
>> Songjun Wu (14):
>> MIPS: dts: Change upper case to lower case
>> MIPS: dts: Add aliases node for lantiq danube serial
>> serial: lantiq: Get serial id from dts
>> serial: lantiq: Change ltq_w32_mask to asc_update_bits
>> MIPS: lantiq: Unselect SWAP_IO_SPACE when LANTIQ is selected
>> serial: lantiq: Use readl/writel instead of ltq_r32/ltq_w32
>> serial: lantiq: Rename fpiclk to freqclk
>> serial: lantiq: Replace clk_enable/clk_disable with clk generic API
>> serial: lantiq: Add CCF support
>> serial: lantiq: Reorder the head files
>> include: Add lantiq.h in include/linux/
>> serial: lantiq: Replace lantiq_soc.h with lantiq.h
>> serial: lantiq: Change init_lqasc to static declaration
>> dt-bindings: serial: lantiq: Add optional properties for CCF
> It appears that you only copied me on patches 1, 2 & 5. I've applied
> patch 1 to mips-next for 4.20, but I have no clue whether your other
> patches were deemed acceptable by serial or DT maintainers & I have no
> context for the changes being made, so I can neither apply nor ack
> patches 2 & 5. Please copy me on the whole series next time.
>
> Thanks,
> Paul
Thanks.
I will resend the patches and cc all the patches to you.
^ permalink raw reply
* Re: [RFC][PATCHv2 0/4] less deadlock prone serial consoles
From: Sergey Senozhatsky @ 2018-10-16 8:12 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Sergey Senozhatsky, linux-kernel, Petr Mladek, Steven Rostedt,
Daniel Wang, Andrew Morton, Linus Torvalds, Greg Kroah-Hartman,
Alan Cox, Jiri Slaby, Peter Feiner, linux-serial,
Sergey Senozhatsky
In-Reply-To: <20181016072328.GA4030@hirez.programming.kicks-ass.net>
On (10/16/18 09:23), Peter Zijlstra wrote:
> On Tue, Oct 16, 2018 at 02:04:24PM +0900, Sergey Senozhatsky wrote:
> > Hello,
> >
> >
> > RFC
> >
> >
> > The patch set reduces the number of ways serial consoles
> > can deadlock the system, including the recently reported deadlock
> > in panic().
>
> That's what I use early_printk for... :-)
I've been waiting for this comment ;-)
-ss
^ permalink raw reply
* Re: [RFC][PATCHv2 2/4] printk: move printk_safe macros to printk header
From: Peter Zijlstra @ 2018-10-16 7:27 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: linux-kernel, Petr Mladek, Steven Rostedt, Daniel Wang,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky
In-Reply-To: <20181016050428.17966-3-sergey.senozhatsky@gmail.com>
On Tue, Oct 16, 2018 at 02:04:26PM +0900, Sergey Senozhatsky wrote:
> Make printk_safe_enter_irqsave()/etc macros available to the
> rest of the kernel.
>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
> include/linux/printk.h | 40 +++++++++++++++++++++++++++++++++++++
> kernel/printk/internal.h | 37 ----------------------------------
> kernel/printk/printk_safe.c | 6 ++++--
> 3 files changed, 44 insertions(+), 39 deletions(-)
>
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index cf3eccfe1543..75f99441fd54 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -157,6 +157,46 @@ static inline void printk_nmi_direct_enter(void) { }
> static inline void printk_nmi_direct_exit(void) { }
> #endif /* PRINTK_NMI */
>
> +#ifdef CONFIG_PRINTK
> +extern void printk_safe_enter(void);
> +extern void printk_safe_exit(void);
> +
> +#define printk_safe_enter_irqsave(flags) \
> + do { \
> + local_irq_save(flags); \
> + printk_safe_enter(); \
> + } while (0)
> +
> +#define printk_safe_exit_irqrestore(flags) \
> + do { \
> + printk_safe_exit(); \
> + local_irq_restore(flags); \
> + } while (0)
> +
> +#define printk_safe_enter_irq() \
> + do { \
> + local_irq_disable(); \
> + printk_safe_enter(); \
> + } while (0)
> +
> +#define printk_safe_exit_irq() \
> + do { \
> + printk_safe_exit(); \
> + local_irq_enable(); \
> + } while (0)
So I really _really_ hate all this. Utterly detest it.
That whole magic 'safe' printk buffer crap is just that crap.
Instead of this tinkering around the edges, why don't you make the main
logbuf a lockless ringbuffer and then delegate the actual printing of
that buffer to a kthread, except for earlycon, which can do synchronous
output.
^ permalink raw reply
* Re: [RFC][PATCHv2 0/4] less deadlock prone serial consoles
From: Peter Zijlstra @ 2018-10-16 7:23 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: linux-kernel, Petr Mladek, Steven Rostedt, Daniel Wang,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky
In-Reply-To: <20181016050428.17966-1-sergey.senozhatsky@gmail.com>
On Tue, Oct 16, 2018 at 02:04:24PM +0900, Sergey Senozhatsky wrote:
> Hello,
>
>
> RFC
>
>
> The patch set reduces the number of ways serial consoles
> can deadlock the system, including the recently reported deadlock
> in panic().
That's what I use early_printk for... :-)
^ permalink raw reply
* [RFC][PATCHv2 4/4] tty: 8250: switch to uart_port locking helpers
From: Sergey Senozhatsky @ 2018-10-16 5:04 UTC (permalink / raw)
To: linux-kernel
Cc: Petr Mladek, Steven Rostedt, Daniel Wang, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky,
Sergey Senozhatsky
In-Reply-To: <20181016050428.17966-1-sergey.senozhatsky@gmail.com>
Switch to uart_port_lock macros:
- use uart_port_lock_for_printk()/uart_port_unlock_after_printk() in
serial8250_console_write().
- use uart_port_lock_*()/uart_port_unlock_*() elsewhere.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
drivers/tty/serial/8250/8250_core.c | 8 +--
drivers/tty/serial/8250/8250_dma.c | 4 +-
drivers/tty/serial/8250/8250_port.c | 81 +++++++++++++----------------
3 files changed, 43 insertions(+), 50 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 94f3e1c64490..34fe546260f6 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -268,7 +268,7 @@ static void serial8250_backup_timeout(struct timer_list *t)
unsigned int iir, ier = 0, lsr;
unsigned long flags;
- spin_lock_irqsave(&up->port.lock, flags);
+ uart_port_lock_irqsave(&up->port, flags);
/*
* Must disable interrupts or else we risk racing with the interrupt
@@ -302,7 +302,7 @@ static void serial8250_backup_timeout(struct timer_list *t)
if (up->port.irq)
serial_out(up, UART_IER, ier);
- spin_unlock_irqrestore(&up->port.lock, flags);
+ uart_port_unlock_irqrestore(&up->port, flags);
/* Standard timer interval plus 0.2s to keep the port running */
mod_timer(&up->timer,
@@ -1078,9 +1078,9 @@ void serial8250_unregister_port(int line)
if (uart->em485) {
unsigned long flags;
- spin_lock_irqsave(&uart->port.lock, flags);
+ uart_port_lock_irqsave(&uart->port, flags);
serial8250_em485_destroy(uart);
- spin_unlock_irqrestore(&uart->port.lock, flags);
+ uart_port_unlock_irqrestore(&uart->port, flags);
}
uart_remove_one_port(&serial8250_reg, &uart->port);
diff --git a/drivers/tty/serial/8250/8250_dma.c b/drivers/tty/serial/8250/8250_dma.c
index bfa1a857f3ff..a9255b4e9353 100644
--- a/drivers/tty/serial/8250/8250_dma.c
+++ b/drivers/tty/serial/8250/8250_dma.c
@@ -22,7 +22,7 @@ static void __dma_tx_complete(void *param)
dma_sync_single_for_cpu(dma->txchan->device->dev, dma->tx_addr,
UART_XMIT_SIZE, DMA_TO_DEVICE);
- spin_lock_irqsave(&p->port.lock, flags);
+ uart_port_lock_irqsave(&p->port, flags);
dma->tx_running = 0;
@@ -39,7 +39,7 @@ static void __dma_tx_complete(void *param)
serial_port_out(&p->port, UART_IER, p->ier);
}
- spin_unlock_irqrestore(&p->port.lock, flags);
+ uart_port_unlock_irqrestore(&p->port, flags);
}
static void __dma_rx_complete(void *param)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index f776b3eafb96..340aecf23dc5 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -790,9 +790,9 @@ static void enable_rsa(struct uart_8250_port *up)
{
if (up->port.type == PORT_RSA) {
if (up->port.uartclk != SERIAL_RSA_BAUD_BASE * 16) {
- spin_lock_irq(&up->port.lock);
+ uart_port_lock_irq(&up->port);
__enable_rsa(up);
- spin_unlock_irq(&up->port.lock);
+ uart_port_unlock_irq(&up->port);
}
if (up->port.uartclk == SERIAL_RSA_BAUD_BASE * 16)
serial_out(up, UART_RSA_FRR, 0);
@@ -812,7 +812,7 @@ static void disable_rsa(struct uart_8250_port *up)
if (up->port.type == PORT_RSA &&
up->port.uartclk == SERIAL_RSA_BAUD_BASE * 16) {
- spin_lock_irq(&up->port.lock);
+ uart_port_lock_irq(&up->port);
mode = serial_in(up, UART_RSA_MSR);
result = !(mode & UART_RSA_MSR_FIFO);
@@ -825,7 +825,7 @@ static void disable_rsa(struct uart_8250_port *up)
if (result)
up->port.uartclk = SERIAL_RSA_BAUD_BASE_LO * 16;
- spin_unlock_irq(&up->port.lock);
+ uart_port_unlock_irq(&up->port);
}
}
#endif /* CONFIG_SERIAL_8250_RSA */
@@ -1236,7 +1236,7 @@ static void autoconfig(struct uart_8250_port *up)
* We really do need global IRQs disabled here - we're going to
* be frobbing the chips IRQ enable register to see if it exists.
*/
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(port, flags);
up->capabilities = 0;
up->bugs = 0;
@@ -1275,7 +1275,7 @@ static void autoconfig(struct uart_8250_port *up)
/*
* We failed; there's nothing here
*/
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(port, flags);
DEBUG_AUTOCONF("IER test failed (%02x, %02x) ",
scratch2, scratch3);
goto out;
@@ -1299,7 +1299,7 @@ static void autoconfig(struct uart_8250_port *up)
status1 = serial_in(up, UART_MSR) & 0xF0;
serial8250_out_MCR(up, save_mcr);
if (status1 != 0x90) {
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(port, flags);
DEBUG_AUTOCONF("LOOP test failed (%02x) ",
status1);
goto out;
@@ -1372,7 +1372,7 @@ static void autoconfig(struct uart_8250_port *up)
serial_out(up, UART_IER, 0);
out_lock:
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(port, flags);
/*
* Check if the device is a Fintek F81216A
@@ -1483,12 +1483,12 @@ static enum hrtimer_restart serial8250_em485_handle_stop_tx(struct hrtimer *t)
p = em485->port;
serial8250_rpm_get(p);
- spin_lock_irqsave(&p->port.lock, flags);
+ uart_port_lock_irqsave(&p->port, flags);
if (em485->active_timer == &em485->stop_tx_timer) {
__do_stop_tx_rs485(p);
em485->active_timer = NULL;
}
- spin_unlock_irqrestore(&p->port.lock, flags);
+ uart_port_unlock_irqrestore(&p->port, flags);
serial8250_rpm_put(p);
return HRTIMER_NORESTART;
}
@@ -1637,12 +1637,12 @@ static enum hrtimer_restart serial8250_em485_handle_start_tx(struct hrtimer *t)
em485 = container_of(t, struct uart_8250_em485, start_tx_timer);
p = em485->port;
- spin_lock_irqsave(&p->port.lock, flags);
+ uart_port_lock_irqsave(&p->port, flags);
if (em485->active_timer == &em485->start_tx_timer) {
__start_tx(&p->port);
em485->active_timer = NULL;
}
- spin_unlock_irqrestore(&p->port.lock, flags);
+ uart_port_unlock_irqrestore(&p->port, flags);
return HRTIMER_NORESTART;
}
@@ -1884,7 +1884,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
if (iir & UART_IIR_NO_INT)
return 0;
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(port, flags);
status = serial_port_in(port, UART_LSR);
@@ -1897,7 +1897,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
if ((!up->dma || up->dma->tx_err) && (status & UART_LSR_THRE))
serial8250_tx_chars(up);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(port, flags);
return 1;
}
EXPORT_SYMBOL_GPL(serial8250_handle_irq);
@@ -1932,9 +1932,9 @@ static int serial8250_tx_threshold_handle_irq(struct uart_port *port)
if ((iir & UART_IIR_ID) == UART_IIR_THRI) {
struct uart_8250_port *up = up_to_u8250p(port);
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(port, flags);
serial8250_tx_chars(up);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(port, flags);
}
iir = serial_port_in(port, UART_IIR);
@@ -1949,10 +1949,10 @@ static unsigned int serial8250_tx_empty(struct uart_port *port)
serial8250_rpm_get(up);
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(port, flags);
lsr = serial_port_in(port, UART_LSR);
up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(port, flags);
serial8250_rpm_put(up);
@@ -2025,13 +2025,13 @@ static void serial8250_break_ctl(struct uart_port *port, int break_state)
unsigned long flags;
serial8250_rpm_get(up);
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(port, flags);
if (break_state == -1)
up->lcr |= UART_LCR_SBC;
else
up->lcr &= ~UART_LCR_SBC;
serial_port_out(port, UART_LCR, up->lcr);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(port, flags);
serial8250_rpm_put(up);
}
@@ -2282,7 +2282,7 @@ int serial8250_do_startup(struct uart_port *port)
* the interrupt is enabled. Delays are necessary to
* allow register changes to become visible.
*/
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(port, flags);
if (up->port.irqflags & IRQF_SHARED)
disable_irq_nosync(port->irq);
@@ -2298,7 +2298,7 @@ int serial8250_do_startup(struct uart_port *port)
if (port->irqflags & IRQF_SHARED)
enable_irq(port->irq);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(port, flags);
/*
* If the interrupt is not reasserted, or we otherwise
@@ -2320,7 +2320,7 @@ int serial8250_do_startup(struct uart_port *port)
*/
serial_port_out(port, UART_LCR, UART_LCR_WLEN8);
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(port, flags);
if (up->port.flags & UPF_FOURPORT) {
if (!up->port.irq)
up->port.mctrl |= TIOCM_OUT1;
@@ -2367,7 +2367,7 @@ int serial8250_do_startup(struct uart_port *port)
}
dont_test_tx_en:
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(port, flags);
/*
* Clear the interrupt registers again for luck, and clear the
@@ -2434,17 +2434,17 @@ void serial8250_do_shutdown(struct uart_port *port)
/*
* Disable interrupts from this port
*/
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(port, flags);
up->ier = 0;
serial_port_out(port, UART_IER, 0);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(port, flags);
synchronize_irq(port->irq);
if (up->dma)
serial8250_release_dma(up);
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(port, flags);
if (port->flags & UPF_FOURPORT) {
/* reset interrupts on the AST Fourport board */
inb((port->iobase & 0xfe0) | 0x1f);
@@ -2453,7 +2453,7 @@ void serial8250_do_shutdown(struct uart_port *port)
port->mctrl &= ~TIOCM_OUT2;
serial8250_set_mctrl(port, port->mctrl);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(port, flags);
/*
* Disable break condition and FIFOs
@@ -2679,7 +2679,7 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
* interrupts disabled.
*/
serial8250_rpm_get(up);
- spin_lock_irqsave(&port->lock, flags);
+ uart_port_lock_irqsave(port, flags);
up->lcr = cval; /* Save computed LCR */
@@ -2783,7 +2783,7 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios,
serial_port_out(port, UART_FCR, up->fcr); /* set fcr */
}
serial8250_set_mctrl(port, port->mctrl);
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_irqrestore(port, flags);
serial8250_rpm_put(up);
/* Don't rewrite B0 */
@@ -2806,15 +2806,15 @@ void serial8250_do_set_ldisc(struct uart_port *port, struct ktermios *termios)
{
if (termios->c_line == N_PPS) {
port->flags |= UPF_HARDPPS_CD;
- spin_lock_irq(&port->lock);
+ uart_port_lock_irq(port);
serial8250_enable_ms(port);
- spin_unlock_irq(&port->lock);
+ uart_port_unlock_irq(port);
} else {
port->flags &= ~UPF_HARDPPS_CD;
if (!UART_ENABLE_MS(port, termios->c_cflag)) {
- spin_lock_irq(&port->lock);
+ uart_port_lock_irq(port);
serial8250_disable_ms(port);
- spin_unlock_irq(&port->lock);
+ uart_port_unlock_irq(port);
}
}
}
@@ -3252,19 +3252,13 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
struct uart_port *port = &up->port;
unsigned long flags;
unsigned int ier;
- int locked = 1;
+ bool locked;
touch_nmi_watchdog();
serial8250_rpm_get(up);
- if (port->sysrq)
- locked = 0;
- else if (oops_in_progress)
- locked = spin_trylock_irqsave(&port->lock, flags);
- else
- spin_lock_irqsave(&port->lock, flags);
-
+ uart_port_lock_for_printk(port, flags, locked);
/*
* First save the IER then disable the interrupts
*/
@@ -3300,8 +3294,7 @@ void serial8250_console_write(struct uart_8250_port *up, const char *s,
if (up->msr_saved_flags)
serial8250_modem_status(up);
- if (locked)
- spin_unlock_irqrestore(&port->lock, flags);
+ uart_port_unlock_after_printk(port, flags, locked);
serial8250_rpm_put(up);
}
--
2.19.1
^ permalink raw reply related
* [RFC][PATCHv2 3/4] serial: introduce uart_port locking helpers
From: Sergey Senozhatsky @ 2018-10-16 5:04 UTC (permalink / raw)
To: linux-kernel
Cc: Petr Mladek, Steven Rostedt, Daniel Wang, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky,
Sergey Senozhatsky
In-Reply-To: <20181016050428.17966-1-sergey.senozhatsky@gmail.com>
The patch introduces several macros which should make serial console
drivers less printk() deadlock prone.
There are several console driver entry points so we are looking at
several slightly different deadlock scenarios.
- The first entry point is console ->write() callback, which we call
from printk(). A possible deadlock scenario there is:
CPU0
<NMI>
spin_lock_irqsave(&port->lock, flags) << deadlock
serial_foo_write()
call_console_drivers()
console_unlock()
console_flush_on_panic()
panic()
<NMI/>
spin_lock_irqsave(&port->lock, flags)
serial_foo_write()
call_console_drivers()
console_unlock()
printk()
...
->write() callback, generally speaking, must be re-entrant in two cases:
1) sysrq
2) panic (oops_in_progress)
And this is what uart_port_lock_for_printk()/uart_port_unlock_after_printk()
macros are for.
Usage example:
static serial_foo_write(...)
{
struct uart_port *port;
unsigned long flags;
bool locked;
uart_port_lock_for_printk(port, flags, locked);
uart_console_write(port, s, count, serial_foo_putchar);
uart_port_unlock_after_printk(port, flags, locked);
}
Some of the serial drivers already use _some sort_ of
uart_port_lock_for_printk(), some are not panic() re-entrant. This
should address the issue.
- The rest (of entry points) requires a bit different handling.
Let's take a look at the following backtrace:
CPU0
<IRQ>
spin_lock_irqsave(&port->lock, flags) << deadlock
serial_foo_write()
call_console_drivers()
console_unlock()
printk()
__queue_work()
tty_flip_buffer_push()
spin_lock_irqsave(&port->lock, flags)
serial_foo_handle_IRQ()
<IRQ/>
Serial drivers invoke tons of core kernel functions - WQ, MM, etc. All
of which may printk() in various cases. So we can't really just
"remove those printk-s". The simples way to address this seems to be
PRINTK_SAFE_CONTEXT_MASK.
printk_safe() is a special printk() mode, which redirects recursive
printk()-s to a secondary (per-CPU) buffer; so we don't re-enter
printk() and serial console driver. The secondary (per-CPU) buffer is
flushed and printed to the consoles later, from a safe context, when
we know that we are not in printk()-recursion anymore.
And this is what uart_port_lock_*()/uart_port_unlock_*() macros are
for. With uart_port_lock_irqsave() the previous example would turn
into:
CPU0
<IRQ>
irq_work_queue()
printk_safe_log_store()
printk()
__queue_work()
tty_flip_buffer_push()
uart_port_lock_irqsave(port, flags)
serial_foo_handle_IRQ()
<IRQ/>
As of now, no consoles are re-entrant when printk() initiated by
console's driver IRQ handler. This macro should address the issue.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
include/linux/serial_core.h | 48 +++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 047fa67d039b..acc6966fea8e 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -266,6 +266,54 @@ struct uart_port {
void *private_data; /* generic platform data pointer */
};
+#define uart_port_lock_irq(p) \
+ do { \
+ printk_safe_enter_irq(); \
+ spin_lock(&(p)->lock); \
+ } while (0)
+
+#define uart_port_unlock_irq(p) \
+ do { \
+ spin_unlock(&(p)->lock); \
+ printk_safe_exit_irq(); \
+ } while (0)
+
+#define uart_port_lock_irqsave(p, flags) \
+ do { \
+ printk_safe_enter_irqsave(flags); \
+ spin_lock(&(p)->lock); \
+ } while (0)
+
+#define uart_port_unlock_irqrestore(p, flags) \
+ do { \
+ spin_unlock(&(p)->lock); \
+ printk_safe_exit_irqrestore(flags); \
+ } while (0)
+
+#if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(SUPPORT_SYSRQ)
+#define uart_port_in_sysrq(p) p->sysrq
+#else
+#define uart_port_in_sysrq(p) 0
+#endif
+
+#define uart_port_lock_for_printk(p, flags, locked) \
+ do { \
+ locked = true; \
+ if (uart_port_in_sysrq(p)) \
+ locked = false; \
+ else if (oops_in_progress) \
+ locked = spin_trylock_irqsave(&(p)->lock, \
+ flags); \
+ else \
+ spin_lock_irqsave(&(p)->lock, flags); \
+ } while (0)
+
+#define uart_port_unlock_after_printk(p, flags, locked) \
+ do { \
+ if (locked) \
+ spin_unlock_irqrestore(&(p)->lock, flags); \
+ } while (0)
+
static inline int serial_port_in(struct uart_port *up, int offset)
{
return up->serial_in(up, offset);
--
2.19.1
^ permalink raw reply related
* [RFC][PATCHv2 2/4] printk: move printk_safe macros to printk header
From: Sergey Senozhatsky @ 2018-10-16 5:04 UTC (permalink / raw)
To: linux-kernel
Cc: Petr Mladek, Steven Rostedt, Daniel Wang, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky,
Sergey Senozhatsky
In-Reply-To: <20181016050428.17966-1-sergey.senozhatsky@gmail.com>
Make printk_safe_enter_irqsave()/etc macros available to the
rest of the kernel.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
include/linux/printk.h | 40 +++++++++++++++++++++++++++++++++++++
kernel/printk/internal.h | 37 ----------------------------------
kernel/printk/printk_safe.c | 6 ++++--
3 files changed, 44 insertions(+), 39 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index cf3eccfe1543..75f99441fd54 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -157,6 +157,46 @@ static inline void printk_nmi_direct_enter(void) { }
static inline void printk_nmi_direct_exit(void) { }
#endif /* PRINTK_NMI */
+#ifdef CONFIG_PRINTK
+extern void printk_safe_enter(void);
+extern void printk_safe_exit(void);
+
+#define printk_safe_enter_irqsave(flags) \
+ do { \
+ local_irq_save(flags); \
+ printk_safe_enter(); \
+ } while (0)
+
+#define printk_safe_exit_irqrestore(flags) \
+ do { \
+ printk_safe_exit(); \
+ local_irq_restore(flags); \
+ } while (0)
+
+#define printk_safe_enter_irq() \
+ do { \
+ local_irq_disable(); \
+ printk_safe_enter(); \
+ } while (0)
+
+#define printk_safe_exit_irq() \
+ do { \
+ printk_safe_exit(); \
+ local_irq_enable(); \
+ } while (0)
+#else
+/*
+ * On !PRINTK builds we still export console output related locks
+ * and some functions (console_unlock()/tty/etc.), so printk-safe
+ * must preserve the existing local IRQ guarantees.
+ */
+#define printk_safe_enter_irqsave(flags) local_irq_save(flags)
+#define printk_safe_exit_irqrestore(flags) local_irq_restore(flags)
+
+#define printk_safe_enter_irq() local_irq_disable()
+#define printk_safe_exit_irq() local_irq_enable()
+#endif
+
#ifdef CONFIG_PRINTK
asmlinkage __printf(5, 0)
int vprintk_emit(int facility, int level,
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 0f1898820cba..03c10f361a15 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -32,32 +32,6 @@ int vprintk_store(int facility, int level,
__printf(1, 0) int vprintk_default(const char *fmt, va_list args);
__printf(1, 0) int vprintk_deferred(const char *fmt, va_list args);
__printf(1, 0) int vprintk_func(const char *fmt, va_list args);
-void __printk_safe_enter(void);
-void __printk_safe_exit(void);
-
-#define printk_safe_enter_irqsave(flags) \
- do { \
- local_irq_save(flags); \
- __printk_safe_enter(); \
- } while (0)
-
-#define printk_safe_exit_irqrestore(flags) \
- do { \
- __printk_safe_exit(); \
- local_irq_restore(flags); \
- } while (0)
-
-#define printk_safe_enter_irq() \
- do { \
- local_irq_disable(); \
- __printk_safe_enter(); \
- } while (0)
-
-#define printk_safe_exit_irq() \
- do { \
- __printk_safe_exit(); \
- local_irq_enable(); \
- } while (0)
void defer_console_output(void);
@@ -65,15 +39,4 @@ void defer_console_output(void);
__printf(1, 0) int vprintk_func(const char *fmt, va_list args) { return 0; }
-/*
- * In !PRINTK builds we still export logbuf_lock spin_lock, console_sem
- * semaphore and some of console functions (console_unlock()/etc.), so
- * printk-safe must preserve the existing local IRQ guarantees.
- */
-#define printk_safe_enter_irqsave(flags) local_irq_save(flags)
-#define printk_safe_exit_irqrestore(flags) local_irq_restore(flags)
-
-#define printk_safe_enter_irq() local_irq_disable()
-#define printk_safe_exit_irq() local_irq_enable()
-
#endif /* CONFIG_PRINTK */
diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
index 0913b4d385de..367fcb2c39f0 100644
--- a/kernel/printk/printk_safe.c
+++ b/kernel/printk/printk_safe.c
@@ -359,16 +359,18 @@ static __printf(1, 0) int vprintk_safe(const char *fmt, va_list args)
}
/* Can be preempted by NMI. */
-void __printk_safe_enter(void)
+void printk_safe_enter(void)
{
this_cpu_inc(printk_context);
}
+EXPORT_SYMBOL_GPL(printk_safe_enter);
/* Can be preempted by NMI. */
-void __printk_safe_exit(void)
+void printk_safe_exit(void)
{
this_cpu_dec(printk_context);
}
+EXPORT_SYMBOL_GPL(printk_safe_exit);
__printf(1, 0) int vprintk_func(const char *fmt, va_list args)
{
--
2.19.1
^ permalink raw reply related
* [RFC][PATCHv2 1/4] panic: avoid deadlocks in re-entrant console drivers
From: Sergey Senozhatsky @ 2018-10-16 5:04 UTC (permalink / raw)
To: linux-kernel
Cc: Petr Mladek, Steven Rostedt, Daniel Wang, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky,
Sergey Senozhatsky
In-Reply-To: <20181016050428.17966-1-sergey.senozhatsky@gmail.com>
>From printk()/serial console point of view panic() is special, because
it may force CPU to re-enter printk() or/and serial console driver.
Therefore, some of serial consoles drivers are re-entrant. E.g. 8250:
serial8250_console_write()
{
if (port->sysrq)
locked = 0;
else if (oops_in_progress)
locked = spin_trylock_irqsave(&port->lock, flags);
else
spin_lock_irqsave(&port->lock, flags);
...
}
panic() does set oops_in_progress via bust_spinlocks(1), so in theory
we should be able to re-enter serial console driver from panic():
CPU0
<NMI>
uart_console_write()
serial8250_console_write() // if (oops_in_progress)
// spin_trylock_irqsave()
call_console_drivers()
console_unlock()
console_flush_on_panic()
bust_spinlocks(1) // oops_in_progress++
panic()
<NMI/>
spin_lock_irqsave(&port->lock, flags) // spin_lock_irqsave()
serial8250_console_write()
call_console_drivers()
console_unlock()
printk()
...
However, this does not happen and we deadlock in serial console on
port->lock spinlock. And the problem is that console_flush_on_panic()
called after bust_spinlocks(0):
void panic(const char *fmt, ...)
{
bust_spinlocks(1);
...
bust_spinlocks(0);
console_flush_on_panic();
...
}
bust_spinlocks(0) decrements oops_in_progress, so oops_in_progress
can go back to zero. Thus even re-entrant console drivers will simply
spin on port->lock spinlock. Given that port->lock may already be
locked either by a stopped CPU, or by the very same CPU we execute
panic() on (for instance, NMI panic() on printing CPU) the system
deadlocks and does not reboot.
Fix this by setting oops_in_progress before console_flush_on_panic(),
so re-entrant console drivers will trylock the port->lock instead of
spinning on it forever.
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
kernel/panic.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/kernel/panic.c b/kernel/panic.c
index f6d549a29a5c..a0e60ccf3031 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -237,7 +237,13 @@ void panic(const char *fmt, ...)
if (_crash_kexec_post_notifiers)
__crash_kexec(NULL);
+ /*
+ * Decrement oops_in_progress and let bust_spinlocks() to
+ * unblank_screen(), console_unblank() and wake_up_klogd()
+ */
bust_spinlocks(0);
+ /* Set oops_in_progress, so we can reenter serial console driver */
+ bust_spinlocks(1);
/*
* We may have ended up stopping the CPU holding the lock (in
--
2.19.1
^ permalink raw reply related
* [RFC][PATCHv2 0/4] less deadlock prone serial consoles
From: Sergey Senozhatsky @ 2018-10-16 5:04 UTC (permalink / raw)
To: linux-kernel
Cc: Petr Mladek, Steven Rostedt, Daniel Wang, Peter Zijlstra,
Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, Alan Cox,
Jiri Slaby, Peter Feiner, linux-serial, Sergey Senozhatsky,
Sergey Senozhatsky
Hello,
RFC
The patch set reduces the number of ways serial consoles
can deadlock the system, including the recently reported deadlock
in panic().
The test scenario I ran on my x86_64 box:
- panic() the system while one of the CPUs holds 8250's
uart_port->lock.
Test results:
base | patched
-----------------------------------------
deadlock | flush logbuf
| reboot the system
To keep the cover letter short, please find full description in
commit messages.
P.S. since this is RFC I modify only 8250 serial driver.
Sergey Senozhatsky (4):
panic: avoid deadlocks in re-entrant console drivers
printk: move printk_safe macros to printk header
seial: introduce uart_port locking helpers
tty: 8250: switch to uart_port locking helpers
drivers/tty/serial/8250/8250_core.c | 8 +--
drivers/tty/serial/8250/8250_dma.c | 4 +-
drivers/tty/serial/8250/8250_port.c | 81 +++++++++++++----------------
include/linux/printk.h | 40 ++++++++++++++
include/linux/serial_core.h | 48 +++++++++++++++++
kernel/panic.c | 6 +++
kernel/printk/internal.h | 37 -------------
kernel/printk/printk_safe.c | 6 ++-
8 files changed, 141 insertions(+), 89 deletions(-)
--
2.19.1
^ permalink raw reply
* Re: [PATCH 00/14] serial: langtiq: Add CCF suppport
From: Paul Burton @ 2018-10-15 21:58 UTC (permalink / raw)
To: Songjun Wu
Cc: yixin.zhu@linux.intel.com, chuanhua.lei@linux.intel.com,
hauke.mehrtens@intel.com, devicetree@vger.kernel.org,
linux-serial@vger.kernel.org, James Hogan,
linux-mips@linux-mips.org, Jiri Slaby,
linux-kernel@vger.kernel.org, Thomas Gleixner,
Philippe Ombredanne, Rob Herring, Kate Stewart,
Greg Kroah-Hartman, Mark Rutland, Ralf Baechle
In-Reply-To: <20180924102803.30263-1-songjun.wu@linux.intel.com>
Hi Songjun,
On Mon, Sep 24, 2018 at 06:27:49PM +0800, Songjun Wu wrote:
> This patch series is for adding common clock framework support
> for langtiq serial driver, mainly includes:
s/langtiq/lantiq/ ...
> 1) Add common clock framework support.
> 2) Modify the dts file according to the DT conventions.
> 3) Replace the platform dependent functions with kernel functions
>
> Songjun Wu (14):
> MIPS: dts: Change upper case to lower case
> MIPS: dts: Add aliases node for lantiq danube serial
> serial: lantiq: Get serial id from dts
> serial: lantiq: Change ltq_w32_mask to asc_update_bits
> MIPS: lantiq: Unselect SWAP_IO_SPACE when LANTIQ is selected
> serial: lantiq: Use readl/writel instead of ltq_r32/ltq_w32
> serial: lantiq: Rename fpiclk to freqclk
> serial: lantiq: Replace clk_enable/clk_disable with clk generic API
> serial: lantiq: Add CCF support
> serial: lantiq: Reorder the head files
> include: Add lantiq.h in include/linux/
> serial: lantiq: Replace lantiq_soc.h with lantiq.h
> serial: lantiq: Change init_lqasc to static declaration
> dt-bindings: serial: lantiq: Add optional properties for CCF
It appears that you only copied me on patches 1, 2 & 5. I've applied
patch 1 to mips-next for 4.20, but I have no clue whether your other
patches were deemed acceptable by serial or DT maintainers & I have no
context for the changes being made, so I can neither apply nor ack
patches 2 & 5. Please copy me on the whole series next time.
Thanks,
Paul
^ permalink raw reply
* [PATCH] tty: serial: qcom_geni_serial: Don't slow all ports just for kgdb
From: Douglas Anderson @ 2018-10-15 20:44 UTC (permalink / raw)
To: gregkh
Cc: msavaliy, linux-arm-msm, alokc, mka, evgreen, Douglas Anderson,
linux-kernel, linux-serial, Jiri Slaby
If you turn on CONFIG_KGDB then you'll get CONFIG_CONSOLE_POLL
selected.
If you have CONFIG_CONSOLE_POLL selected then the GENI serial driver
was setting RX_BYTES_PW to 1 for _all_ UART ports.
This doesn't seem like such a good idea. Let's only set RX_BYTES_PW
to 1 for the console port.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/tty/serial/qcom_geni_serial.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index d3b5261ee80a..9ee6ce725e43 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -89,9 +89,9 @@
#define MAX_LOOPBACK_CFG 3
#ifdef CONFIG_CONSOLE_POLL
-#define RX_BYTES_PW 1
+#define CONSOLE_RX_BYTES_PW 1
#else
-#define RX_BYTES_PW 4
+#define CONSOLE_RX_BYTES_PW 4
#endif
struct qcom_geni_serial_port {
@@ -853,11 +853,13 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
unsigned int rxstale = DEFAULT_BITS_PER_CHAR * STALE_TIMEOUT;
u32 proto;
- if (uart_console(uport))
+ if (uart_console(uport)) {
port->tx_bytes_pw = 1;
- else
+ port->rx_bytes_pw = CONSOLE_RX_BYTES_PW;
+ } else {
port->tx_bytes_pw = 4;
- port->rx_bytes_pw = RX_BYTES_PW;
+ port->rx_bytes_pw = 4;
+ }
proto = geni_se_read_proto(&port->se);
if (proto != GENI_SE_UART) {
--
2.19.1.331.ge82ca0e54c-goog
^ permalink raw reply related
* Re: [PATCH v3 1/1] serial: imx - Add dma buffer confugration via sysfs
From: Greg KH @ 2018-10-15 14:01 UTC (permalink / raw)
To: Fabien Lahoudere; +Cc: linux-kernel, linux-serial
In-Reply-To: <1539249903-6316-1-git-send-email-fabien.lahoudere@collabora.com>
On Thu, Oct 11, 2018 at 11:25:03AM +0200, Fabien Lahoudere wrote:
> In order to optimize serial communication on imx53 and imx6, we may
> need to tweak DMA period and buffer length per period.
Why can you not just automatically determine this information? What is
userspace going to know that the kernel can not just learn now?
Having tunables is nice, but it is even better to not need them at all.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: Fix receive on SCIFA/SCIFB variants with DMA
From: Wolfram Sang @ 2018-10-15 11:49 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Jiri Slaby, Yoshihiro Shimoda, Ulrich Hecht,
Wolfram Sang, linux-serial, linux-renesas-soc, linux-kernel
In-Reply-To: <20181012121908.sbwsojs7vvhiztbb@ninjato>
[-- Attachment #1: Type: text/plain, Size: 515 bytes --]
> > Fixes: 2c4ee23530ffc022 ("serial: sh-sci: Postpone DMA release when falling back to PIO")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> > ---
> > Reported on r8a7790/lager using SCIFA1 on Debug Serial 1.
>
> I hope to test this on Lager this weekend. I am quite sure it will work
> now, but then we have this formally checked, too.
Done now:
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2] of: base: Change logic in of_alias_get_alias_list()
From: Rob Herring @ 2018-10-12 19:28 UTC (permalink / raw)
To: Michal Simek
Cc: linux-kernel@vger.kernel.org, Michal Simek, Geert Uytterhoeven,
Greg Kroah-Hartman, devicetree, Jiri Slaby,
open list:SERIAL DRIVERS, Frank Rowand,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <1bc1488a4cbf21fafc64269958ba3ed04e0a3c1c.1539322989.git.michal.simek@xilinx.com>
On Fri, Oct 12, 2018 at 12:43 AM Michal Simek <michal.simek@xilinx.com> wrote:
>
> Check compatible string first before setting up bit in bitmap to also
> cover cases that allocated bitfield is not big enough.
> Show warning about it but let driver to continue to work with allocated
> bitfield to keep at least some devices (included console which
> is commonly close to serial0) to work.
>
> Fixes: b1078c355d76 ("of: base: Introduce of_alias_get_alias_list() to check alias IDs")
> Fixes: ae1cca3fa347 ("serial: uartps: Change uart ID port allocation")
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> Changes in v2:
> - Use EOVERFLOW instead of EINVAL - Rob
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH] serial: sh-sci: Fix receive on SCIFA/SCIFB variants with DMA
From: Wolfram Sang @ 2018-10-12 12:19 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Greg Kroah-Hartman, Jiri Slaby, Yoshihiro Shimoda, Ulrich Hecht,
Wolfram Sang, linux-serial, linux-renesas-soc, linux-kernel
In-Reply-To: <20181009174158.4476-1-geert+renesas@glider.be>
[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]
On Tue, Oct 09, 2018 at 07:41:58PM +0200, Geert Uytterhoeven wrote:
> On SCIFA and SCIFB serial ports with DMA support (i.e. some ports on
> R-Car Gen2 and RZ/G1 SoCs), receive DMA operations are submitted before
> the DMA channel pointer is initialized. Hence this fails, and the
> driver tries to fall back to PIO. However, at this early phase in the
> initialization sequence, fallback to PIO does not work, leading to a
> serial port that cannot receive any data.
>
> Fix this by calling sci_submit_rx() after initialization of the DMA
> channel pointer.
>
> Reported-by: Jinzai Solution and RVC Test Teams via Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Brainstorming: keep Shimoda-san's address as is and mention the test
teams in the commit message?
> Fixes: 2c4ee23530ffc022 ("serial: sh-sci: Postpone DMA release when falling back to PIO")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> Reported on r8a7790/lager using SCIFA1 on Debug Serial 1.
I hope to test this on Lager this weekend. I am quite sure it will work
now, but then we have this formally checked, too.
> Tested on r8a7791/koelsch using SCIFA3 on EXIO-B.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [SPAM][PATCH 2/4] dmaengine: mtk_uart_dma: add Mediatek uart DMA support
From: Long Cheng @ 2018-10-12 6:18 UTC (permalink / raw)
To: Sean Wang
Cc: Mark Rutland, devicetree, srv_heupstream, Greg Kroah-Hartman,
linux-kernel, Matthias Brugger, Vinod Koul, Rob Herring,
linux-mediatek, linux-serial, Jiri Slaby, dmaengine, Yingjoe Chen,
Dan Williams, Ed Blake, linux-arm-kernel
In-Reply-To: <1538171066.30348.221.camel@mtkswgap22>
On Sat, 2018-09-29 at 05:44 +0800, Sean Wang wrote:
> Hi,
>
> On Thu, 2018-09-20 at 14:41 +0800, Long Cheng wrote:
> > In DMA engine framework, add 8250 mtk dma to support it.
> >
> > Signed-off-by: Long Cheng <long.cheng@mediatek.com>
> > ---
> > drivers/dma/8250_mtk_dma.c | 1049 ++++++++++++++++++++++++++++++++++++++++++++
> > drivers/dma/Kconfig | 11 +
> > drivers/dma/Makefile | 1 +
>
> the driver should be moved to driver/dma/mediatek
ok, i will move these.
>
> > 3 files changed, 1061 insertions(+)
> > create mode 100644 drivers/dma/8250_mtk_dma.c
> >
> > diff --git a/drivers/dma/8250_mtk_dma.c b/drivers/dma/8250_mtk_dma.c
> > new file mode 100644
> > index 0000000..a07844e
> > --- /dev/null
> > +++ b/drivers/dma/8250_mtk_dma.c
> > @@ -0,0 +1,1049 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Mediatek 8250 DMA driver.
> > + *
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Long Cheng <long.cheng@mediatek.com>
> > + */
> > +
> > +#define pr_fmt(fmt) "8250-mtk-dma: " fmt
>
> pr_fmt can be removed since no place is called with pr_fmt
>
OK, i will remove it.
> > +#define DRV_NAME "8250-mtk-dma"
> > +
>
> use KBUILD_MODNAME instead
>
OK, i will modify it.
>
> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/list.h>
> > +#include <linux/module.h>
> > +#include <linux/of_dma.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/pm_runtime.h>
> > +
> > +#include "virt-dma.h"
> > +
> > +#define MTK_SDMA_REQUESTS 127
>
> name it as MTK_SDMA_DEFAULT_REQUESTS seems to be more readable
>
OK, i will modify it.
> > +#define MTK_SDMA_CHANNELS (CONFIG_SERIAL_8250_NR_UARTS * 2)
>
> total number of the channels should depend on the capability the hardware provides, it should be detected by the device tree
>
in kernel config, CONFIG_SERIAL_8250_NR_UARTS is uart port numbers. i
think that maybe it's OK.
> > +
> > +#define VFF_RX_INT_FLAG_CLR_B (BIT(0U) | BIT(1U))
> > +#define VFF_TX_INT_FLAG_CLR_B 0
> > +#define VFF_RX_INT_EN0_B BIT(0U) /*rx valid size >= vff thre */
> > +#define VFF_RX_INT_EN1_B BIT(1U)
> > +#define VFF_TX_INT_EN_B BIT(0U) /*tx left size >= vff thre */
> > +#define VFF_INT_EN_CLR_B 0
> > +#define VFF_WARM_RST_B BIT(0U)
> > +#define VFF_EN_B BIT(0U)
> > +#define VFF_STOP_B BIT(0U)
> > +#define VFF_STOP_CLR_B 0
> > +#define VFF_FLUSH_B BIT(0U)
> > +#define VFF_FLUSH_CLR_B 0
> > +#define VFF_4G_SUPPORT_B BIT(0U)
> > +#define VFF_4G_SUPPORT_CLR_B 0
> > +
>
> all postfix U can be removed, and register offset placed along with the bit definition would be good to read
>
OK, I will modify these.
> > +/* interrupt trigger level for tx */
> > +#define VFF_TX_THRE(n) ((n) * 7 / 8)
> > +/* interrupt trigger level for rx */
> > +#define VFF_RX_THRE(n) ((n) * 3 / 4)
> > +
> > +#define MTK_DMA_RING_SIZE 0xffffU
> > +/* invert this bit when wrap ring head again*/
> > +#define MTK_DMA_RING_WRAP 0x10000U
> > +
> > +struct mtk_dmadev {
> > + struct dma_device ddev;
> > + void __iomem *mem_base[MTK_SDMA_CHANNELS];
> > + spinlock_t lock; /* dma dev lock */
> > + struct tasklet_struct task;
>
> tasklet should be removed in general in order to allow the task as soon as possible to be process
> you could refer to mtk_hsdma.c I made in drivers/dma/mediatek/ first
>
there are many data transfer, and UART is low speed devices. So add the
tasklet to process data.
> > + struct list_head pending;
> > + struct clk *clk;
> > + unsigned int dma_requests;
> > + bool support_33bits;
> > + unsigned int dma_irq[MTK_SDMA_CHANNELS];
> > + struct mtk_chan *lch_map[MTK_SDMA_CHANNELS];
>
> why is the map required? do you offer any way for that the channel would be assigned or managed on the runtime?
>
just for record channel. maybe need rename.
> > +};
> > +
> > +struct mtk_chan {
> > + struct virt_dma_chan vc;
> > + struct list_head node;
> > + struct dma_slave_config cfg;
> > + void __iomem *channel_base;
>
> channel_base can be renamed to base since the member is located at mtk_chan
>
OK, i will modify it.
> > + struct mtk_dma_desc *desc;
> > +
> > + bool paused;
>
> I don't think the variable paused since it doesn't support pause function
>
it's used to terminate. maybe rename to stop. if you think it's better.
> > + bool requested;
> > +
> > + unsigned int dma_sig;
> > + unsigned int dma_ch;
> > + unsigned int sgidx;
> > + unsigned int remain_size;
> > + unsigned int rx_ptr;
> > +
> > + /*sync*/
> > + struct completion done; /* dma transfer done */
> > + spinlock_t lock; /* channel lock */
> > + atomic_t loopcnt;
> > + atomic_t entry; /* entry count */
>
> Why the two atomic variables is introduced? they make the whole logic flow a little bit hard to understand.
> In general, the generic dma_virtual_channels can support the most dma engines for how descriptors on each channel being managed.
>
> You should check it first how list vc->desc_allocated, vc->desc_submitted and vc->desc_issued being maintained and really think if
> it's also suitable to the dmaengine. You could refer to mtk-hsdma.c I made in drivers/dma/mediatek/ first.
> The driver totally reuses these vc lists..
>
entry, for data don't lost, if entry > 1, run tasket schedule.
loopcnt, for tx, make sure that all data send.
> > +};
> > +
> > +struct mtk_dma_sg {
> > + dma_addr_t addr;
> > + unsigned int en; /* number of elements (24-bit) */
> > + unsigned int fn; /* number of frames (16-bit) */
> > +};
> > +
> > +struct mtk_dma_desc {
> > + struct virt_dma_desc vd;
> > + enum dma_transfer_direction dir;
> > + dma_addr_t dev_addr;
> > +
>
> I don't see dir, dev_addr being used in code flow? or I missed something ?
>
i will remove it.
> > + unsigned int sglen;
> > + struct mtk_dma_sg sg[0];
> > +};
> > +
> > +enum {
> > + VFF_INT_FLAG = 0x00,
> > + VFF_INT_EN = 0x04,
> > + VFF_EN = 0x08,
> > + VFF_RST = 0x0c,
> > + VFF_STOP = 0x10,
> > + VFF_FLUSH = 0x14,
> > + VFF_ADDR = 0x1c,
> > + VFF_LEN = 0x24,
> > + VFF_THRE = 0x28,
> > + VFF_WPT = 0x2c,
> > + VFF_RPT = 0x30,
> > + /*TX: the buffer size HW can read. RX: the buffer size SW can read.*/
> > + VFF_VALID_SIZE = 0x3c,
> > + /*TX: the buffer size SW can write. RX: the buffer size HW can write.*/
> > + VFF_LEFT_SIZE = 0x40,
> > + VFF_DEBUG_STATUS = 0x50,
> > + VFF_4G_SUPPORT = 0x54,
> > +};
> > +
>
> Add register definition with #define and move them near the bit field definition.
OK.
>
>
> > +static bool mtk_dma_filter_fn(struct dma_chan *chan, void *param);
> > +static struct of_dma_filter_info mtk_dma_info = {
> > + .filter_fn = mtk_dma_filter_fn,
> > +};
> > +
> > +static inline struct mtk_dmadev *to_mtk_dma_dev(struct dma_device *d)
> > +{
> > + return container_of(d, struct mtk_dmadev, ddev);
> > +}
> > +
> > +static inline struct mtk_chan *to_mtk_dma_chan(struct dma_chan *c)
> > +{
> > + return container_of(c, struct mtk_chan, vc.chan);
> > +}
> > +
> > +static inline struct mtk_dma_desc *to_mtk_dma_desc
> > + (struct dma_async_tx_descriptor *t)
> > +{
> > + return container_of(t, struct mtk_dma_desc, vd.tx);
> > +}
> > +
> > +static void mtk_dma_chan_write(struct mtk_chan *c,
> > + unsigned int reg, unsigned int val)
> > +{
> > + writel(val, c->channel_base + reg);
> > +}
> > +
> > +static unsigned int mtk_dma_chan_read(struct mtk_chan *c, unsigned int reg)
> > +{
> > + return readl(c->channel_base + reg);
> > +}
> > +
> > +static void mtk_dma_desc_free(struct virt_dma_desc *vd)
> > +{
> > + struct dma_chan *chan = vd->tx.chan;
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (c->desc && c->cfg.direction == DMA_DEV_TO_MEM)
> > + atomic_dec(&c->entry);
> > +
> > + kfree(c->desc);
> > + c->desc = NULL;
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
>
> The lock should not be necessary and try to make *_desc_free be as simple as possible.
>
> In general, the function is being only to free specific memory about the vd. It should be done by
> kfree(container_of(vd, struct mtk_apdma_vdesc, vd)); And don't need to care what detail the channel and direction is.
>
i remember that if no the lock, maybe will crash. it's timing issue with
'mtk_dma_terminate_all'. So i think that maybe can keep it.
> > +}
> > +
> > +static int mtk_dma_clk_enable(struct mtk_dmadev *mtkd)
> > +{
> > + int ret;
> > +
> > + ret = clk_prepare_enable(mtkd->clk);
> > + if (ret) {
> > + dev_err(mtkd->ddev.dev, "Couldn't enable the clock\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void mtk_dma_clk_disable(struct mtk_dmadev *mtkd)
> > +{
> > + clk_disable_unprepare(mtkd->clk);
> > +}
> > +
> > +static void mtk_dma_remove_virt_list(dma_cookie_t cookie,
> > + struct virt_dma_chan *vc)
> > +{
> > + struct virt_dma_desc *vd;
> > +
> > + if (list_empty(&vc->desc_issued) == 0) {
> > + list_for_each_entry(vd, &vc->desc_issued, node) {
> > + if (cookie == vd->tx.cookie) {
> > + INIT_LIST_HEAD(&vc->desc_issued);
>
> Why force to reinit list desc_issued? that is enqueued by core layer. They are expected to be configured into the hardware by the driver in sequence
> and then dequeued from the list when the descriptor is totally setting done by the driver or completed by the hardware.
long time issue. i just remember that if no this, in
vchan_cookie_complete have some problem.
>
> > + break;
> > + }
> > + }
> > + }
> > +}
> > +
> > +static void mtk_dma_tx_flush(struct dma_chan *chan)
> > +{
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > +
> > + if (mtk_dma_chan_read(c, VFF_FLUSH) == 0U) {
> > + mtk_dma_chan_write(c, VFF_FLUSH, VFF_FLUSH_B);
> > + if (atomic_dec_and_test(&c->loopcnt))
>
> I really think depending on atomic complete a synchronization is a tricky thing. Can we have another elegant way to let synchronization become good to read and easy to maintain?
>
> Or you could refer to mtk_hsdma.c I made in drivers/dma/mediatek/ first, the similar synchronization is also being done here.
>
uart is low speed device. So synchronization is not good
> > + complete(&c->done);
> > + }
> > +}
> > +
> > +/*
> > + * check whether the dma flush operation is finished or not.
> > + * return 0 for flush success.
> > + * return others for flush timeout.
> > + */
> > +static int mtk_dma_check_flush_result(struct dma_chan *chan)
> > +{
> > + struct timespec start, end;
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > +
> > + start = ktime_to_timespec(ktime_get());
> > +
> > + while ((mtk_dma_chan_read(c, VFF_FLUSH) & VFF_FLUSH_B) == VFF_FLUSH_B) {
> > + end = ktime_to_timespec(ktime_get());
> > + if ((end.tv_sec - start.tv_sec) > 1 ||
> > + ((end.tv_sec - start.tv_sec) == 1 &&
> > + end.tv_nsec > start.tv_nsec)) {
> > + dev_err(chan->device->dev,
> > + "[DMA] Polling flush timeout\n");
> > + return -1;
>
> You can check readx_poll_timeout and related APIs instead of the open coding.
>
OK, i can check it.
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void mtk_dma_tx_write(struct dma_chan *chan)
> > +{
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + unsigned int txcount = c->remain_size;
> > + unsigned int len, send, left, wpt, wrap;
> > +
> > + if (atomic_inc_return(&c->entry) > 1) {
>
> atomic variable again. Can you explain more about what the branches (if and else) are doing for ? Let's see if they can be replaced by more elegant ways
first data don't transfer complete. another data come, we need move the
data to tasklet schedule.
>
> > + if (vchan_issue_pending(&c->vc) && !c->desc) {
> > + spin_lock(&mtkd->lock);
> > + list_add_tail(&c->node, &mtkd->pending);
> > + spin_unlock(&mtkd->lock);
> > + tasklet_schedule(&mtkd->task);
> > + }
> > + } else {
> > + len = mtk_dma_chan_read(c, VFF_LEN);
> > + if (mtk_dma_check_flush_result(chan) != 0)
> > + return;
> > +
> > + while ((left = mtk_dma_chan_read(c, VFF_LEFT_SIZE)) > 0U) {
> > + send = min(left, c->remain_size);
> > + wpt = mtk_dma_chan_read(c, VFF_WPT);
> > + wrap = wpt & MTK_DMA_RING_WRAP ? 0U : MTK_DMA_RING_WRAP;
> > +
> > + if ((wpt & (len - 1U)) + send < len)
> > + mtk_dma_chan_write(c, VFF_WPT, wpt + send);
> > + else
> > + mtk_dma_chan_write(c, VFF_WPT,
> > + ((wpt + send) & (len - 1U))
> > + | wrap);
> > +
> > + c->remain_size -= send;
> > + if (c->remain_size == 0U)
>
> the if condition can be moved to the the beginning while condition
OK, i will modify it
>
> > + break;
> > + }
> > +
> > + if (txcount != c->remain_size) {
> > + mtk_dma_chan_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
>
> why need to enable interrupt to trigger the next?
the dma buffer is ring buffer. So we will confirm that all data send.
and confirm that the data trigger done or not.
>
> > + mtk_dma_tx_flush(chan);
> > + }
> > + }
> > + atomic_dec(&c->entry);
> > +}
> > +
> > +static void mtk_dma_start_tx(struct mtk_chan *c)
> > +{
> > + if (mtk_dma_chan_read(c, VFF_LEFT_SIZE) == 0U) {
> > + pr_info("%s maybe need fix? @L %d\n", __func__, __LINE__);
>
> the debug message can be removed
ok, i will remove it.
>
> > + mtk_dma_chan_write(c, VFF_INT_EN, VFF_TX_INT_EN_B);
>
> what is the step for?
enable DMA interrupt.
>
> > + } else {
> > + reinit_completion(&c->done);
> > +
> > + /* inc twice, once for tx_flush, another for tx_interrupt */
> > + atomic_inc(&c->loopcnt);
> > + atomic_inc(&c->loopcnt);
>
> if you have two events for which some point wants to wait, why not create two two completions for them?
it's confirm that TX done. i can use other APIs instead of this.
>
> > + mtk_dma_tx_write(&c->vc.chan);
> > + }
> > + c->paused = false;
> > +}
> > +
> > +static void mtk_dma_get_rx_size(struct mtk_chan *c)
> > +{
> > + unsigned int count;
> > + unsigned int rdptr, wrptr, wrreg, rdreg;
> > + unsigned int rx_size = mtk_dma_chan_read(c, VFF_LEN);
> > +
>
> Sort all declarations in reversed Xmas tree, even along with all occurrences in the other functions
>
OK, i will modify it.
> > + rdreg = mtk_dma_chan_read(c, VFF_RPT);
> > + wrreg = mtk_dma_chan_read(c, VFF_WPT);
> > + rdptr = rdreg & MTK_DMA_RING_SIZE;
> > + wrptr = wrreg & MTK_DMA_RING_SIZE;
> > + count = ((rdreg ^ wrreg) & MTK_DMA_RING_WRAP) ?
> > + (wrptr + rx_size - rdptr) : (wrptr - rdptr);
> > +
> > + c->remain_size = count;
> > + c->rx_ptr = rdptr;
> > +
> > + mtk_dma_chan_write(c, VFF_RPT, wrreg);
> > +}
> > +
> > +static void mtk_dma_start_rx(struct mtk_chan *c)
> > +{
> > + struct dma_chan *chan = &c->vc.chan;
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + struct mtk_dma_desc *d = c->desc;
> > +
> > + if (mtk_dma_chan_read(c, VFF_VALID_SIZE) != 0U &&
> > + d && d->vd.tx.cookie != 0) {
> > + mtk_dma_get_rx_size(c);
> > + mtk_dma_remove_virt_list(d->vd.tx.cookie, &c->vc);
> > + vchan_cookie_complete(&d->vd);
>
> please make vchan_cookie_complete in a completion handler such as a completion ISR, not in a start handler.
the function is called by RX interrupt or rx schedule. can't remove it.
>
> > + } else {
> > + if (mtk_dma_chan_read(c, VFF_VALID_SIZE) != 0U) {
> > + spin_lock(&mtkd->lock);
> > + if (list_empty(&mtkd->pending))
> > + list_add_tail(&c->node, &mtkd->pending);
> > + spin_unlock(&mtkd->lock);
> > + tasklet_schedule(&mtkd->task);
> > + } else {
> > + if (atomic_read(&c->entry) > 0)
> > + atomic_set(&c->entry, 0);
>
> I am not not clear about the magical reset. could you explain more?
in the case, maybe no RX data in RX DMA buffer, So set to 0.
>
> > + }
> > + }
> > +}
> > +
> > +static void mtk_dma_reset(struct mtk_chan *c)
> > +{
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(c->vc.chan.device);
> > +
> > + mtk_dma_chan_write(c, VFF_ADDR, 0);
> > + mtk_dma_chan_write(c, VFF_THRE, 0);
> > + mtk_dma_chan_write(c, VFF_LEN, 0);
> > + mtk_dma_chan_write(c, VFF_RST, VFF_WARM_RST_B);
> > +
> > + while
> > + (mtk_dma_chan_read(c, VFF_EN));
>
> add a timeout, otherwise it would get a hang possibly. you can check readx_poll_timeout and related APIs to add a timeout.
OK, i can modify it.
>
> > +
> > + if (c->cfg.direction == DMA_DEV_TO_MEM)
> > + mtk_dma_chan_write(c, VFF_RPT, 0);
> > + else if (c->cfg.direction == DMA_MEM_TO_DEV)
> > + mtk_dma_chan_write(c, VFF_WPT, 0);
> > + else
> > + dev_info(c->vc.chan.device->dev, "Unknown direction.\n");
>
> is it possible to happen?
>
when first init, it's happen. it's debug log, i can remove it.
> > +
> > + if (mtkd->support_33bits)
> > + mtk_dma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_CLR_B);
> > +}
> > +
> > +static void mtk_dma_stop(struct mtk_chan *c)
> > +{
> > + int polling_cnt;
> > +
> > + mtk_dma_chan_write(c, VFF_FLUSH, VFF_FLUSH_CLR_B);
> > +
> > + polling_cnt = 0;
> > + while ((mtk_dma_chan_read(c, VFF_FLUSH) & VFF_FLUSH_B) ==
> > + VFF_FLUSH_B) {
> > + polling_cnt++;
> > + if (polling_cnt > 10000) {
> > + dev_err(c->vc.chan.device->dev,
> > + "dma stop: polling FLUSH fail, DEBUG=0x%x\n",
> > + mtk_dma_chan_read(c, VFF_DEBUG_STATUS));
> > + break;
> > + }
> > + }
>
> You can check readx_poll_timeout and related APIs to make the fucntion more readable
ok, i will check it.
>
> > +
> > + polling_cnt = 0;
> > + /*set stop as 1 -> wait until en is 0 -> set stop as 0*/
> > + mtk_dma_chan_write(c, VFF_STOP, VFF_STOP_B);
> > + while (mtk_dma_chan_read(c, VFF_EN)) {
> > + polling_cnt++;
> > + if (polling_cnt > 10000) {
> > + dev_err(c->vc.chan.device->dev,
> > + "dma stop: polling VFF_EN fail, DEBUG=0x%x\n",
> > + mtk_dma_chan_read(c, VFF_DEBUG_STATUS));
> > + break;
> > + }
> > + }
>
> You can check readx_poll_timeout and related APIs to make the function more readable.
>
ok, i will check it.
> > + mtk_dma_chan_write(c, VFF_STOP, VFF_STOP_CLR_B);
> > + mtk_dma_chan_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
> > +
> > + if (c->cfg.direction == DMA_DEV_TO_MEM)
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > + else
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > +
> > + c->paused = true;
>
> It's a stop, not a pause
yes. So maybe i can rename it.
>
> > +}
> > +
> > +/*
> > + * We need to deal with 'all channels in-use'
> > + */
> > +static void mtk_dma_rx_sched(struct mtk_chan *c)
> > +{
> > + struct dma_chan *chan = &c->vc.chan;
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > +
> > + if (atomic_read(&c->entry) < 1) {
> > + mtk_dma_start_rx(c);
> > + } else {
> > + spin_lock(&mtkd->lock);
> > + if (list_empty(&mtkd->pending))
> > + list_add_tail(&c->node, &mtkd->pending);
> > + spin_unlock(&mtkd->lock);
> > + tasklet_schedule(&mtkd->task);
>
> I'm not clear why mixing start rx and tasklet rx here. In general, please make all tx/rx job as soon as poosible.
>
> you can refer to the mtk_hsdma.c I made.
>
TX/RX is slow. It's Hw behavior, So need tasklet
> > + }
> > +}
> > +
> > +/*
> > + * This callback schedules all pending channels. We could be more
> > + * clever here by postponing allocation of the real DMA channels to
> > + * this point, and freeing them when our virtual channel becomes idle.
> > + *
> > + * We would then need to deal with 'all channels in-use'
> > + */
> > +static void mtk_dma_sched(unsigned long data)
> > +{
> > + struct mtk_dmadev *mtkd = (struct mtk_dmadev *)data;
> > + struct mtk_chan *c;
> > + struct virt_dma_desc *vd;
> > + dma_cookie_t cookie;
> > + LIST_HEAD(head);
> > + unsigned long flags;
> > +
> > + spin_lock_irq(&mtkd->lock);
> > + list_splice_tail_init(&mtkd->pending, &head);
> > + spin_unlock_irq(&mtkd->lock);
> > +
> > + if (list_empty(&head) == 0) {
>
> !list_empty
>
Ok, i will modify it.
> > + c = list_first_entry(&head, struct mtk_chan, node);
> > + cookie = c->vc.chan.cookie;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
>
> head is a local list so the lock is not neccessary
>
channel 'c' is relate of VC. long time ago, add this, maybe to fix
crash. So i suggest to keep it.
> > + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> > + list_del_init(&c->node);
>
> why not use list_dec version
what's API?
>
> > + mtk_dma_rx_sched(c);
> > + } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
> > + vd = vchan_find_desc(&c->vc, cookie);
> > +
> > + c->desc = to_mtk_dma_desc(&vd->tx);
> > + list_del_init(&c->node);
> > + mtk_dma_start_tx(c);
>
> why is direct call for tx and tasklet for rx?
if RX interrupt too many, need confirm data right, So add rx schedule.
TX data no the case.
>
> > + }
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > + }
> > +}
> > +
> > +static int mtk_dma_alloc_chan_resources(struct dma_chan *chan)
> > +{
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + int ret = -EBUSY;
> > +
> > + pm_runtime_get_sync(mtkd->ddev.dev);
> > +
> > + if (!mtkd->lch_map[c->dma_ch]) {
> > + c->channel_base = mtkd->mem_base[c->dma_ch];
> > + mtkd->lch_map[c->dma_ch] = c;
>
> keep lch_map in mtkd is not necessary. instead, I think we can keep all information required by the physical channel inside mtk_chan, not exported to the whole dma engine.
just record the channel. maybe rename.
>
> > + ret = 1;
>
> why returning 1 here?
>
one channel is requested successfully.
> > + }
> > + c->requested = false;
> > + mtk_dma_reset(c);
> > +
> > + return ret;
> > +}
> > +
> > +static void mtk_dma_free_chan_resources(struct dma_chan *chan)
> > +{
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > +
> > + if (c->requested) {
> > + c->requested = false;
> > + free_irq(mtkd->dma_irq[c->dma_ch], chan);
> > + }
> > +
> > + tasklet_kill(&mtkd->task);
> > +
> > + c->channel_base = NULL;
> > + mtkd->lch_map[c->dma_ch] = NULL;
> > + vchan_free_chan_resources(&c->vc);
>
> I think we should call mtk_dma_terminate first to free all descriptors and then free the other channel resouce in the function.
mtk_dma terminate called by Dmaengine
>
> > +
> > + dev_dbg(mtkd->ddev.dev, "freeing channel for %u\n", c->dma_sig);
> > + c->dma_sig = 0;
> > +
> > + pm_runtime_put_sync(mtkd->ddev.dev);
> > +}
> > +
> > +static enum dma_status mtk_dma_tx_status(struct dma_chan *chan,
> > + dma_cookie_t cookie,
> > + struct dma_tx_state *txstate)
> > +{
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + enum dma_status ret;
> > + unsigned long flags;
> > +
> > + ret = dma_cookie_status(chan, cookie, txstate);
>
> consider txstate null case and directly return when the descriptor is complete, such as
>
> ret = dma_cookie_status(c, cookie, txstate);
> if (ret == DMA_COMPLETE || !txstate)
> return ret;
>
OK, i will check it.
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (ret == DMA_IN_PROGRESS) {
> > + c->rx_ptr = mtk_dma_chan_read(c, VFF_RPT) & MTK_DMA_RING_SIZE;
> > + txstate->residue = c->rx_ptr;
>
> are you sure all descriptors DMA_IN_PROCESS all be processed by hardware ?
no, but it's not matter
>
> > + } else if (ret == DMA_COMPLETE && c->cfg.direction == DMA_DEV_TO_MEM) {
> > + txstate->residue = c->remain_size;
>
> why is residue not zero when dma is complete ?
the size is that move DMA to tty memory.
>
> > + } else {
> > + txstate->residue = 0;
> > + }
>
> setup ->residue by dma_set_residue
>
ok, i will modify it
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +
>
> spin_lock can be dropped
>
> > + return ret;
> > +}
> > +
> > +static unsigned int mtk_dma_desc_size(struct mtk_dma_desc *d)
> > +{
> > + struct mtk_dma_sg *sg;
> > + unsigned int i;
> > + unsigned int size;
> > +
> > + for (size = i = 0; i < d->sglen; i++) {
> > + sg = &d->sg[i];
> > + size += sg->en * sg->fn;
> > + }
> > + return size;
>
> drop the function definition, see the below reason.
>
> > +}
> > +
> > +static struct dma_async_tx_descriptor *mtk_dma_prep_slave_sg
> > + (struct dma_chan *chan, struct scatterlist *sgl,
> > + unsigned int sglen, enum dma_transfer_direction dir,
> > + unsigned long tx_flags, void *context)
> > +{
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + struct scatterlist *sgent;
> > + struct mtk_dma_desc *d;
> > + dma_addr_t dev_addr;
> > + unsigned int i, j, en, frame_bytes;
>
> save the variable frame_bytes an en, it's always kept as 1 during the function call
>
ok, i will modity it.
> > +
> > + en = 1;
> > + frame_bytes = 1;
> > +
> > + if (dir == DMA_DEV_TO_MEM) {
> > + dev_addr = c->cfg.src_addr;
> > + } else if (dir == DMA_MEM_TO_DEV) {
> > + dev_addr = c->cfg.dst_addr;
> > + } else {
> > + dev_err(chan->device->dev, "bad direction\n");
>
> the case seems never happens because the device registers the capability it can support to.
>
yes. for strict, add it.
> > + return NULL;
> > + }
> > +
> > + /* Now allocate and setup the descriptor. */
> > + d = kzalloc(sizeof(*d) + sglen * sizeof(d->sg[0]), GFP_ATOMIC);
> > + if (!d)
> > + return NULL;
> > +
> > + d->dir = dir;
> > + d->dev_addr = dev_addr;
>
> when and where is the d->dev_addr being used?
>
ok, i will remove it.
> > +
> > + j = 0;
> > + for_each_sg(sgl, sgent, sglen, i) {
> > + d->sg[j].addr = sg_dma_address(sgent);
> > + d->sg[j].en = en;
> > + d->sg[j].fn = sg_dma_len(sgent) / frame_bytes;
> > + j++;
> > + }
> > +
> > + d->sglen = j;
> > +
> > + if (dir == DMA_MEM_TO_DEV)
> > + c->remain_size = mtk_dma_desc_size(d);
>
> function mtk_dma_desc_size can be dropped since the function is only used once and the result can be accumulated by for_each_sg
ok i will modify it.
>
> > + return vchan_tx_prep(&c->vc, &d->vd, tx_flags);
> > +}
> > +
> > +static void mtk_dma_issue_pending(struct dma_chan *chan)
> > +{
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + struct mtk_dmadev *mtkd;
> > + struct virt_dma_desc *vd;
> > + dma_cookie_t cookie;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (c->cfg.direction == DMA_DEV_TO_MEM) {
> > + cookie = c->vc.chan.cookie;
> > + mtkd = to_mtk_dma_dev(chan->device);
> > + if (vchan_issue_pending(&c->vc) && !c->desc) {
> > + vd = vchan_find_desc(&c->vc, cookie);
> > + c->desc = to_mtk_dma_desc(&vd->tx);
> > + if (atomic_read(&c->entry) > 0)
> > + atomic_set(&c->entry, 0);
> > + }
> > + } else if (c->cfg.direction == DMA_MEM_TO_DEV) {
> > + cookie = c->vc.chan.cookie;
> > + if (vchan_issue_pending(&c->vc) && !c->desc) {
> > + vd = vchan_find_desc(&c->vc, cookie);
> > + c->desc = to_mtk_dma_desc(&vd->tx);
> > + mtk_dma_start_tx(c);
> > + }
> > + }
> > + spin_unlock_irqrestore(&c->vc.lock, flags)
>
>
> in general, we have to dump all vc->desc_issued into the hw as soon as possible. how is the other descriptors except for the cookie stands for ?
>
sorry, i don't know what's mean. would you like tell me more info.
> > ;
> > +}
> > +
> > +static irqreturn_t mtk_dma_rx_interrupt(int irq, void *dev_id)
> > +{
> > + struct dma_chan *chan = (struct dma_chan *)dev_id;
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > +
> > + if (atomic_inc_return(&c->entry) > 1) {
> > + if (list_empty(&mtkd->pending))
> > + list_add_tail(&c->node, &mtkd->pending);
> > + tasklet_schedule(&mtkd->task);
> > + } else {
> > + mtk_dma_start_rx(c);
> > + }
>
> what's the reason making rx into direct call or tasklet in some case?
the rx is asynchronous. if rx interrupt too much, we need call tasklet
>
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t mtk_dma_tx_interrupt(int irq, void *dev_id)
> > +{
> > + struct dma_chan *chan = (struct dma_chan *)dev_id;
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + struct mtk_dma_desc *d = c->desc;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (c->remain_size != 0U) {
> > + list_add_tail(&c->node, &mtkd->pending);
> > + tasklet_schedule(&mtkd->task);
> > + } else {
> > + mtk_dma_remove_virt_list(d->vd.tx.cookie, &c->vc);
> > + vchan_cookie_complete(&d->vd);
> > + }
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > + if (atomic_dec_and_test(&c->loopcnt))
> > + complete(&c->done);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int mtk_dma_slave_config(struct dma_chan *chan,
> > + struct dma_slave_config *cfg)
> > +{
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(c->vc.chan.device);
> > + int ret;
> > +
> > + c->cfg = *cfg;
> > +
> > + if (cfg->direction == DMA_DEV_TO_MEM) {
> > + unsigned int rx_len = cfg->src_addr_width * 1024;
>
> the dma eingine only supports DMA_SLAVE_BUSWIDTH_1_BYTE, why do we need to consider any other src_addr_width here ?
>
define trigger level to hw
> > +
> > + mtk_dma_chan_write(c, VFF_ADDR, cfg->src_addr);
> > + mtk_dma_chan_write(c, VFF_LEN, rx_len);
> > + mtk_dma_chan_write(c, VFF_THRE, VFF_RX_THRE(rx_len));
> > + mtk_dma_chan_write(c,
> > + VFF_INT_EN, VFF_RX_INT_EN0_B
> > + | VFF_RX_INT_EN1_B);
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_RX_INT_FLAG_CLR_B);
> > + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> > +
> > + if (!c->requested) {
> > + atomic_set(&c->entry, 0);
> > + c->requested = true;
> > + ret = request_irq(mtkd->dma_irq[c->dma_ch],
> > + mtk_dma_rx_interrupt,
> > + IRQF_TRIGGER_NONE,
> > +
>
> move the request_irq to driver probe stage, it can be manage by the core such irq affinity
>
probe seem run one times. we need control it.
> > DRV_NAME, chan);
> > + if (ret < 0) {
> > + dev_err(chan->device->dev, "Can't request rx dma IRQ\n");
> > + return -EINVAL;
> > + }
> > + }
> > + } else if (cfg->direction == DMA_MEM_TO_DEV) {
> > + unsigned int tx_len = cfg->dst_addr_width * 1024;
> > +
>
> the dma eingine only supports DMA_SLAVE_BUSWIDTH_1_BYTE, why do we need to consider any other src_addr_width here ?
>
define trigger level to hw
> > + mtk_dma_chan_write(c, VFF_ADDR, cfg->dst_addr);
> > + mtk_dma_chan_write(c, VFF_LEN, tx_len);
> > + mtk_dma_chan_write(c, VFF_THRE, VFF_TX_THRE(tx_len));
> > + mtk_dma_chan_write(c, VFF_INT_FLAG, VFF_TX_INT_FLAG_CLR_B);
> > + mtk_dma_chan_write(c, VFF_EN, VFF_EN_B);
> > +
> > + if (!c->requested) {
> > + c->requested = true;
> > + ret = request_irq(mtkd->dma_irq[c->dma_ch],
> > + mtk_dma_tx_interrupt,
> > + IRQF_TRIGGER_NONE,
> > + DRV_NAME, chan);
>
> move the request_irq to driver probe stage, it can be manage by the core such irq affinity
>
probe seem run one times. we need control it.
> > + if (ret < 0) {
> > + dev_err(chan->device->dev, "Can't request tx dma IRQ\n");
> > + return -EINVAL;
> > + }
> > + }
> > + } else {
> > + dev_info(chan->device->dev, "Unknown direction!\n");
>
> remove the the unnecessary log
>
ok, i will remove it
> > + }
> > +
> > + if (mtkd->support_33bits)
> > + mtk_dma_chan_write(c, VFF_4G_SUPPORT, VFF_4G_SUPPORT_B);
> > +
> > + if (mtk_dma_chan_read(c, VFF_EN) != VFF_EN_B) {
>
> when is the condition being satisfied ?
>
make sure that hw is OK. maybe the case can't to enter. just for debug,
if any err happened.
> > + dev_err(chan->device->dev,
> > + "config dma dir[%d] fail\n", cfg->direction);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_dma_terminate_all(struct dma_chan *chan)
> > +{
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + unsigned long flags;
> > + LIST_HEAD(head);
> > +
> > + if (atomic_read(&c->loopcnt) != 0)
> > + wait_for_completion(&c->done);
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (c->desc) {
> > + mtk_dma_remove_virt_list(c->desc->vd.tx.cookie, &c->vc);
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +
> > + mtk_dma_desc_free(&c->desc->vd);
> > +
> > + spin_lock_irqsave(&c->vc.lock, flags);
> > + if (!c->paused) {
> > + list_del_init(&c->node);
> > + mtk_dma_stop(c);
>
> I think split logic between descriptors recycle and channel stop would
> be good.
maybe. can i keep it?
>
> > + }
> > + }
> > + vchan_get_all_descriptors(&c->vc, &head);
> > + spin_unlock_irqrestore(&c->vc.lock, flags);
> > +
> > + vchan_dma_desc_free_list(&c->vc, &head);
>
>
> Can you tell more about the situation for vc->desc_allocated, vc->desc_submitted and vc->desc_issued when the function is being called ?
> Which list do the descriptors hardware is processing stay on?
>
wait_for_completion(&c->done), these parameters has list init.
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_dma_device_pause(struct dma_chan *chan)
> > +{
> > + /* Pause/Resume only allowed with cyclic mode */
> > + return -EINVAL;
> > +}
>
> I guess we don't nodde to register the dumb pause handler
yes, just match dma engine. i can remove it.
>
> > +
> > +static int mtk_dma_device_resume(struct dma_chan *chan)
> > +{
> > + /* Pause/Resume only allowed with cyclic mode */
> > + return -EINVAL;
> > +}
>
> I guess we don't nodde to register the dumb resume handler
>
yes, just match dma engine. i can remove it.
> > +
> > +static int mtk_dma_chan_init(struct mtk_dmadev *mtkd)
> > +{
>
> Directly span the function in the driver probe is good to know how many channels are request and how large size its memory being allocated.
ok. i can remove the function. and move the code to probe.
>
> > + struct mtk_chan *c;
> > +
> > + c = devm_kzalloc(mtkd->ddev.dev, sizeof(*c), GFP_KERNEL);
> > + if (!c)
> > + return -ENOMEM;
> > +
> > + c->vc.desc_free = mtk_dma_desc_free;
> > + vchan_init(&c->vc, &mtkd->ddev);
> > + spin_lock_init(&c->lock);
> > + INIT_LIST_HEAD(&c->node);
> > +
> > + init_completion(&c->done);
> > + atomic_set(&c->loopcnt, 0);
> > + atomic_set(&c->entry, 0);
>
> I really don't like to magic atomic variable. It would make it's harder to track each descriptors usage either pending or active in the dma engine.
i see, but need to control the follow.
>
> > +
> > + return 0;
> > +}
> > +
> > +static void mtk_dma_free(struct mtk_dmadev *mtkd)
> > +{
> > + tasklet_kill(&mtkd->task);
> > + while (list_empty(&mtkd->ddev.channels) == 0) {
> > + struct mtk_chan *c = list_first_entry(&mtkd->ddev.channels,
> > + struct mtk_chan, vc.chan.device_node);
> > +
> > + list_del(&c->vc.chan.device_node);
> > + tasklet_kill(&c->vc.task);
> > + devm_kfree(mtkd->ddev.dev, c);
>
> devm_kfree isn't be called explicitly in the driver, it would be taken care when driver is unloaded by the core.
>
> > + }
> > +}
> > +
> > +static const struct of_device_id mtk_uart_dma_match[] = {
> > + { .compatible = "mediatek,mt6577-uart-dma", },
> > + { /* sentinel */ },
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_uart_dma_match);
> > +
> > +static int mtk_dma_probe(struct platform_device *pdev)
> > +{
> > + struct mtk_dmadev *mtkd;
> > + struct resource *res;
> > + unsigned int i;
> > + int rc;
> > +
> > + mtkd = devm_kzalloc(&pdev->dev, sizeof(*mtkd), GFP_KERNEL);
> > + if (!mtkd)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < MTK_SDMA_CHANNELS; i++) {
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > + if (!res)
> > + return -ENODEV;
> > + mtkd->mem_base[i] = devm_ioremap_resource(&pdev->dev, res);
> > + if (IS_ERR(mtkd->mem_base[i]))
> > + return PTR_ERR(mtkd->mem_base[i]);
> > + }
> > +
> > + /* request irq */
>
> remove the comment that is not identical to the implementation as below code snippet
ok, i will remove it.
>
> > + for (i = 0; i < MTK_SDMA_CHANNELS; i++) {
> > + mtkd->dma_irq[i] = platform_get_irq(pdev, i);
> > + if ((int)mtkd->dma_irq[i] < 0) {
>
> the cast seems not necessary
if device tree error, we can know what's err.
>
> > + dev_err(&pdev->dev, "failed to get IRQ[%d]\n", i);
> > + return -EINVAL;
> > + }
> > + }
> > +
> > + mtkd->clk = devm_clk_get(&pdev->dev, NULL);
>
> assign a name to the "apdma" per the binding you adds
>
> > + if (IS_ERR(mtkd->clk)) {
> > + dev_err(&pdev->dev, "No clock specified\n");
> > + return PTR_ERR(mtkd->clk);
> > + }
> > +
> > + if (of_property_read_bool(pdev->dev.of_node, "dma-33bits")) {
> > + dev_info(&pdev->dev, "Support dma 33bits\n");
> > + mtkd->support_33bits = true;
> > + }
> > +
> > + if (mtkd->support_33bits)
> > + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(33));
> > + else
> > + rc = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > + if (rc)
> > + return rc;
> > +
> > + dma_cap_set(DMA_SLAVE, mtkd->ddev.cap_mask);
> > + mtkd->ddev.device_alloc_chan_resources = mtk_dma_alloc_chan_resources;
> > + mtkd->ddev.device_free_chan_resources = mtk_dma_free_chan_resources;
> > + mtkd->ddev.device_tx_status = mtk_dma_tx_status;
> > + mtkd->ddev.device_issue_pending = mtk_dma_issue_pending;
> > + mtkd->ddev.device_prep_slave_sg = mtk_dma_prep_slave_sg;
> > + mtkd->ddev.device_config = mtk_dma_slave_config;
> > + mtkd->ddev.device_pause = mtk_dma_device_pause;
> > + mtkd->ddev.device_resume = mtk_dma_device_resume;
>
> Is it possible that we don't register pause and resume handler if the dma engine cannot support to?
ok, i will remove it
>
> > + mtkd->ddev.device_terminate_all = mtk_dma_terminate_all;
> > + mtkd->ddev.src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> > + mtkd->ddev.dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE);
> > + mtkd->ddev.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> > + mtkd->ddev.residue_granularity = DMA_RESIDUE_GRANULARITY_SEGMENT;
> > + mtkd->ddev.dev = &pdev->dev;
> > + INIT_LIST_HEAD(&mtkd->ddev.channels);
> > + INIT_LIST_HEAD(&mtkd->pending);
> > +
> > + spin_lock_init(&mtkd->lock);
> > + tasklet_init(&mtkd->task, mtk_dma_sched, (unsigned long)mtkd);
> > +
> > + mtkd->dma_requests = MTK_SDMA_REQUESTS;
> > + if (of_property_read_u32(pdev->dev.of_node,
> > + "dma-requests", &mtkd->dma_requests) != 0) {
>
> extra "! = 0" can be dropped in the condition check
ok, i will remove it.
>
> > + dev_info(&pdev->dev,
> > + "Missing dma-requests property, using %u.\n",
> > + MTK_SDMA_REQUESTS);
> > + }
> > +
> > + for (i = 0; i < MTK_SDMA_CHANNELS; i++) {
> > + rc = mtk_dma_chan_init(mtkd);
> > + if (rc)
> > + goto err_no_dma;
> > + }
> > +
> > + pm_runtime_enable(&pdev->dev);
> > + pm_runtime_set_active(&pdev->dev);
> > +
> > + rc = dma_async_device_register(&mtkd->ddev);
> > + if (rc) {
> > + dev_warn(&pdev->dev, "fail to register DMA device: %d\n", rc);
>
> drop the debug log
ok, i will remove it
>
> > + mtk_dma_clk_disable(mtkd);
>
> move the undo jobs at the tail of the function to error handling clean
>
ok, i will modify
> > + goto err_no_dma;
> > + }
> > +
> > + platform_set_drvdata(pdev, mtkd);
> > +
> > + if (pdev->dev.of_node) {
>
> you don't need that since all mtk drivers are dt based
you are right. for strict, i just suggest that keep it.
>
> > + mtk_dma_info.dma_cap = mtkd->ddev.cap_mask;
> > +
> > + /* Device-tree DMA controller registration */
> > + rc = of_dma_controller_register(pdev->dev.of_node,
> > + of_dma_simple_xlate,
> > + &mtk_dma_info);
> > + if (rc) {
> > + dev_warn(&pdev->dev, "fail to register DMA controller\n");
>
> drop the debug log
ok, i will remove it.
>
> > + dma_async_device_unregister(&mtkd->ddev);
> > + mtk_dma_clk_disable(mtkd);
>
> move the undo jobs at the tail of the function to make error handling clean
>
Ok, i will modify it.
> > + goto err_no_dma;
> > + }
> > + }
> > +
> > + return rc;
> > +
> > +err_no_dma:
> > + mtk_dma_free(mtkd);
> > + return rc;
> > +}
> > +
> > +static int mtk_dma_remove(struct platform_device *pdev)
> > +{
> > + struct mtk_dmadev *mtkd = platform_get_drvdata(pdev);
> > +
> > + if (pdev->dev.of_node)
> > + of_dma_controller_free(pdev->dev.of_node);
> > +
> > + pm_runtime_disable(&pdev->dev);
> > + pm_runtime_put_noidle(&pdev->dev);
> > +
> > + dma_async_device_unregister(&mtkd->ddev);
> > +
> > + mtk_dma_free(mtkd);
>
> Extra task in the remove handler is needed to add here such as ensure that VC tasks being killed, disable hardware and its interrupts and wait for pending ISRs task all done.
before the function, terminate all function will be called, i remember.
in terminate_all function, do this.
>
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int mtk_dma_suspend(struct device *dev)
> > +{
> > + struct mtk_dmadev *mtkd = dev_get_drvdata(dev);
> > +
> > + if (!pm_runtime_suspended(dev))
> > + mtk_dma_clk_disable(mtkd);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_dma_resume(struct device *dev)
> > +{
> > + int ret;
> > + struct mtk_dmadev *mtkd = dev_get_drvdata(dev);
> > +
> > + if (!pm_runtime_suspended(dev)) {
> > + ret = mtk_dma_clk_enable(mtkd);
> > + if (ret) {
> > + dev_info(dev, "fail to enable clk: %d\n", ret);
>
> get rid of the debug message
>
ok, i will remove it.
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_dma_runtime_suspend(struct device *dev)
> > +{
> > + struct mtk_dmadev *mtkd = dev_get_drvdata(dev);
> > +
> > + mtk_dma_clk_disable(mtkd);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_dma_runtime_resume(struct device *dev)
> > +{
> > + int ret;
> > + struct mtk_dmadev *mtkd = dev_get_drvdata(dev);
> > +
> > + ret = mtk_dma_clk_enable(mtkd);
> > + if (ret) {
> > + dev_warn(dev, "fail to enable clk: %d\n", ret);
>
> get rid of the debug message
>
ok, i will remove it.
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +#endif /* CONFIG_PM_SLEEP */
> > +
> > +static const struct dev_pm_ops mtk_dma_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(mtk_dma_suspend, mtk_dma_resume)
> > + SET_RUNTIME_PM_OPS(mtk_dma_runtime_suspend,
> > + mtk_dma_runtime_resume, NULL)
> > +};
> > +
> > +static struct platform_driver mtk_dma_driver = {
> > + .probe = mtk_dma_probe,
>
> mtk_apdma_uart_probe? I guess apdma is a real name for the dma engine
yes. i can modify it.
>
> > + .remove = mtk_dma_remove,
>
> mtk_apdma_uart_remove? same reason as the above
yes. i can modify it.
>
> > + .driver = {
> > + .name = "8250-mtk-dma",
>
> .name = KBUILD_MODNAME,
>
OK, i will modify it.
> > + .pm = &mtk_dma_pm_ops,
> > + .of_match_table = of_match_ptr(mtk_uart_dma_match),
> > + },
> > +};
> > +
> > +static bool mtk_dma_filter_fn(struct dma_chan *chan, void *param)
> > +{
> > + if (chan->device->dev->driver == &mtk_dma_driver.driver) {
> > + struct mtk_dmadev *mtkd = to_mtk_dma_dev(chan->device);
> > + struct mtk_chan *c = to_mtk_dma_chan(chan);
> > + unsigned int req = *(unsigned int *)param;
> > +
> > + if (req <= mtkd->dma_requests) {
> > + c->dma_sig = req;
> > + c->dma_ch = req;
> > + return true;
> > + }
> > + }
> > + return false;
> > +}
>
> I really wonder if the filter fn is necessary stuff made here but I am not fully sure.
>
> Can you look into more about of_dma_controller_register and its usage to see if the generic call satisfies your need?
i will the function record the dma channel.
>
> > +
> > +static int mtk_dma_init(void)
> > +{
> > + return platform_driver_register(&mtk_dma_driver);
> > +}
> > +subsys_initcall(mtk_dma_init);
> > +
> > +static void __exit mtk_dma_exit(void)
> > +{
> > + platform_driver_unregister(&mtk_dma_driver);
> > +}
> > +module_exit(mtk_dma_exit);
>
> Use module_platform_driver instead and then add more information for the module such as MODULE_DESCRIPTION, MODULE_AUTHOR, MODULE_LICENSE.
>
ok, i will modify it.
> > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> > index dacf3f4..cfa1699 100644
> > --- a/drivers/dma/Kconfig
> > +++ b/drivers/dma/Kconfig
> > @@ -151,6 +151,17 @@ config DMA_JZ4780
> > If you have a board based on such a SoC and wish to use DMA for
> > devices which can use the DMA controller, say Y or M here.
> >
> > +config DMA_MTK_UART
> > + tristate "MediaTek SoCs APDMA support for UART"
> > + depends on OF
> > + select DMA_ENGINE
> > + select DMA_VIRTUAL_CHANNELS
> > + help
> > + Support for the UART DMA engine found on MediaTek MTK SoCs.
> > + when 8250 mtk uart is enabled, and if you want to using DMA,
> > + you can enable the config. the DMA engine just only be used
> > + with MediaTek Socs.
> > +
> > config DMA_SA11X0
> > tristate "SA-11x0 DMA support"
> > depends on ARCH_SA1100 || COMPILE_TEST
> > diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> > index c91702d..42690d8 100644
> > --- a/drivers/dma/Makefile
> > +++ b/drivers/dma/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_COH901318) += coh901318.o coh901318_lli.o
> > obj-$(CONFIG_DMA_BCM2835) += bcm2835-dma.o
> > obj-$(CONFIG_DMA_JZ4740) += dma-jz4740.o
> > obj-$(CONFIG_DMA_JZ4780) += dma-jz4780.o
> > +obj-$(CONFIG_DMA_MTK_UART) += 8250_mtk_dma.o
>
> move Makefile update to drivers/dma/mediatek
>
OK. i will modify it.
> > obj-$(CONFIG_DMA_SA11X0) += sa11x0-dma.o
> > obj-$(CONFIG_DMA_SUN4I) += sun4i-dma.o
> > obj-$(CONFIG_DMA_SUN6I) += sun6i-dma.o
>
>
^ permalink raw reply
* Re: Insanely high baud rates
From: H. Peter Anvin @ 2018-10-12 5:48 UTC (permalink / raw)
To: Theodore Y. Ts'o, Alan Cox, linux-serial,
Linux Kernel Mailing List, Greg Kroah-Hartman, Jiri Slaby,
Johan Hovold, Alexander Viro
In-Reply-To: <20181011214035.GC565@thunk.org>
On 10/11/18 2:40 PM, Theodore Y. Ts'o wrote:
> On Thu, Oct 11, 2018 at 07:14:30AM -0700, hpa@zytor.com wrote:
>>>
>>> I mean - what is the baud rate of a pty ?
>>
>> Whatever the master wants it to be...
>
> I think Alan's point is that it is highly unlikely you would be able
> to push the equivalent of 4 gbps through a PTY layer. The TTY later
> was never engineered for those speeds, and the real question is ---
> what's the point? That's what Ethernet is for.
>
I like to consider the far future; I think things like the Y2038 problem is a
good example. Or, as I like to put it, "numbers are cheap, running out of
numbers is expensive." Sounds like we have enough of a plan that we aren't
completely stuck should it ever become an issue. It might never, of course.
-hpa
^ permalink raw reply
* [PATCH v2] of: base: Change logic in of_alias_get_alias_list()
From: Michal Simek @ 2018-10-12 5:43 UTC (permalink / raw)
To: linux-kernel, monstr, geert, gregkh
Cc: devicetree, Jiri Slaby, Rob Herring, linux-serial, Frank Rowand,
linux-arm-kernel
Check compatible string first before setting up bit in bitmap to also
cover cases that allocated bitfield is not big enough.
Show warning about it but let driver to continue to work with allocated
bitfield to keep at least some devices (included console which
is commonly close to serial0) to work.
Fixes: b1078c355d76 ("of: base: Introduce of_alias_get_alias_list() to check alias IDs")
Fixes: ae1cca3fa347 ("serial: uartps: Change uart ID port allocation")
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
Changes in v2:
- Use EOVERFLOW instead of EINVAL - Rob
I have looked at it and I don't think there should be necessary to
report error immediately back with partially initialized bitfield.
The reason is that still there could be a console device which is most
likely below that max limit and it is worth to return at least that nbits
properly filled.
It will also enable cases that you can still continue to use aliases
higher then fields prepared for devices without alias.
To be fixed patches are present in tty-next branch.
And this patch depends on
"of: base: Fix english spelling in of_alias_get_alias_list()"
---
drivers/of/base.c | 22 ++++++++++++----------
drivers/tty/serial/xilinx_uartps.c | 2 +-
2 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 908de45f966b..6418205a05f5 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -1953,13 +1953,15 @@ int of_alias_get_id(struct device_node *np, const char *stem)
* The function travels the lookup table to record alias ids for the given
* device match structures and alias stem.
*
- * Return: 0 or -ENOSYS when !CONFIG_OF
+ * Return: 0 or -ENOSYS when !CONFIG_OF or
+ * -EOVERFLOW if alias ID is greater then allocated nbits
*/
int of_alias_get_alias_list(const struct of_device_id *matches,
const char *stem, unsigned long *bitmap,
unsigned int nbits)
{
struct alias_prop *app;
+ int ret = 0;
/* Zero bitmap field to make sure that all the time it is clean */
bitmap_zero(bitmap, nbits);
@@ -1976,21 +1978,21 @@ int of_alias_get_alias_list(const struct of_device_id *matches,
continue;
}
- if (app->id >= nbits) {
- pr_debug("%s: ID %d greater then bitmap field %d\n",
- __func__, app->id, nbits);
- continue;
- }
-
if (of_match_node(matches, app->np)) {
pr_debug("%s: Allocated ID %d\n", __func__, app->id);
- set_bit(app->id, bitmap);
+
+ if (app->id >= nbits) {
+ pr_warn("%s: ID %d >= than bitmap field %d\n",
+ __func__, app->id, nbits);
+ ret = -EOVERFLOW;
+ } else {
+ set_bit(app->id, bitmap);
+ }
}
- /* Alias exists but is not compatible with matches */
}
mutex_unlock(&of_mutex);
- return 0;
+ return ret;
}
EXPORT_SYMBOL_GPL(of_alias_get_alias_list);
diff --git a/drivers/tty/serial/xilinx_uartps.c b/drivers/tty/serial/xilinx_uartps.c
index d452dceb0cb3..b8f8abc96631 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -1393,7 +1393,7 @@ static int cdns_get_id(struct platform_device *pdev)
if (!alias_bitmap_initialized) {
ret = of_alias_get_alias_list(cdns_uart_of_match, "serial",
alias_bitmap, MAX_UART_INSTANCES);
- if (ret) {
+ if (ret && ret != -EOVERFLOW) {
mutex_unlock(&bitmap_lock);
return ret;
}
--
1.9.1
^ permalink raw reply related
* Re: Insanely high baud rates
From: Theodore Y. Ts'o @ 2018-10-11 21:40 UTC (permalink / raw)
To: hpa
Cc: Alan Cox, linux-serial, Linux Kernel Mailing List,
Greg Kroah-Hartman, Jiri Slaby, Johan Hovold, Alexander Viro
In-Reply-To: <8A583BC1-A295-48D2-B4C3-3E70AA50D035@zytor.com>
On Thu, Oct 11, 2018 at 07:14:30AM -0700, hpa@zytor.com wrote:
> >
> >I mean - what is the baud rate of a pty ?
>
> Whatever the master wants it to be...
I think Alan's point is that it is highly unlikely you would be able
to push the equivalent of 4 gbps through a PTY layer. The TTY later
was never engineered for those speeds, and the real question is ---
what's the point? That's what Ethernet is for.
- Ted
^ permalink raw reply
* Re: Insanely high baud rates
From: H. Peter Anvin @ 2018-10-11 19:39 UTC (permalink / raw)
To: Craig Milo Rogers, Alan Cox
Cc: linux-serial, Linux Kernel Mailing List, Greg Kroah-Hartman,
Jiri Slaby, Johan Hovold, Alexander Viro
In-Reply-To: <20181011193651.GA11500@isi.edu>
On 10/11/18 12:36 PM, Craig Milo Rogers wrote:
> On 18.10.11, Alan Cox wrote:
>> I mean - what is the baud rate of a pty ?
>
> Solaris made the distinction between B0, which means pty hangup mode,
> and any other baud rate:
>
> https://docs.oracle.com/cd/E88353_01/html/E37851/pty-4d.html
>
> But... why not implement a pty bandwidth limitation layer? You say, I
> need to justify it? It's for, uh... protecting the system from unrestricted
> pty usage DOS attacks! Yeah. That's what it's for.
B0 is hangup, that's not in question.
-hpa
^ permalink raw reply
* Re: Insanely high baud rates
From: Craig Milo Rogers @ 2018-10-11 19:36 UTC (permalink / raw)
To: Alan Cox
Cc: hpa, linux-serial, Linux Kernel Mailing List, Greg Kroah-Hartman,
Jiri Slaby, Johan Hovold, Alexander Viro
In-Reply-To: <20181011133134.085624af@alans-desktop>
On 18.10.11, Alan Cox wrote:
> I mean - what is the baud rate of a pty ?
Solaris made the distinction between B0, which means pty hangup mode,
and any other baud rate:
https://docs.oracle.com/cd/E88353_01/html/E37851/pty-4d.html
But... why not implement a pty bandwidth limitation layer? You say, I
need to justify it? It's for, uh... protecting the system from unrestricted
pty usage DOS attacks! Yeah. That's what it's for.
Craig Milo Rogers
^ permalink raw reply
* Re: Insanely high baud rates
From: hpa @ 2018-10-11 14:14 UTC (permalink / raw)
To: Alan Cox
Cc: linux-serial, Linux Kernel Mailing List, Greg Kroah-Hartman,
Jiri Slaby, Johan Hovold, Alexander Viro
In-Reply-To: <20181011133134.085624af@alans-desktop>
On October 11, 2018 5:31:34 AM PDT, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
>> I'm mostly wondering if it is worth future-proofing for new
>transports. It sounds like we can have a consensus on leaving the upper
>4 bits of the speed fields reserved, but leave the details of
>implementation for the future?
>
>It seems reasonable, although I think the reality is that any future
>transport is not going to be a true serial link, but some kind of
>serial
>emulation layer. For those the speed really only matters to tell
>editors
>and the like not to bother being clever.
>
>I mean - what is the baud rate of a pty ?
>
>Alan
Whatever the master wants it to be...
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox