linux-kernel - Re: [PATCH v8] i2c: virtio: add a virtio i2c frontend driver

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] [day] [month] [year] [list]

Date:   Fri, 19 Mar 2021 09:27:11 +0100
From:   Arnd Bergmann <arnd@...db.de>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Jie Deng <jie.deng@...el.com>,
        "Enrico Weigelt, metux IT consult" <lkml@...ux.net>,
        Linux I2C <linux-i2c@...r.kernel.org>,
        virtualization@...ts.linux-foundation.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Wolfram Sang <wsa@...nel.org>,
        Jason Wang <jasowang@...hat.com>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        conghui.chen@...el.com, kblaiech@...lanox.com,
        jarkko.nikula@...ux.intel.com,
        Sergey Semin <Sergey.Semin@...kalelectronics.ru>,
        Mike Rapoport <rppt@...nel.org>, loic.poulain@...aro.org,
        Tali Perry <tali.perry1@...il.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        yu1.wang@...el.com, shuo.a.liu@...el.com,
        Stefan Hajnoczi <stefanha@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH v8] i2c: virtio: add a virtio i2c frontend driver

On Fri, Mar 19, 2021 at 7:35 AM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> On 19-03-21, 14:29, Jie Deng wrote:
> > I also see example drivers/i2c/busses/i2c-xiic.c. Some people might think
> > this way is more clearer than
> >
> > updating each member in probe. Basically, I think it's just a matter of
> > personal preference which doesn't
>
> Memory used by one instance of struct i2c_adapter (on arm64):
>
> struct i2c_adapter {
...
> };
>
> So, this extra instance that i2c-xiic.c is creating (and that you want to
> create) is going to waste 1KB of memory and it will never be used.
>
> This is bad coding practice IMHO and it is not really about personal preference.

Agreed. At the minimum, it should have been written as an explicit
memcpy() in the few drivers that have that pattern instead of a benign
looking struct assignment, but even then there is nothing good about it
really. Notably, the largest member by far is the 'struct device', and
that by itself should be a red flag as a device is never meant to be
allocated statically (this used to be common in pre-DT days, but
even then was considered bad style).

I suppose the i2c_add_adapter()/i2c_add_numbered_adapter()
interface could be redesigned to handle this better, since every
driver needs to set the same few fields. That however requires finding
someone to spend the effort on coming up with a nice design and
converting lots of drivers over.

       Arnd