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: <20210319063553.eq5aorcyiame6u2e@vireshk-i7>
Date:   Fri, 19 Mar 2021 12:05:53 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Jie Deng <jie.deng@...el.com>
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 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.

-- 
viresh