| 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: <20090306145832.80f1a382.akpm@linux-foundation.org>
Date: Fri, 6 Mar 2009 14:58:32 -0800
From: Andrew Morton <akpm@...ux-foundation.org>
To: Bryan Wu <cooloney@...nel.org>
Cc: samuel@...tiz.org, irda-users@...ts.sourceforge.net,
linux-kernel@...r.kernel.org, graf.yang@...log.com,
cooloney@...nel.org
Subject: Re: [PATCH] [net/irda]: new Blackfin on-chip SIR IrDA driver
On Fri, 6 Mar 2009 14:44:45 +0800
Bryan Wu <cooloney@...nel.org> wrote:
> From: Graf Yang <graf.yang@...log.com>
>
> Signed-off-by: Graf Yang <graf.yang@...log.com>
> Signed-off-by: Bryan Wu <cooloney@...nel.org>
Mutter. Nothing to be said at all?
> drivers/net/irda/bfin_sir.h | 147 ++++++++
> 4 files changed, 989 insertions(+), 0 deletions(-)
> create mode 100644 drivers/net/irda/bfin_sir.c
> create mode 100644 drivers/net/irda/bfin_sir.h
>
> diff --git a/drivers/net/irda/Kconfig b/drivers/net/irda/Kconfig
> index e631755..0e4d7e0 100644
> --- a/drivers/net/irda/Kconfig
> +++ b/drivers/net/irda/Kconfig
> @@ -17,6 +17,51 @@ config IRTTY_SIR
>
> If unsure, say Y.
>
> +config BFIN_SIR
> + tristate "Blackfin SIR on UART"
> + depends on BLACKFIN && IRDA
> + default n
> + help
> + Say Y here if your want to enable SIR function on Blackfin UART
> + devices.
> +
> + To activate this driver you can start irattach like:
> + "irattach irda0 -s"
> +
> + Saying M, it will be built as a module named bfin_sir.
> +
> + Note that you need to turn off one of the serial drivers for SIR
> + to use that UART.
> +
> +config BFIN_SIR3
> + bool "Blackfin SIR on UART3"
> + depends on BFIN_SIR && !SERIAL_BFIN_UART3 && (BF54x)
> +
> +config BFIN_SIR1
> + bool "Blackfin SIR on UART1"
> + depends on BFIN_SIR && !SERIAL_BFIN_UART1 && (!BF531 && !BF532 && !BF533 && !BF561)
> +
> +config BFIN_SIR0
> + bool "Blackfin SIR on UART0"
> + depends on BFIN_SIR && !SERIAL_BFIN_UART0
> +
> +config BFIN_SIR2
> + bool "Blackfin SIR on UART2"
> + depends on BFIN_SIR && !SERIAL_BFIN_UART2 && (BF54x || BF538 || BF539)
> +
> +choice
> + prompt "SIR Mode"
> + depends on BFIN_SIR
> + default SIR_BFIN_DMA
> +
> +config SIR_BFIN_DMA
> + bool "DMA mode"
> + depends on !DMA_UNCACHED_NONE
> +
> +config SIR_BFIN_PIO
> + bool "PIO mode"
> +endchoice
This arrangement is pretty user-hostile. It requires that people
reconfigure and rebuild their kernel any time they want to change the
driver options.
It would be much better to be able to do this via a kernel boot option.
It would be better still to be able to do this via a module parameter.
It would be better still to be able to do this via a runtime knob:
ioctl, sysfs file, whatever.
it would be better still to do all this automatically at probe time.
What you've chosen here is the worst possible solution, sorry.
> comment "Dongle support"
>
> ...
>
> +static void turnaround_delay(unsigned long last_jif, int mtt)
> +{
> + long ticks;
> +
> + mtt = mtt < 10000 ? 10000 : mtt;
> + ticks = 1 + mtt / (USEC_PER_SEC / HZ);
> + schedule_timeout_interruptible(ticks);
> +}
If the calling task has signal_pending() then the
schedule_timeout_interruptible() will return immediately. I bet this
breaks the driver.
Fixable by using schedule_timeout_uninterruptible().
>
> ...
>
> +static void bfin_sir_stop_tx(struct bfin_sir_port *port)
> +{
> +#ifdef CONFIG_SIR_BFIN_DMA
> + disable_dma(port->tx_dma_channel);
> +#endif
> +
> + while (!(SIR_UART_GET_LSR(port) & THRE))
> + continue;
cpu_relax().
> + SIR_UART_STOP_TX(port);
> +}
> +
>
> ...
>
> +#ifdef CONFIG_SIR_BFIN_PIO
> +static void bfin_sir_tx_chars(struct net_device *dev)
> +{
> + unsigned int chr;
> + struct bfin_sir_self *self = netdev_priv(dev);
> + struct bfin_sir_port *port = self->sir_port;
> +
> + if (self->tx_buff.len != 0) {
> + chr = *(self->tx_buff.data);
> + SIR_UART_PUT_CHAR(port, chr);
> + self->tx_buff.data++;
> + self->tx_buff.len--;
I don't see any locking which protects the tx_buff.
> + } else {
> + self->stats.tx_packets++;
> + self->stats.tx_bytes += self->tx_buff.data - self->tx_buff.head;
> + if (self->newspeed) {
> + bfin_sir_set_speed(port, self->newspeed);
> + self->speed = self->newspeed;
> + self->newspeed = 0;
> + }
> + bfin_sir_stop_tx(port);
> + bfin_sir_enable_rx(port);
> + /* I'm hungry! */
> + netif_wake_queue(dev);
> + }
> +}
> +
> +static void bfin_sir_rx_chars(struct net_device *dev)
> +{
> + struct bfin_sir_self *self = netdev_priv(dev);
> + struct bfin_sir_port *port = self->sir_port;
> + unsigned char ch;
> +
> + SIR_UART_CLEAR_LSR(port);
> + ch = SIR_UART_GET_CHAR(port);
> + async_unwrap_char(dev, &self->stats, &self->rx_buff, ch);
> + dev->last_rx = jiffies;
> +}
> +
> +static irqreturn_t bfin_sir_rx_int(int irq, void *dev_id)
> +{
> + struct net_device *dev = dev_id;
> + struct bfin_sir_self *self = netdev_priv(dev);
> + struct bfin_sir_port *port = self->sir_port;
blank line here.
> + while ((SIR_UART_GET_LSR(port) & DR))
> + bfin_sir_rx_chars(dev);
> +
> + return IRQ_HANDLED;
> +}
> +
>
> ...
>
> +static irqreturn_t bfin_sir_dma_tx_int(int irq, void *dev_id)
> +{
> + struct net_device *dev = dev_id;
> + struct bfin_sir_self *self = netdev_priv(dev);
> + struct bfin_sir_port *port = self->sir_port;
> + spin_lock(&self->lock);
please prefer to put a blank line after end-of-locals and before
start-of-code,
> + if (!(get_dma_curr_irqstat(port->tx_dma_channel)&DMA_RUN)) {
> + clear_dma_irqstat(port->tx_dma_channel);
> + bfin_sir_stop_tx(port);
> +
> + self->stats.tx_packets++;
> + self->stats.tx_bytes += self->tx_buff.len;
> + self->tx_buff.len = 0;
> + if (self->newspeed) {
> + bfin_sir_set_speed(port, self->newspeed);
> + self->speed = self->newspeed;
> + self->newspeed = 0;
> + }
> + bfin_sir_enable_rx(port);
> + /* I'm hungry! */
> + netif_wake_queue(dev);
> + port->tx_done = 1;
> + }
> + spin_unlock(&self->lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void bfin_sir_dma_rx_chars(struct net_device *dev)
> +{
> + struct bfin_sir_self *self = netdev_priv(dev);
> + struct bfin_sir_port *port = self->sir_port;
> + int i;
> +
like that.
> + SIR_UART_CLEAR_LSR(port);
> +
> + for (i = port->rx_dma_buf.head; i < port->rx_dma_buf.tail; i++)
> + async_unwrap_char(dev, &self->stats, &self->rx_buff, port->rx_dma_buf.buf[i]);
> +}
> +
>
> ...
>
> +static int bfin_sir_stop(struct net_device *dev)
> +{
> + struct bfin_sir_self *self = netdev_priv(dev);
> +
> + flush_scheduled_work();
flush_scheduled_work() flushes all work items. It is preferable to use
flush_work() to flush a single item.
> + bfin_sir_shutdown(self->sir_port, dev);
> +
> + if (self->rxskb) {
> + dev_kfree_skb(self->rxskb);
> + self->rxskb = NULL;
> + }
> +
> + /* Stop IrLAP */
> + if (self->irlap) {
> + irlap_close(self->irlap);
> + self->irlap = NULL;
> + }
> +
> + netif_stop_queue(dev);
> + self->open = 0;
> +
> + return 0;
> +}
> +
> +static int bfin_sir_init_iobuf(iobuff_t *io, int size)
> +{
> + io->head = kmalloc(size, GFP_KERNEL | GFP_DMA);
Does it actually _need_ the GFP_DMA?
It is wrong (or at least, contradictory) to combine GFP_KERNEL and
GFP_DMA together in this manner. Use one or the other.
> + if (!io->head)
> + return -ENOMEM;
> + io->truesize = size;
> + io->in_frame = FALSE;
> + io->state = OUTSIDE_FRAME;
> + io->data = io->head;
> + return 0;
> +}
> +
>
> ...
>
--
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