linux-kernel - Re: [PATCH v3 2/2] iio: magnetometer: si7210: add driver for Si7210

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]

Message-ID: <20250114235323.3xkktco7fsb6pmzk@antoni-VivoBook-ASUSLaptop-X512FAY-K512FA>
Date: Wed, 15 Jan 2025 00:53:23 +0100
From: Antoni Pokusinski <apokusinski01@...il.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: jic23@...nel.org, lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, andrej.skvortzov@...il.com,
	neil.armstrong@...aro.org, icenowy@...c.io, megi@....cz,
	danila@...xyga.com, javier.carrasco.cruz@...il.com, andy@...nel.org,
	linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v3 2/2] iio: magnetometer: si7210: add driver for Si7210

On Tue, Jan 14, 2025 at 11:43:11AM +0200, Andy Shevchenko wrote:
> On Tue, Jan 14, 2025 at 12:19 AM Antoni Pokusinski
> <apokusinski01@...il.com> wrote:
> 
> > Thanks for the review. I'm currently implementing some changes in the
> > driver according to the review, however I have some doubts regarding
> > removal of the `i2c_client` from `si7210_data`.
> 
> ...
> 
> > > > +struct si7210_data {
> > > > +       struct i2c_client *client;
> > >
> > > Do we really need a room for that? Isn't it derivable from the below
> > > regmap? Also note the frequency of use of client vs. regmap. The
> > > result in the object file can be much better if regmap becomes the
> > > first member here. Check it (with bloat-o-meter, for example).
> > >
> >
> > I used arm-linux-nm and the bloat-o-meter to compare the sizes and it
> > turned out that the version which contains the `i2c_client` has
> > slightly smaller size actually. Here are the results:
> >
> > $ ./scripts/bloat-o-meter -p arm-linux-  ./old_si7210.ko  ./new_si7210.ko
> > add/remove: 0/0 grow/shrink: 1/0 up/down: 4/0 (4)
> > Function                                     old     new   delta
> > si7210_probe                                 556     560      +4
> > Total: Before=4021, After=4025, chg +0.10%
> >
> > Here is the diff (shortened for better readability) between
> > the old_si7210.ko (uses `si7210_data->i2c_client`) and
> > new_si7210.ko (does not use `si7210_data->i2c_client`):
> >
> >  struct si7210_data {
> > -       struct i2c_client *client;
> >         struct regmap *regmap;
> > ...
> >  static int si7210_device_wake(struct si7210_data *data)
> >  {
> > +       struct device *dev = regmap_get_device(data->regmap);
> >         int ret;
> >
> > -       ret = i2c_smbus_read_byte(data->client);
> > +       ret = i2c_smbus_read_byte(to_i2c_client(dev));
> > ...
> > static int si7210_probe(struct i2c_client *client)
> >         data = iio_priv(indio_dev);
> > -       data->client = client;
> >
> > Hence, I guess that it's actually better to leave the `i2c_client` as it is.
> 
> I don't think you have tested all that I was talking about, i.e. have
> you tried to swap the positions of client and regmap? What I expect is
> that when you swap them you will see a good size reduction due to
> pointer arithmetics becoming no-op for the regmap pointer. And then
> the dropping of the client might waste all that beneficial size.
> 

Ok, so I've tried to swap the `i2c_client` and `regmap` pointers and...
there was no change shown by the bloat-o-meter. The only improvement was
that the new object file (that is after moving the `regmap` to the
beginning of the struct) was 8 bytes smaller in file size.

Out of curiosity I've also tried moving
the `regmap` further away in the structure (e.g. I placed it after the
regulator and mutex) but there was still no change. I am a bit confused,
since this behavior is different from what you described that it should
be.

> > > > +       struct regmap *regmap;
> > > > +       struct regulator *vdd;
> > > > +       struct mutex fetch_lock; /* lock for a single measurement fetch */
> > > > +       s8 temp_offset;
> > > > +       s8 temp_gain;
> > > > +       s8 scale_20_a[A_REGS_COUNT];
> > > > +       s8 scale_200_a[A_REGS_COUNT];
> > > > +       u8 curr_scale;
> > > > +};
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Kinds regards,
Antoni