| 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
| ||
|
Date: Tue, 10 Mar 2009 04:03:10 -0400
From: Mike Frysinger <vapier.adi@...il.com>
To: graff.yang@...il.com
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 Tue, Mar 10, 2009 at 03:29, <graff.yang@...il.com> wrote:
> +config BFIN_SIR3
> + bool "Blackfin SIR on UART3"
> +config BFIN_SIR1
> + bool "Blackfin SIR on UART1"
> +config BFIN_SIR0
> + bool "Blackfin SIR on UART0"
> +config BFIN_SIR2
> + bool "Blackfin SIR on UART2"
looks like an odd order for things. or maybe you count to 3
differently from me :).
> +static int bfin_sir_set_speed(struct bfin_sir_port *port, int speed)
> +{
> + int ret = -EINVAL;
> + unsigned int quot;
> + unsigned short val, lsr, lcr = 0;
> +
> + lcr = WLS(8);
the lcr init to 0 looks pretty pointless to me ...
> + quot = (port->clk + (8 * speed)) / (16 * speed);
isnt the SIR affected by the same anomalies as the UART ? in other
words, you just made that adjustment to the UART recently ...
> + do {
> + lsr = SIR_UART_GET_LSR(port);
> + } while (!(lsr & TEMT));
i'm pretty sure we determined that it is not the job of the kernel to
make sure the line is clear before we go changing speeds. plus, we
had a few bugs on the UART driver when polling for bits on a
peripheral that wasnt enabled yet ...
> + SIR_UART_PUT_DLL(port, quot & 0xFF);
> + SSYNC();
> + SIR_UART_PUT_DLH(port, (quot >> 8) & 0xFF);
> + SSYNC();
i'm pretty sure that first SSYNC is not needed
> + /*printk(KERN_DEBUG "bfin_sir: Set new speed %d\n", speed);*/
pr_debug() ?
> +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);
> + if (!(get_dma_curr_irqstat(port->tx_dma_channel)&DMA_RUN)) {
really should be whitespace around that &
> +static irqreturn_t bfin_sir_dma_rx_int(int irq, void *dev_id)
> +{
> +....
> + mod_timer(&(port->rx_dma_timer), jiffies + DMA_SIR_RX_FLUSH_JIFS);
those paren are unnecessary
> +static int bfin_sir_startup(struct bfin_sir_port *port, struct net_device *dev)
> +{
> +#ifdef CONFIG_SIR_BFIN_DMA
> + dma_addr_t dma_handle;
> +#endif /* CONFIG_SIR_BFIN_DMA */
> +
> + if (request_dma(port->rx_dma_channel, "BFIN_UART_RX") < 0) {
> + printk(KERN_WARNING "bfin_sir: Unable to attach SIR RX DMA channel\n");
should be dev_warn(&dev->dev, ...) ? if so, i'd check all the places
where printk() is used when a net_device is available ...
> +static int __devinit bfin_sir_probe(struct platform_device *pdev)
> +{
> + struct net_device *dev;
> + struct bfin_sir_self *self;
> + unsigned int baudrate_mask;
> + struct bfin_sir_port *sir_port;
> + int err = 0;
> +
> + err = peripheral_request(per[pdev->id][0], DRIVER_NAME);
> + if (err)
> + return err;
> + err = peripheral_request(per[pdev->id][1], DRIVER_NAME);
> + if (err)
> + return err;
the first per list was just leaked ...
> + sir_port = kmalloc(sizeof(struct bfin_sir_port), GFP_KERNEL);
sizeof(*sir_port)
> + dev = alloc_irdadev(sizeof(struct bfin_sir_self));
sizeof(*dev)
> + baudrate_mask = IR_9600;
> +
> + switch (max_rate) {
> + case 115200:
> + baudrate_mask |= IR_115200;
> + case 57600:
> + baudrate_mask |= IR_57600;
> + case 38400:
> + baudrate_mask |= IR_38400;
> + case 19200:
> + baudrate_mask |= IR_19200;
> + }
what if someone specified max_rate = 1231245 ?
> + if (err) {
> +err_mem_3:
> + kfree(self->tx_buff.head);
> +err_mem_2:
> + kfree(self->rx_buff.head);
> +err_mem_1:
> + free_netdev(dev);
> +err_mem_0:
> + kfree(sir_port);
> + }
the peripheral pins are leaked here as well
> + if (err == 0)
> + platform_set_drvdata(pdev, sir_port);
considering you tested "if (err)" right before, an "} else" would make
more sense
> +MODULE_AUTHOR("Graf.Yang <graf.yang@...log.com>");
i think your name has no "." ? :)
> --- /dev/null
> +++ b/drivers/net/irda/bfin_sir.h
> +#ifdef CONFIG_SIR_BFIN_DMA
> +struct dma_rx_buf {
> + char *buf;
> + int head;
> + int tail;
> + };
no indentation in closing brace
> +static unsigned short per[][2] = {
> + {P_UART0_RX, P_UART0_TX},
> + {P_UART1_RX, P_UART1_TX},
> + {P_UART2_RX, P_UART2_TX},
> + {P_UART3_RX, P_UART3_TX},
> + };
no indentation in closing brace and really this should be const
-mike
--
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