linux-kernel - Re: [PATCH 1/3] i3c: master: Add driver for Synopsys DesignWare IP

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VfYYmr7o6xkDbjaad3HxyzZgODzKfSNWdPXjM+YjTMANA@mail.gmail.com>
Date:   Sat, 21 Jul 2018 18:35:33 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Vitor soares <Vitor.Soares@...opsys.com>
Cc:     Boris Brezillon <boris.brezillon@...tlin.com>,
        Wolfram Sang <wsa@...-dreams.de>,
        linux-i2c <linux-i2c@...r.kernel.org>,
        Jonathan Corbet <corbet@....net>,
        Linux Documentation List <linux-doc@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Przemyslaw Sroka <psroka@...ence.com>,
        Arkadiusz Golec <agolec@...ence.com>,
        Alan Douglas <adouglas@...ence.com>,
        Bartosz Folta <bfolta@...ence.com>,
        Damian Kos <dkos@...ence.com>,
        Alicja Jurasik-Urbaniak <alicja@...ence.com>,
        Cyprian Wronka <cwronka@...ence.com>,
        Suresh Punnoose <sureshp@...ence.com>,
        Rafal Ciepiela <rafalc@...ence.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Nishanth Menon <nm@...com>, Rob Herring <robh+dt@...nel.org>,
        Pawel Moll <pawel.moll@....com>,
        Mark Rutland <mark.rutland@....com>,
        "ijc+devicetree" <ijc+devicetree@...lion.org.uk>,
        Kumar Gala <galak@...eaurora.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Xiang Lin <Xiang.Lin@...aptics.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Sekhar Nori <nsekhar@...com>,
        Przemyslaw Gaj <pgaj@...ence.com>,
        Peter Rosin <peda@...ntia.se>,
        Joao Pinto <Joao.Pinto@...opsys.com>
Subject: Re: [PATCH 1/3] i3c: master: Add driver for Synopsys DesignWare IP

On Fri, Jul 20, 2018 at 11:57 PM, Vitor soares
<Vitor.Soares@...opsys.com> wrote:
> This patch add driver for Synopsys DesignWare IP on top of
> I3C subsystem patchset proposal V6

Some of comments below related to style.
But unaligned helpers I think is good to use.

> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/i3c/master.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>

All of them required? Why?

> +       default:

Just return false here?

> +               break;
> +       }
> +
> +       return false;

> +       for (i = 0; i < nbytes; i += 4) {
> +               u32 data = 0;
> +

> +               for (j = 0; j < 4 && (i + j) < nbytes; j++)
> +                       data |= (u32)bytes[i + j] << (j * 8);

NIH of get_unaligned_le32()

> +
> +               writel(data, master->regs + RX_TX_DATA_PORT);
> +       }
> +}
> +
> +static void dw_i3c_master_read_rx_fifo(struct dw_i3c_master *master,
> +                                      u8 *bytes, int nbytes)
> +{
> +       int i, j;
> +
> +       for (i = 0; i < nbytes; i += 4) {
> +               u32 data;
> +
> +               data = readl(master->regs + RX_TX_DATA_PORT);
> +


> +               for (j = 0; j < 4 && (i + j) < nbytes; j++)
> +                       bytes[i + j] = data >> (j * 8);

Ditto put_unaligned_le32() ?

> +       }
> +}

I'm wondering what else you open coded instead of using helpers we already have.

> +               writel(cmd->cmd_hi, master->regs + COMMAND_QUEUE_PORT);
> +               writel(cmd->cmd_lo, master->regs + COMMAND_QUEUE_PORT);

hmm... writesl()?

> +       info->pid = (u64)readl(master->regs + SLV_PID_VALUE);

Why explicit casting?


> +       u32 r;
> +
> +
> +       core_rate = clk_get_rate(master->core_clk);

Too many blank lines in between.

> +
> +

Ditto.

> +       /* Prepare DAT before launching DAA. */
> +       for (pos = 0; pos < master->maxdevs; pos++) {
> +               if (olddevs & BIT(pos))
> +                       continue;
> +
> +               ret = i3c_master_get_free_addr(m, last_addr + 1);
> +               if (ret < 0)
> +                       return -ENOSPC;
> +               master->addrs[pos] = ret;

> +               p = (ret >> 6) ^ (ret >> 5) ^ (ret >> 4) ^ (ret >> 3) ^
> +                   (ret >> 2) ^ (ret >> 1) ^ ret ^ 1;
> +               p = p & 1;

Is it parity calculus? Do we have something implemented in kernel already?

Btw, https://graphics.stanford.edu/~seander/bithacks.html#ParityNaive
offered this

v ^= v >> 4;
v &= 0xf;
v = (0x6996 >> v) & 1;


-- 
With Best Regards,
Andy Shevchenko