| 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: <20101109103737.40bfab7b@linux.intel.com>
Date: Tue, 9 Nov 2010 10:37:37 +0000
From: Alan Cox <alan@...ux.intel.com>
To: Tomoya MORINAGA <tomoya-linux@....okisemi.com>
Cc: Greg Kroah-Hartman <gregkh@...e.de>,
Ben Dooks <ben-linux@...ff.org>,
Kukjin Kim <kgene.kim@...sung.com>,
Mike Frysinger <vapier@...too.org>,
Feng Tang <feng.tang@...el.com>,
Tobias Klauser <tklauser@...tanz.ch>,
linux-kernel@...r.kernel.org, yong.y.wang@...el.com,
qi.wang@...el.com, kok.howg.ewe@...el.com,
andrew.chih.howe.khor@...el.com
Subject: Re: [PATCH] EG20T: Update PCH_UART driver to 2.6.36
> +#ifdef CONFIG_PCH_DMA
> + #include <linux/dmaengine.h>
> + #include <linux/pch_dma.h>
> +#endif
The PCH DMA will have been included in the kernel anyway before this
merge so you can remove the ifdefs and include it regardless
> +#if !defined(PORT_PCH_256FIFO) || !defined(PORT_PCH_64FIFO)
> + #undef PORT_PCH_256FIFO
> + #undef PORT_PCH_64FIFO
> + #define PORT_PCH_256FIFO (PORT_MAX_8250+1) /* PCH UART with
> 256 byte
> + FIFO */
> + #define PORT_PCH_64FIFO (PORT_MAX_8250+2) /* PCH UART with
> 64 byte
> + FIFO */
> +#endif
FIFO config really all wants to be done at runtime.
> +static inline void wr(void __iomem *addr, unsigned int value)
> +{
> +#if PCH_UART_DMA_REG_BOUNDARY == 4
> + iowrite32(value, addr);
> +#else
> + iowrite8(value, (void *)addr);
Why the cast ?
> +#endif
Again you've got magic ifdefs with no explanation ?
> +}
> +
> +static void pch_uart_hal_request(struct pci_dev *pdev, int fifosize,
> + int base_baud)
> +{
> + struct eg20t_port *priv = pci_get_drvdata(pdev);
> +
> + priv->trigger_level = 1;
> + priv->fcr = 0;
> +}
> +
> +static int __get_msr(struct eg20t_port *priv, void __iomem *base)
> +{
Please avoid __ names for functions
> + unsigned int msr;
> +
> + msr = rr(base + PCH_UART_MSR);
> + priv->dmsr |= msr & PCH_UART_MSR_DELTA;
> +
> + return (int)msr;
Why - if it is unsigned then why not return unsigned values ?
> +static int pch_uart_hal_enable_interrupt(struct eg20t_port *priv,
> + unsigned int flag)
> +{
> + void __iomem *base;
> + int ret;
> +
> + base = priv->membase;
> + ret = __pch_uart_hal_enable_interrupt(base, flag);
> +
> + return ret;
Why not just
"return __pch_uart_hal_enable_interrupt(priv->membase, flag);"
in fact why not just remove all these wrappers entirely ?
> +static int push_rx(struct eg20t_port *priv, const unsigned char *buf,
> + int size)
> +{
> + struct uart_port *port;
> + struct tty_struct *tty;
> + int sz, i, j;
> + int loop;
> + int pushed;
> +
> + port = &priv->port;
> + tty = port->state->port.tty;
tty ports are refcounted
tty = tty_port_tty_get(...)
and when finished tty_kref_put(tty);
Also tty may be NULL, if the port closed as you were doing this, so
check the return from tty_port_tty_get and act accordingly.
> +static int handle_error(struct eg20t_port *priv, int err)
> +{
> + int ret = 0;
> + return ret;
> +}
Why ?????
> +
> +#ifndef CONFIG_PCH_DMA
DMA should be runtime configuration - vendors need to build generic
kernels so need things like DMA switching to be done on load not on
compile
> +static void pch_uart_set_mctrl(struct uart_port *port, unsigned int
> mctrl) +{
> +}
Seems to be unimplemented ?
> +static void pch_uart_set_termios(struct uart_port *port,
> + struct ktermios *termios, struct
> ktermios *old) +{
> + int baud;
> + unsigned int parity, bits, stb;
> + struct eg20t_port *priv;
> + unsigned long flags;
> +
> + priv = container_of(port, struct eg20t_port, port);
> + switch (termios->c_cflag & CSIZE) {
> + case CS5:
> + bits = PCH_UART_HAL_5BIT;
> + break;
> + case CS6:
> + bits = PCH_UART_HAL_6BIT;
> + break;
> + case CS7:
> + bits = PCH_UART_HAL_7BIT;
> + break;
> + default: /* CS8 */
> + bits = PCH_UART_HAL_8BIT;
> + break;
> + }
> + if (termios->c_cflag & CSTOPB)
> + stb = PCH_UART_HAL_STB2;
> + else
> + stb = PCH_UART_HAL_STB1;
> +
> + if (termios->c_cflag & PARENB) {
> + if (!(termios->c_cflag & PARODD))
> + parity = PCH_UART_HAL_PARITY_ODD;
> + else
> + parity = PCH_UART_HAL_PARITY_EVEN;
> +
> + } else {
> + parity = PCH_UART_HAL_PARITY_NONE;
> + }
If you don't support CPARMRK then you should clear that bit in
termios->c_flag so the caller knows it couldn't be set.
> + baud = uart_get_baud_rate(port, termios, old, 0,
> port->uartclk / 16); +
> + spin_lock_irqsave(&port->lock, flags);
> +
> + uart_update_timeout(port, termios->c_cflag, baud);
> + pch_uart_hal_set_line(priv, baud, parity, bits, stb);
Baud rate should also be written back here
/* Don't rewrite B0 */
if (tty_termios_baud_rate(termios))
tty_termios_encode_baud_rate(termios, baud, baud);
> + txbuf = __get_free_page(GFP_KERNEL|GFP_DMA);
> + if (!txbuf)
> + goto init_port_error_end;
> +
> + rxbuf = __get_free_page(GFP_KERNEL|GFP_DMA);
> + if (!rxbuf)
> + goto init_port_free_txbuf;
No - for bus masterable DMA buffers use the dma_alloc_coherent
interfaces with the correct device pointer, otherwise it will break on
a system with an IOMMU.
I assume the correct device in this case would be the DMA controller ?
The big thing I don't understand here is the locking model - what stops
interrupts and other things interfering with each other. For almost all
of the calls coming from the serial layer the port lock protects them
but I see no protection on the IRQ side at all ?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists