RE: [PATCH 1/2] tty: goldfish: Use streaming DMA for r/w operations on Ranchu platforms

[Miodrag Dinic]

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@xxxxxxxxxx]
Sent: Tuesday, August 22, 2017 9:00 PM
To: Aleksandar Markovic
Cc: linux-kernel@xxxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; jslaby@xxxxxxxx; alan@xxxxxxxxxxxxxxx; jinqian@xxxxxxxxxxx; Aleksandar Markovic; Miodrag Dinic; Petar Jovanovic; Raghu Gandham; James Hogan; ralf@xxxxxxxxxxxxxx
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