linux-kernel - Re: [PATCH v7 01/10] i3c: Add core I3C infrastructure

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]

Date:   Thu, 6 Sep 2018 15:38:49 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Boris Brezillon <boris.brezillon@...tlin.com>
Cc:     Wolfram Sang <wsa@...-dreams.de>,
        Linux I2C <linux-i2c@...r.kernel.org>,
        gregkh <gregkh@...uxfoundation.org>,
        Jonathan Corbet <corbet@....net>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        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>,
        Ian Campbell <ijc+devicetree@...lion.org.uk>,
        Kumar Gala <galak@...eaurora.org>,
        DTML <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Vitor Soares <Vitor.Soares@...opsys.com>,
        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>, mshettel@...eaurora.org,
        swboyd@...omium.org
Subject: Re: [PATCH v7 01/10] i3c: Add core I3C infrastructure

On Wed, Sep 5, 2018 at 5:41 PM Boris Brezillon
<boris.brezillon@...tlin.com> wrote:

> ---
> Changes in v7:
> - Stop representing the I3C master as a device under the I3C bus
> - Enforce a 1:1 relationship between i3c_bus and i3c_master_controller
>   objects

Thanks for implementing those changes. What is your feeling so far
about the difference? Has it gotten much simpler as I was hoping?

I definitely like this version much better. I have found a couple of
things that I point out below that could be improved (or me being
proven wrong on them), but overall I think it looks great and I don't
see major issues.

Great work!

> +struct i3c_bus *i3c_bus_create(struct i3c_master_controller *master)
> +{
> +       struct i3c_bus *i3cbus;
> +       int ret;
> +
> +       i3cbus = kzalloc(sizeof(*i3cbus), GFP_KERNEL);
> +       if (!i3cbus)
> +               return ERR_PTR(-ENOMEM);

I find it a bit confusing to have separate i3c_master_controller
and i3c_bus structures with this version. Why not merge the
two structures into one now and move the bus management
into master.c?

> +static int i3c_master_attach_i3c_dev(struct i3c_master_controller *master,
> +                                    struct i3c_dev_desc *dev)
> +{
> +       int ret;
> +
> +       /*
> +        * We don't attach devices to the controller until they are
> +        * addressable on the bus.
> +

Apparently the new gmail version decided to cut off the second half of your
email after this line when I hit reply, which makes it much harder for me
to continue a proper review. I fear I'll have to get a real email client
again :(

> + * The I3C bus is represented with its own object and not implicitly described
> + * by the I3C master to cope with the multi-master functionality, where one bus
> + * can be shared amongst several masters, each of them requesting bus ownership
> + * when they need to.

This comment is now stale, even without merging the structures, right?

> +struct i3c_master_controller {
> +       struct device *parent;
> +       struct i3c_dev_desc *this;
> +       struct i2c_adapter i2c;

I think the 'parent' pointer is better omitted, it should always be
the same as master->dev->parent, right?

Since it contains an i2c_adapter, maybe a good name for the
combined i3c_master_controller+i3c_bus structure would
be 'i3c_adapter'?

+#define i3c_bus_for_each_i2cdev(bus, dev)                              \
+       list_for_each_entry(dev, &(bus)->devs.i2c, common.node)
+
+#define i3c_bus_for_each_i3cdev(bus, dev)                              \
+       list_for_each_entry(dev, &(bus)->devs.i3c, common.node)

I wonder if it would simplify things to drop the i2c and i3c
device lists and instead implement these for_each loops
based on device_for_each_child() with a check of the
bus_type==&i2c_bus/&i3c_bus. That might help with locking
and keeping the two lists synchronized, which may or
may not be a problem here.

         Arnd