| 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: <fa17f083805e46079bfb4ed89b83d372cfe7e77c.camel@microchip.com>
Date: Fri, 9 Feb 2024 04:52:08 +0000
From: <Rengarajan.S@...rochip.com>
To: <jirislaby@...nel.org>, <linux-serial@...r.kernel.org>,
<gregkh@...uxfoundation.org>, <Kumaravel.Thiagarajan@...rochip.com>,
<linux-kernel@...r.kernel.org>, <Tharunkumar.Pasumarthi@...rochip.com>
CC: <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH v1 tty] 8250: microchip: pci1xxxx: Add Burst mode
transmission support in uart driver for reading from FIFO
Hi Jiri Slaby,
The patch has been accepted and added in the tty-git tree and will be
merged during the next merged window. Should the changes be given as a
seperate incremental patch or should we re-submit this patch again.
On Thu, 2024-02-08 at 02:49 +0000, Rengarajan S - I69107 wrote:
> Hi Jiri Slaby,
>
> Thanks for reviewing the patch. Will address the comments in the next
> patch revision.
>
> On Mon, 2024-02-05 at 08:56 +0100, Jiri Slaby wrote:
> > [Some people who received this message don't often get email from
> > jirislaby@...nel.org. Learn why this is important at
> > https://aka.ms/LearnAboutSenderIdentification ]
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe
> >
> > On 25. 01. 24, 11:00, Rengarajan S wrote:
> > > pci1xxxx_handle_irq reads the burst status and checks if the FIFO
> > > is empty and is ready to accept the incoming data. The handling
> > > is
> > > done in pci1xxxx_tx_burst where each transaction processes data
> > > in
> > > block of DWORDs, while any remaining bytes are processed
> > > individually,
> > > one byte at a time.
> > >
> > > Signed-off-by: Rengarajan S <rengarajan.s@...rochip.com>
> > > ---
> > > drivers/tty/serial/8250/8250_pci1xxxx.c | 106
> > > ++++++++++++++++++++++++
> > > 1 file changed, 106 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > > b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > > index 558c4c7f3104..d53605bf908d 100644
> > > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > ...
> > > @@ -344,6 +348,105 @@ static void pci1xxxx_rx_burst(struct
> > > uart_port *port, u32 uart_status)
> > > }
> > > }
> > >
> > > +static void pci1xxxx_process_write_data(struct uart_port *port,
> > > + struct circ_buf *xmit,
> > > + int *data_empty_count,
> >
> > count is unsigned, right?
> >
> > > + u32 *valid_byte_count)
> > > +{
> > > + u32 valid_burst_count = *valid_byte_count /
> > > UART_BURST_SIZE;
> > > +
> > > + /*
> > > + * Each transaction transfers data in DWORDs. If there are
> > > less than
> > > + * four remaining valid_byte_count to transfer or if the
> > > circular
> > > + * buffer has insufficient space for a DWORD, the data is
> > > transferred
> > > + * one byte at a time.
> > > + */
> > > + while (valid_burst_count) {
> > > + if (*data_empty_count - UART_BURST_SIZE < 0)
> >
> > Huh?
> >
> > *data_empty_count < UART_BURST_SIZE
> >
> > instead?
> >
> > > + break;
> > > + if (xmit->tail > (UART_XMIT_SIZE -
> > > UART_BURST_SIZE))
> >
> > Is this the same as easy to understand:
> >
> > CIRC_CNT_TO_END(xmit->head, xmit->tail, UART_XMIT_SIZE) <
> > UART_BURST_SIZE
> >
> > ?
> >
> > > + break;
> > > + writel(*(unsigned int *)&xmit->buf[xmit->tail],
> > > + port->membase + UART_TX_BURST_FIFO);
> >
> > What about unaligned accesses?
> >
> > And you really wanted to spell u32 explicitly, not uint.
> >
> > > + *valid_byte_count -= UART_BURST_SIZE;
> > > + *data_empty_count -= UART_BURST_SIZE;
> > > + valid_burst_count -= UART_BYTE_SIZE;
> > > +
> > > + xmit->tail = (xmit->tail + UART_BURST_SIZE) &
> > > + (UART_XMIT_SIZE - 1);
> >
> > uart_xmit_advance()
> >
> > > + }
> > > +
> > > + while (*valid_byte_count) {
> > > + if (*data_empty_count - UART_BYTE_SIZE < 0)
> > > + break;
> > > + writeb(xmit->buf[xmit->tail], port->membase +
> > > + UART_TX_BYTE_FIFO);
> > > + *data_empty_count -= UART_BYTE_SIZE;
> > > + *valid_byte_count -= UART_BYTE_SIZE;
> > > +
> > > + /*
> > > + * When the tail of the circular buffer is reached,
> > > the next
> > > + * byte is transferred to the beginning of the
> > > buffer.
> > > + */
> > > + xmit->tail = (xmit->tail + UART_BYTE_SIZE) &
> > > + (UART_XMIT_SIZE - 1);
> >
> > uart_xmit_advance()
> >
> > > +
> > > + /*
> > > + * If there are any pending burst count, data is
> > > handled by
> > > + * transmitting DWORDs at a time.
> > > + */
> > > + if (valid_burst_count && (xmit->tail <
> > > + (UART_XMIT_SIZE - UART_BURST_SIZE)))
> > > + break;
> > > + }
> > > +}
> >
> > This nested double loop is _really_ hard to follow. It's actually
> > terrible with cut & paste mistakes.
> >
> > Could these all loops be simply replaced by something like this
> > pseudo
> > code (a single loop):
> >
> > while (data_empty_count) {
> > cnt = CIRC_CNT_TO_END();
> > if (!cnt)
> > break;
> > if (cnt < UART_BURST_SIZE || (tail & 3)) { // is_unaligned()
> > writeb();
> > cnt = 1;
> > } else {
> > writel()
> > cnt = UART_BURST_SIZE;
> > }
> > uart_xmit_advance(cnt);
> > data_empty_count -= cnt;
> > }
> >
> > ?
> >
> >
> > > +static void pci1xxxx_tx_burst(struct uart_port *port, u32
> > > uart_status)
> > > +{
> > > + struct uart_8250_port *up = up_to_u8250p(port);
> > > + u32 valid_byte_count;
> > > + int data_empty_count;
> > > + struct circ_buf *xmit;
> > > +
> > > + xmit = &port->state->xmit;
> > > +
> > > + if (port->x_char) {
> > > + writeb(port->x_char, port->membase + UART_TX);
> > > + port->icount.tx++;
> > > + port->x_char = 0;
> > > + return;
> > > + }
> > > +
> > > + if ((uart_tx_stopped(port)) || (uart_circ_empty(xmit))) {
> > > + port->ops->stop_tx(port);
> >
> > This looks weird standing here. You should handle this below along
> > with RPM.
> >
> > > + } else {
> >
> > The condition should be IMO inverted with this block in its body:
> >
> > > + data_empty_count =
> > > (pci1xxxx_read_burst_status(port)
> > > &
> > > + UART_BST_STAT_TX_COUNT_MASK) >>
> > > 8;
> > > + do {
> > > + valid_byte_count =
> > > uart_circ_chars_pending(xmit);
> > > +
> > > + pci1xxxx_process_write_data(port, xmit,
> > > +
> > > &data_empty_count,
> > > +
> > > &valid_byte_count);
> > > +
> > > + port->icount.tx++;
> >
> > Why do you increase the stats only once per burst? (Solved by
> > uart_xmit_advance() above.)
> >
> > > + if (uart_circ_empty(xmit))
> > > + break;
> > > + } while (data_empty_count && valid_byte_count);
> > > + }
> > > +
> > > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
> > > + uart_write_wakeup(port);
> > > +
> > > + /*
> > > + * With RPM enabled, we have to wait until the FIFO is
> > > empty
> > > before
> > > + * the HW can go idle. So we get here once again with
> > > empty
> > > FIFO and
> > > + * disable the interrupt and RPM in __stop_tx()
> > > + */
> > > + if (uart_circ_empty(xmit) && !(up->capabilities &
> > > UART_CAP_RPM))
> > > + port->ops->stop_tx(port);
> >
> > I wonder why this driver needs this and others don't? Should they
> > be
> > fixed too?
> >
> > > +}
> > > +
> > > static int pci1xxxx_handle_irq(struct uart_port *port)
> > > {
> > > unsigned long flags;
> > > @@ -359,6 +462,9 @@ static int pci1xxxx_handle_irq(struct
> > > uart_port
> > > *port)
> > > if (status & UART_BST_STAT_LSR_RX_MASK)
> > > pci1xxxx_rx_burst(port, status);
> > >
> > > + if (status & UART_BST_STAT_LSR_THRE)
> > > + pci1xxxx_tx_burst(port, status);
> > > +
> > > spin_unlock_irqrestore(&port->lock, flags);
> > >
> > > return 1;
> >
> > --
> > js
> > suse labs
> >
>
Powered by blists - more mailing lists