| 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: Wed, 23 Aug 2017 10:46:45 +0000
From: Miodrag Dinic <Miodrag.Dinic@...tec.com>
To: Dan Carpenter <dan.carpenter@...cle.com>,
Aleksandar Markovic <aleksandar.markovic@...rk.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"jslaby@...e.com" <jslaby@...e.com>,
"alan@...ux.intel.com" <alan@...ux.intel.com>,
"jinqian@...roid.com" <jinqian@...roid.com>,
Aleksandar Markovic <Aleksandar.Markovic@...tec.com>,
Petar Jovanovic <Petar.Jovanovic@...tec.com>,
Raghu Gandham <Raghu.Gandham@...tec.com>,
James Hogan <James.Hogan@...tec.com>,
"ralf@...ux-mips.org" <ralf@...ux-mips.org>
Subject: RE: [PATCH 1/2] tty: goldfish: Use streaming DMA for r/w operations
on Ranchu platforms
Hello Dan,
thank you for your comments. Please find the answers inline:
> >
> > + GOLDFISH_TTY_VERSION = 0x20,
>
> This is an offset, right? It's not version 32? The name is misleading.
> Maybe call it GOLDFISH_VERSION_REG.> -static void goldfish_tty_do_write(int line, const char *buf, unsigned count)
Yes I guess it is a little bit misleading.
We will refactor this according to your suggestion in the next version.
Thanks.
> > +static inline void do_rw_io(struct goldfish_tty *qtty,
> ^^^^^^
> Don't make functions inline. Leave that for the compiler to decide.
> It's smarter than we are and it ignores our input anyway.
It will be fixed in next version.
> + if (qtty->version) {
>
> It's cleaner to write be "if (qtty->version > 0)" because the version
> numbers are numbers and not booleans.
Ditto.
> > + if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(32))) {
> > + dev_err(&pdev->dev, "No suitable DMA available.\n");
> > + goto err_create_driver_failed;
>
> This goto is wrong and also "ret" isn't necessarily set correctly.
> It's better to preserve the error code from dma_set_mask().
>
> "Goto err_create_driver_failed" basically says that we just called
> create_driver(). It's a come-from label name which is rubbish because
> I can see just from looking at the patch, without any other context,
> that actually dma_set_mask() failed and not create_driver()... Label
> names should say what the goto does. In this case what we want to do is
> decrement goldfish_tty_current_line_count and call
> goldfish_tty_delete_driver() if we're the last user. A good label name
> for this would be "goto dec_line_count;". But actually the label which
> does this is called "goto err_request_irq_failed;"...
Thank you for the suggestions. We will make appropriate changes and submit
the next patch version soon.
Kind regards,
Miodrag
________________________________________
From: Dan Carpenter [dan.carpenter@...cle.com]
Sent: Tuesday, August 22, 2017 9:00 PM
To: Aleksandar Markovic
Cc: linux-kernel@...r.kernel.org; gregkh@...uxfoundation.org; jslaby@...e.com; alan@...ux.intel.com; jinqian@...roid.com; Aleksandar Markovic; Miodrag Dinic; Petar Jovanovic; Raghu Gandham; James Hogan; ralf@...ux-mips.org
Subject: Re: [PATCH 1/2] tty: goldfish: Use streaming DMA for r/w operations on Ranchu platforms
On Tue, Aug 22, 2017 at 05:56:36PM +0200, Aleksandar Markovic wrote:
> drivers/tty/goldfish.c | 120 ++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 108 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/tty/goldfish.c b/drivers/tty/goldfish.c
> index 996bd47..ac98d5a 100644
> --- a/drivers/tty/goldfish.c
> +++ b/drivers/tty/goldfish.c
> @@ -22,6 +22,8 @@
> #include <linux/io.h>
> #include <linux/module.h>
> #include <linux/goldfish.h>
> +#include <linux/mm.h>
> +#include <linux/dma-mapping.h>
>
> enum {
> GOLDFISH_TTY_PUT_CHAR = 0x00,
> @@ -32,6 +34,8 @@ enum {
> GOLDFISH_TTY_DATA_LEN = 0x14,
> GOLDFISH_TTY_DATA_PTR_HIGH = 0x18,
>
> + GOLDFISH_TTY_VERSION = 0x20,
This is an offset, right? It's not version 32? The name is misleading.
Maybe call it GOLDFISH_VERSION_REG.
> +
> GOLDFISH_TTY_CMD_INT_DISABLE = 0,
> GOLDFISH_TTY_CMD_INT_ENABLE = 1,
> GOLDFISH_TTY_CMD_WRITE_BUFFER = 2,
> @@ -45,6 +49,8 @@ struct goldfish_tty {
> u32 irq;
> int opencount;
> struct console console;
> + u32 version;
> + struct device *dev;
> };
>
> static DEFINE_MUTEX(goldfish_tty_lock);
> @@ -53,24 +59,93 @@ static u32 goldfish_tty_line_count = 8;
> static u32 goldfish_tty_current_line_count;
> static struct goldfish_tty *goldfish_ttys;
>
> -static void goldfish_tty_do_write(int line, const char *buf, unsigned count)
> +static inline void do_rw_io(struct goldfish_tty *qtty,
^^^^^^
Don't make functions inline. Leave that for the compiler to decide.
It's smarter than we are and it ignores our input anyway.
> + unsigned long address,
> + unsigned int count,
> + int is_write)
> {
> unsigned long irq_flags;
> - struct goldfish_tty *qtty = &goldfish_ttys[line];
> void __iomem *base = qtty->base;
> +
> spin_lock_irqsave(&qtty->lock, irq_flags);
> - gf_write_ptr(buf, base + GOLDFISH_TTY_DATA_PTR,
> - base + GOLDFISH_TTY_DATA_PTR_HIGH);
> + gf_write_ptr((void *)address, base + GOLDFISH_TTY_DATA_PTR,
> + base + GOLDFISH_TTY_DATA_PTR_HIGH);
> writel(count, base + GOLDFISH_TTY_DATA_LEN);
> - writel(GOLDFISH_TTY_CMD_WRITE_BUFFER, base + GOLDFISH_TTY_CMD);
> +
> + if (is_write)
> + writel(GOLDFISH_TTY_CMD_WRITE_BUFFER, base + GOLDFISH_TTY_CMD);
> + else
> + writel(GOLDFISH_TTY_CMD_READ_BUFFER, base + GOLDFISH_TTY_CMD);
> +
> spin_unlock_irqrestore(&qtty->lock, irq_flags);
> }
>
> +static inline void goldfish_tty_rw(struct goldfish_tty *qtty,
> + unsigned long addr,
> + unsigned int count,
> + int is_write)
> +{
> + dma_addr_t dma_handle;
> + enum dma_data_direction dma_dir;
> +
> + dma_dir = (is_write ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
> + if (qtty->version) {
It's cleaner to write be "if (qtty->version > 0)" because the version
numbers are numbers and not booleans.
> + /*
> + * Goldfish TTY for Ranchu platform uses
> + * physical addresses and DMA for read/write operations
> + */
> + unsigned long addr_end = addr + count;
> +
> + while (addr < addr_end) {
> + unsigned long pg_end = (addr & PAGE_MASK) + PAGE_SIZE;
> + unsigned long next =
> + pg_end < addr_end ? pg_end : addr_end;
> + unsigned long avail = next - addr;
> +
> + /*
> + * Map the buffer's virtual address to the DMA address
> + * so the buffer can be accessed by the device.
> + */
> + dma_handle = dma_map_single(qtty->dev, (void *)addr,
> + avail, dma_dir);
> +
> + if (dma_mapping_error(qtty->dev, dma_handle)) {
> + dev_err(qtty->dev, "tty: DMA mapping error.\n");
> + return;
> + }
> + do_rw_io(qtty, dma_handle, avail, is_write);
> +
> + /*
> + * Unmap the previously mapped region after
> + * the completion of the read/write operation.
> + */
> + dma_unmap_single(qtty->dev, dma_handle, avail, dma_dir);
> +
> + addr += avail;
> + }
> + } else {
> + /*
> + * Old style Goldfish TTY used on the Goldfish platform
> + * uses virtual addresses.
> + */
> + do_rw_io(qtty, addr, count, is_write);
> + }
> +}
> +
> +static void goldfish_tty_do_write(int line, const char *buf,
> + unsigned int count)
> +{
> + struct goldfish_tty *qtty = &goldfish_ttys[line];
> + unsigned long address = (unsigned long)(void *)buf;
> +
> + goldfish_tty_rw(qtty, address, count, 1);
> +}
> +
> static irqreturn_t goldfish_tty_interrupt(int irq, void *dev_id)
> {
> struct goldfish_tty *qtty = dev_id;
> void __iomem *base = qtty->base;
> - unsigned long irq_flags;
> + unsigned long address;
> unsigned char *buf;
> u32 count;
>
> @@ -79,12 +154,10 @@ static irqreturn_t goldfish_tty_interrupt(int irq, void *dev_id)
> return IRQ_NONE;
>
> count = tty_prepare_flip_string(&qtty->port, &buf, count);
> - spin_lock_irqsave(&qtty->lock, irq_flags);
> - gf_write_ptr(buf, base + GOLDFISH_TTY_DATA_PTR,
> - base + GOLDFISH_TTY_DATA_PTR_HIGH);
> - writel(count, base + GOLDFISH_TTY_DATA_LEN);
> - writel(GOLDFISH_TTY_CMD_READ_BUFFER, base + GOLDFISH_TTY_CMD);
> - spin_unlock_irqrestore(&qtty->lock, irq_flags);
> +
> + address = (unsigned long)(void *)buf;
> + goldfish_tty_rw(qtty, address, count, 0);
> +
> tty_schedule_flip(&qtty->port);
> return IRQ_HANDLED;
> }
> @@ -271,6 +344,29 @@ static int goldfish_tty_probe(struct platform_device *pdev)
> qtty->port.ops = &goldfish_port_ops;
> qtty->base = base;
> qtty->irq = irq;
> + qtty->dev = &pdev->dev;
> +
> + /* Goldfish TTY device used by the Goldfish emulator
> + * should identify itself with 0, forcing the driver
> + * to use virtual addresses. Goldfish TTY device
> + * on Ranchu emulator (qemu2) returns 1 here and
> + * driver will use physical addresses.
> + */
> + qtty->version = readl(base + GOLDFISH_TTY_VERSION);
> +
> + /* Goldfish TTY device on Ranchu emulator (qemu2)
> + * will use DMA for read/write IO operations.
> + */
> + if (qtty->version > 0) {
> + /* Initialize dma_mask to 32-bits.
> + */
> + if (!pdev->dev.dma_mask)
> + pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
> + if (dma_set_mask(&pdev->dev, DMA_BIT_MASK(32))) {
> + dev_err(&pdev->dev, "No suitable DMA available.\n");
> + goto err_create_driver_failed;
This goto is wrong and also "ret" isn't necessarily set correctly.
It's better to preserve the error code from dma_set_mask().
"Goto err_create_driver_failed" basically says that we just called
create_driver(). It's a come-from label name which is rubbish because
I can see just from looking at the patch, without any other context,
that actually dma_set_mask() failed and not create_driver()... Label
names should say what the goto does. In this case what we want to do is
decrement goldfish_tty_current_line_count and call
goldfish_tty_delete_driver() if we're the last user. A good label name
for this would be "goto dec_line_count;". But actually the label which
does this is called "goto err_request_irq_failed;"...
regards,
dan carpenter
Powered by blists - more mailing lists