| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 28 Mar 2023 11:23:45 +0200
From: Jiri Slaby <jirislaby@...nel.org>
To: Jacky Huang <ychuang570808@...il.com>, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, lee@...nel.org,
mturquette@...libre.com, sboyd@...nel.org, p.zabel@...gutronix.de,
gregkh@...uxfoundation.org
Cc: devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
arnd@...db.de, schung@...oton.com, mjchen@...oton.com,
Jacky Huang <ychuang3@...oton.com>
Subject: Re: [PATCH v6 11/12] tty: serial: Add Nuvoton ma35d1 serial driver
support
On 28. 03. 23, 4:19, Jacky Huang wrote:
> +static void transmit_chars(struct uart_ma35d1_port *up)
> +{
> + struct circ_buf *xmit = &up->port.state->xmit;
> + int count;
> + u8 ch;
> +
> + if (uart_tx_stopped(&up->port)) {
> + ma35d1serial_stop_tx(&up->port);
> + return;
> + }
> + if (uart_circ_empty(xmit)) {
> + __stop_tx(up);
> + return;
> + }
Why is this necessary?
> + count = UART_FIFO_DEPTH - ((serial_in(up, UART_REG_FSR) & FSR_TXPTR_MSK) >> 16);
> +
> + uart_port_tx_limited(&up->port, ch, count,
> + !(serial_in(up, UART_REG_FSR) & FSR_TX_FULL),
> + serial_out(up, UART_REG_THR, ch),
> + ({}));
> +
> + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> + uart_write_wakeup(&up->port);
> +
> + if (uart_circ_empty(xmit))
> + __stop_tx(up);
uart_port_tx_limited() should take care about the above and this too, right?
> +}
...
> +static void receive_chars(struct uart_ma35d1_port *up)
> +{
> + u8 flag;
> + u32 fsr;
> + unsigned int ch;
Shouldn't ch be u8 too?
> + int max_count = 256;
> +
> + fsr = serial_in(up, UART_REG_FSR);
> + do {
> + flag = TTY_NORMAL;
> + up->port.icount.rx++;
> +
> + if (unlikely(fsr & (FSR_BIF | FSR_FEF | FSR_PEF | FSR_RX_OVER_IF))) {
> + if (fsr & FSR_BIF) {
> + up->port.icount.brk++;
> + if (uart_handle_break(&up->port))
> + continue;
> + }
> + if (fsr & FSR_FEF)
> + up->port.icount.frame++;
> + if (fsr & FSR_PEF)
> + up->port.icount.parity++;
> + if (fsr & FSR_RX_OVER_IF)
> + up->port.icount.overrun++;
> +
> + serial_out(up, UART_REG_FSR, fsr &
> + (FSR_BIF | FSR_FEF | FSR_PEF | FSR_RX_OVER_IF));
> +
> + if (fsr & FSR_BIF)
> + flag = TTY_BREAK;
> + else if (fsr & FSR_PEF)
> + flag = TTY_PARITY;
> + else if (fsr & FSR_FEF)
> + flag = TTY_FRAME;
> + }
> +
> + ch = serial_in(up, UART_REG_RBR);
> + if (uart_handle_sysrq_char(&up->port, ch))
> + continue;
> +
> + spin_lock(&up->port.lock);
> + uart_insert_char(&up->port, fsr, FSR_RX_OVER_IF, ch, flag);
> + spin_unlock(&up->port.lock);
> +
> + fsr = serial_in(up, UART_REG_FSR);
> + } while (!(fsr & FSR_RX_EMPTY) && (max_count-- > 0));
> +
> + spin_lock(&up->port.lock);
> + tty_flip_buffer_push(&up->port.state->port);
> + spin_unlock(&up->port.lock);
> +}
...
> +static int ma35d1serial_remove(struct platform_device *dev)
> +{
> + struct uart_port *port = platform_get_drvdata(dev);
> +
> + if (port) {
Can this ever be NULL?
> + uart_remove_one_port(&ma35d1serial_reg, port);
> + free_irq(port->irq, port);
> + }
> + return 0;
> +}
> +
> +static int ma35d1serial_suspend(struct platform_device *dev, pm_message_t state)
> +{
> + int i;
> + struct uart_ma35d1_port *up;
> +
> + if (dev->dev.of_node)
> + i = of_alias_get_id(dev->dev.of_node, "serial");
> + if (i < 0) {
> + dev_err(&dev->dev, "failed to get alias/pdev id, errno %d\n", i);
> + return i;
> + }
> + up = &ma35d1serial_ports[i];
platform_get_drvdata(dev) ?
> + if (i == 0) {
> + up->console_baud_rate = serial_in(up, UART_REG_BAUD);
> + up->console_line = serial_in(up, UART_REG_LCR);
> + up->console_int = serial_in(up, UART_REG_IER);
> + }
> + return 0;
> +}
> +
> +static int ma35d1serial_resume(struct platform_device *dev)
> +{
> + int i;
> + struct uart_ma35d1_port *up;
> +
> + if (dev->dev.of_node)
> + i = of_alias_get_id(dev->dev.of_node, "serial");
> + if (i < 0) {
> + dev_err(&dev->dev, "failed to get alias/pdev id, errno %d\n", i);
> + return i;
> + }
> + up = &ma35d1serial_ports[i];
> + if (i == 0) {
> + serial_out(up, UART_REG_BAUD, up->console_baud_rate);
> + serial_out(up, UART_REG_LCR, up->console_line);
> + serial_out(up, UART_REG_IER, up->console_int);
> + }
> + return 0;
> +}
No uart_suspend_port()/uart_resume_port()? You don't wait for
transmitter to be empty in suspend. You don't stop tx etc.
> +static struct platform_driver ma35d1serial_driver = {
> + .probe = ma35d1serial_probe,
> + .remove = ma35d1serial_remove,
> + .suspend = ma35d1serial_suspend,
> + .resume = ma35d1serial_resume,
> + .driver = {
> + .name = "ma35d1-uart",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(ma35d1_serial_of_match),
> + },
> +};
> +
> +static int __init ma35d1serial_init(void)
> +{
> + int ret;
> +
> + ret = uart_register_driver(&ma35d1serial_reg);
> + if (ret)
> + return ret;
> + ret = platform_driver_register(&ma35d1serial_driver);
> + if (ret)
> + uart_unregister_driver(&ma35d1serial_reg);
> + return ret;
> +}
> +
> +static void __exit ma35d1serial_exit(void)
> +{
> + platform_driver_unregister(&ma35d1serial_driver);
> + uart_unregister_driver(&ma35d1serial_reg);
> +}
> +
> +module_init(ma35d1serial_init);
> +module_exit(ma35d1serial_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("MA35D1 serial driver");
> +MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR);
Why is this needed? How are other platform drivers autoloaded?
thanks,
--
js
suse labs
Powered by blists - more mailing lists