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

Message-ID: <6df192ef-abc1-35a6-298d-e3e67655ac1f@intel.com>
Date:   Fri, 19 Mar 2021 14:56:37 +0800
From:   Jie Deng <jie.deng@...el.com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     Arnd Bergmann <arnd@...db.de>,
        "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 2021/3/19 14:35, Viresh Kumar 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 {
>          struct module *            owner;                /*     0     8 */
>          unsigned int               class;                /*     8     4 */
>
>          /* XXX 4 bytes hole, try to pack */
>
>          const struct i2c_algorithm  * algo;              /*    16     8 */
>          void *                     algo_data;            /*    24     8 */
>          const struct i2c_lock_operations  * lock_ops;    /*    32     8 */
>          struct rt_mutex            bus_lock;             /*    40    32 */
>          /* --- cacheline 1 boundary (64 bytes) was 8 bytes ago --- */
>          struct rt_mutex            mux_lock;             /*    72    32 */
>          int                        timeout;              /*   104     4 */
>          int                        retries;              /*   108     4 */
>          struct device              dev;                  /*   112   744 */
>
>          /* XXX last struct has 7 bytes of padding */
>
>          /* --- cacheline 13 boundary (832 bytes) was 24 bytes ago --- */
>          long unsigned int          locked_flags;         /*   856     8 */
>          int                        nr;                   /*   864     4 */
>          char                       name[48];             /*   868    48 */
>
>          /* XXX 4 bytes hole, try to pack */
>
>          /* --- cacheline 14 boundary (896 bytes) was 24 bytes ago --- */
>          struct completion          dev_released;         /*   920    32 */
>          struct mutex               userspace_clients_lock; /*   952    32 */
>          /* --- cacheline 15 boundary (960 bytes) was 24 bytes ago --- */
>          struct list_head           userspace_clients;    /*   984    16 */
>          struct i2c_bus_recovery_info * bus_recovery_info; /*  1000     8 */
>          const struct i2c_adapter_quirks  * quirks;       /*  1008     8 */
>          struct irq_domain *        host_notify_domain;   /*  1016     8 */
>          /* --- cacheline 16 boundary (1024 bytes) --- */
>
>          /* size: 1024, cachelines: 16, members: 19 */
>          /* sum members: 1016, holes: 2, sum holes: 8 */
>          /* paddings: 1, sum paddings: 7 */
> };
>
> 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.


I will remove that structure and update the members in probe.