| 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
| ||
|
Message-ID: <89268a72-468f-415d-0f52-e1e2b8f1ad1e@gmail.com>
Date: Wed, 29 Mar 2023 16:06:31 +0800
From: Jacky Huang <ychuang570808@...il.com>
To: Jiri Slaby <jirislaby@...nel.org>, 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
Dear Jiri,
Thanks for your advice.
On 2023/3/28 下午 05:23, Jiri Slaby wrote:
> 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?
>
I will remove this reduntant uart_circ_empty() check.
>> + 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?
Yes, you are right. These two are also redundant. I will remove them.
Thank you.
>
>> +}
> ...
>> +static void receive_chars(struct uart_ma35d1_port *up)
>> +{
>> + u8 flag;
>> + u32 fsr;
>> + unsigned int ch;
>
> Shouldn't ch be u8 too?
OK, I will fix it as u8.
>
>> + 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?
OK, I will remove this NULL check.
>
>> + 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) ?
We will fix it.
>
>> + 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.
>
We will fix it in the next version.
>> +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,
Yes, it's not needed. I will remove it.
Best regards,
Jacky Huang
Powered by blists - more mailing lists