| 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: <20100913172544.a81e8422.akpm@linux-foundation.org>
Date: Mon, 13 Sep 2010 17:25:44 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: Manuel Stahl <manuel.stahl@....fraunhofer.de>
Cc: linux-kernel@...r.kernel.org, Greg KH <greg@...ah.com>
Subject: Re: [PATCH] Add sc16is7x2 driver
On Fri, 03 Sep 2010 15:11:53 +0200
Manuel Stahl <manuel.stahl@....fraunhofer.de> wrote:
> This patch adds support for the sc16is7x2 chips.
>
The patch was pretty badly wordwrapped.
>
> ...
>
> +#define WRITE_CMD(reg, ch) (REG_WRITE | (reg & 0xf) << 3 | (ch & 0x1) << 1)
> +#define READ_CMD(reg, ch) (REG_READ | (reg & 0xf) << 3 | (ch & 0x1) << 1)
It would be better to implement these as lower-case-named inlined C
functions.
> +
> +/* 16bit SPI command to read or write a register */
> +struct sc16is7x2_spi_reg {
> + u8 cmd;
> + u8 value;
> +} __attribute__ ((packed));
We have a __packed helper macro for this.
> +struct sc16is7x2_chip;
> +
> +/*
> + * Some registers must be read back to modify.
> + * To save time we cache them here in memory
That implies that the caches have some locking (the mutex, perhaps?).
Documenting that here would be appropriate.
>
> ...
>
> +/* ******************************** SPI
> ********************************* */
> +
> +
> +/*
> + * Reserve memory for command sequence
> + * @param cnt number of commands
This "@param" stuff is some form of markup which the kernel does not
use. Please use standard kerneldoc markup throughout the driver, or
just plain old English-language comments.
> + */
> +static inline struct sc16is7x2_spi_reg *
> +sc16is7x2_alloc_spi_cmds(unsigned cnt)
> +{
> + return kzalloc(sizeof(struct sc16is7x2_spi_reg)*cnt, GFP_KERNEL);
Use kcalloc() here.
> +}
> +
>
> ...
>
> +/*
> + * sc16is7x2_write_async - Write a new register content (async)
> + */
> +static inline int sc16is7x2_write_async(struct spi_device *spi, u8 reg,
> u8 ch,
> + u8 value)
> +{
> + struct sc16is7x2_spi_reg *cmd = sc16is7x2_alloc_spi_cmds(1);
> + if (!cmd)
> + return -ENOMEM;
> + sc16is7x2_add_write_cmd(cmd, reg, ch, value);
> + return sc16is7x2_spi_async(spi, cmd, 1);
> +}
It's comventional to place a blank line between end-of-definitions and
start-of-code. The driver has many instances of this.
> +/*
> + * sc16is7x2_write - Write a new register content (sync)
> + */
> +static int sc16is7x2_write(struct spi_device *spi, u8 reg, u8 ch, u8 val)
> +{
> + u16 word = REG_WRITE | (reg & 0xf) << 3 | (ch & 0x3) << 1 | val << 8;
> + return spi_write(spi, (const u8 *)&word, sizeof(word));
> +}
There's another.
> +/**
> + * sc16is7x2_read - Read back register content
> + * @spi: The SPI device
> + * @reg: Register offset
> + *
> + * Returns positive 8 bit value from device if successful or a
> + * negative value on error
> + */
ah-hah. That's a correct kerneldoc comment.
> +static int sc16is7x2_read(struct spi_device *spi, unsigned reg,
> unsigned ch)
> +{
> + u8 cmd = REG_READ | (reg & 0xf) << 3 | (ch & 0x3) << 1;
> + return spi_w8r8(spi, cmd);
> +}
> +
> +/* ******************************** UART
> ********************************* */
> +
> +/* Uart divisor latch write */
> +static inline void sc16is7x2_add_dl_write_cmd(struct sc16is7x2_spi_reg
> *cmd,
> + u8 ch, int value)
> +{
> + sc16is7x2_add_write_cmd(&cmd[0], UART_DLL, ch, value & 0xff);
> + sc16is7x2_add_write_cmd(&cmd[1], UART_DLM, ch, value >> 8 & 0xff);
> +}
Probably the driver doesn't need any explicit "inline" usage at all -
modern gcc's work all that out for themselves. And if gcc disagreed
with your inline directive, it will just ignore it.
> +static unsigned int sc16is7x2_tx_empty(struct uart_port *port)
> +{
> + struct sc16is7x2_channel *chan =
> + container_of(port, struct sc16is7x2_channel, uart);
> + struct sc16is7x2_chip *ts = chan->chip;
> + unsigned lsr;
> +
> + dev_dbg(&ts->spi->dev, "%s\n", __func__);
> +
> + mutex_lock(&chan->lock);
> + lsr = chan->lsr;
> + mutex_unlock(&chan->lock);
It's strange to put locking around a single atomic read. What are we
trying to do here?
> + return lsr & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
> +}
> +
>
> ...
>
> +static void sc16is7x2_break_ctl(struct uart_port *port, int break_state)
> +{
> + struct sc16is7x2_channel *chan =
> + container_of(port, struct sc16is7x2_channel, uart);
> + struct sc16is7x2_chip *ts = chan->chip;
> + unsigned ch = port->line & 0x01;
> + unsigned long flags;
> +
> + dev_dbg(&ts->spi->dev, "%s\n", __func__);
> +
> + spin_lock_irqsave(&chan->uart.lock, flags);
> + if (break_state == -1)
> + chan->lcr |= UART_LCR_SBC;
> + else
> + chan->lcr &= ~UART_LCR_SBC;
> + spin_unlock_irqrestore(&chan->uart.lock, flags);
hm. Above we use the mutex to protect char->lcr but here we're using a
spinlock.
> + sc16is7x2_write_async(ts->spi, UART_LCR, ch, chan->lcr);
> +}
> +
>
> ...
>
> +#define MIN(a, b) ((a < b) ? (a) : (b))
Nope. Use the min() from include/linux/kernel.h.
>
> ...
>
> +static irqreturn_t sc16is7x2_interrupt(int irq, void *dev_id)
> +{
> + struct sc16is7x2_chip *ts = dev_id;
> +
> + dev_dbg(&ts->spi->dev, "%s\n", __func__);
> +
> + if (!ts->force_end_work && !work_pending(&ts->work) &&
> + !freezing(current) && !ts->suspending)
> + queue_work(ts->workqueue, &ts->work);
> +
> + return IRQ_HANDLED;
> +}
The kernel has infrastructure for "threaded irqs" nowadays. What this
driver is doing basically reimplements that concept. Did you consider
using threaded IRQs directly?
>
> ...
>
--
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