| 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: <1236685698.5183.86.camel@dy>
Date: Tue, 10 Mar 2009 19:48:18 +0800
From: gyang <graf.yang@...log.com>
To: graff yang <graff.yang@...il.com>
Cc: Mike Frysinger <vapier.adi@...il.com>, samuel@...tiz.org,
irda-users@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
cooloney@...nel.org
Subject: Re: [PATCH] [net/irda]: new Blackfin on-chip SIR IrDA driver
On Tue, 2009-03-10 at 19:25 +0800, graff yang wrote:
>
>
> On Tue, Mar 10, 2009 at 4:03 PM, Mike Frysinger <vapier.adi@...il.com>
> wrote:
> 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 ...
>
> SIR hasn't encounter such anomalies. Anyway, I will add it.
>
>
>
> > + 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.
But we should prevent changing speed when the byte is sending out.
> 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 ?
>
> The initial value of baudrate_mask is IR_9600.
>
>
>
> > + 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 "." ? :)
>
> Thanks.
>
>
>
>
> > --- /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