| 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: <CACMJSet4WZj3qNCDGUkcg6NPS0aO10L5jq=rETvLFTg78iULtA@mail.gmail.com>
Date: Mon, 28 Nov 2022 12:03:24 +0100
From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc: Bartosz Golaszewski <brgl@...ev.pl>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...ainline.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>,
Vinod Koul <vkoul@...nel.org>, Alex Elder <elder@...nel.org>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-serial@...r.kernel.org
Subject: Re: [PATCH v3 13/13] tty: serial: qcom-geni-serial: add support for
serial engine DMA
On Fri, 25 Nov 2022 at 15:37, Srinivas Kandagatla
<srinivas.kandagatla@...aro.org> wrote:
>
<snip>
> >
> > @@ -552,18 +558,11 @@ static void handle_rx_console(struct uart_port *uport, u32 bytes, bool drop)
> >
> > static void handle_rx_uart(struct uart_port *uport, u32 bytes, bool drop)
> > {
> > - struct tty_port *tport;
> > struct qcom_geni_serial_port *port = to_dev_port(uport);
> > - u32 num_bytes_pw = port->tx_fifo_width / BITS_PER_BYTE;
> > - u32 words = ALIGN(bytes, num_bytes_pw) / num_bytes_pw;
> > + struct tty_port *tport = &uport->state->port;
> > int ret;
> >
> > - tport = &uport->state->port;
> > - ioread32_rep(uport->membase + SE_GENI_RX_FIFOn, port->rx_fifo, words);
> > - if (drop)
> > - return;
> > -
>
> Are we removing FIFO support for uart?
>
> It it not a overhead to use dma for small transfers that are less than
> fifo size?
>
You made me think about it but no - while I haven't measured it yet, I
don't think it's worth the code complexity behind it. The i2c driver
doesn't do it this way for short transfers either. If you insist I can
test it and come forward with numbers but unless we encounter a
real-life example of the need for lots of very short reads/writes in
short succession, I don't think we should overcomplicate this.
>
> > - ret = tty_insert_flip_string(tport, port->rx_fifo, bytes);
> > + ret = tty_insert_flip_string(tport, port->rx_buf, bytes);
> > if (ret != bytes) {
> > dev_err(uport->dev, "%s:Unable to push data ret %d_bytes %d\n",
> > __func__, ret, bytes);
> > @@ -578,7 +577,70 @@ static unsigned int qcom_geni_serial_tx_empty(struct uart_port *uport)
> > return !readl(uport->membase + SE_GENI_TX_FIFO_STATUS);
> > }
> >
> > -static void qcom_geni_serial_start_tx(struct uart_port *uport)
> > +static void qcom_geni_serial_stop_tx_dma(struct uart_port *uport)
> > +{
> > + struct qcom_geni_serial_port *port = to_dev_port(uport);
> > + bool done;
>
> -->
> > + u32 status;
> ...
> > +
> > + status = readl(uport->membase + SE_GENI_STATUS);
> > + if (!(status & M_GENI_CMD_ACTIVE))
> > + return;
> <---
>
> this code snippet is repeating more than few times in the patches, looks
> like it could be made to a inline helper.
>
Makes sense.
>
> > +
> > + if (port->rx_dma_addr) {
> > + geni_se_tx_dma_unprep(&port->se, port->tx_dma_addr,
> > + port->tx_remaining);
> > + port->tx_dma_addr = (dma_addr_t)NULL;
> > + port->tx_remaining = 0;
> > + }
> > +
> > + m_irq_en = readl(uport->membase + SE_GENI_M_IRQ_EN);
> > + writel(m_irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> > + geni_se_cancel_m_cmd(&port->se);
> > +
> > + done = qcom_geni_serial_poll_bit(uport, SE_GENI_S_IRQ_STATUS,
> > + S_CMD_CANCEL_EN, true);
> > + if (!done) {
> > + geni_se_abort_m_cmd(&port->se);
> > + qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> > + M_CMD_ABORT_EN, true);
>
> return is not checked, there are few more such instances in the patch.
>
Yes, but this is an error path. What would I do if the cancel failed
to go through and then the abort failed as well? I can at best emit an
error message.
> > + writel(M_CMD_ABORT_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
> > + }
> > +
> > + writel(M_CMD_CANCEL_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
> > +}
> > +
> > +static void qcom_geni_serial_start_tx_dma(struct uart_port *uport)
> > +{
> > + struct qcom_geni_serial_port *port = to_dev_port(uport);
> > + struct circ_buf *xmit = &uport->state->xmit;
> > + unsigned int xmit_size;
> > + int ret;
> > +
> > + if (port->tx_dma_addr)
> > + return;
> Is this condition actually possible?
>
It should never happen but I wanted to be sure. Maybe a BUG_ON() or
WARN_ON() would be better?
>
> > +
> > + xmit_size = uart_circ_chars_pending(xmit);
> > + if (xmit_size < WAKEUP_CHARS)
> > + uart_write_wakeup(uport);
> > +
> > + xmit_size = CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE);
> > +
> > + qcom_geni_serial_setup_tx(uport, xmit_size);
> > +
> > + ret = geni_se_tx_dma_prep(&port->se, &xmit->buf[xmit->tail],
> > + xmit_size, &port->tx_dma_addr);
> > + if (ret) {
> > + dev_err(uport->dev, "unable to start TX SE DMA: %d\n", ret);
> > + qcom_geni_serial_stop_tx_dma(uport);
> > + return;
> > + }
> > +
> > + port->tx_remaining = xmit_size;
> > +}
> > +
>
> ...
Bart
Powered by blists - more mailing lists