netdev - Re: [PATCH 1/3] can: rcar_canfd: Simplify clock handling

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: <CAMZ6RqK32+i69Rd3qDDNm4cgQa3+m3ikZTbpt5jCGrkNt76+vw@mail.gmail.com>
Date: Thu, 6 Jun 2024 20:04:58 +0900
From: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Marc Kleine-Budde <mkl@...gutronix.de>, Wolfram Sang <wsa+renesas@...g-engineering.com>, 
	linux-can@...r.kernel.org, linux-renesas-soc@...r.kernel.org, 
	netdev@...r.kernel.org
Subject: Re: [PATCH 1/3] can: rcar_canfd: Simplify clock handling

On Thu. 6 June 2024 at 19:15, Geert Uytterhoeven <geert@...ux-m68k.org> wrote:
> Hi Vincent,
>
> On Sun, Jun 2, 2024 at 10:03 AM Vincent MAILHOL
> <mailhol.vincent@...adoo.fr> wrote:
> > On Wed. 29 May 2024 at 18:12, Geert Uytterhoeven
> > <geert+renesas@...der.be> wrote:
> > > The main CAN clock is either the internal CANFD clock, or the external
> > > CAN clock.  Hence replace the two-valued enum by a simple boolean flag.
> > > Consolidate all CANFD clock handling inside a single branch.
> >
> > For what it is worth, your patch also saves up to 8 bytes in struct
> > rcar_canfd_global (depends on the architecture).
>
> True.
>
> > > @@ -545,8 +539,8 @@ struct rcar_canfd_global {
> > >         struct platform_device *pdev;   /* Respective platform device */
> > >         struct clk *clkp;               /* Peripheral clock */
> > >         struct clk *can_clk;            /* fCAN clock */
> > > -       enum rcar_canfd_fcanclk fcan;   /* CANFD or Ext clock */
> > >         unsigned long channels_mask;    /* Enabled channels mask */
> > > +       bool extclk;                    /* CANFD or Ext clock */
> > >         bool fdmode;                    /* CAN FD or Classical CAN only mode */
> >
> > Notwithstanding comment: you may consider to replace those two booleans by a:
> >
> >           unsigned int flags;
> >
> > This way, no more fields would be needed in the future if more quirks are added.
>
> Using "unsigned int flags" and BIT(x) flags would increase code size
> by 8 bytes (arm/arm64).

I am not sure where you derive your figure from, but looking at the pahole:

  $ pahole drivers/net/can/rcar/rcar_canfd.o -C rcar_canfd_global
  struct rcar_canfd_global {
      struct rcar_canfd_channel * ch[8];               /*     0    64 */
      /* --- cacheline 1 boundary (64 bytes) --- */
      void *                     base;                 /*    64     8 */
      struct platform_device *   pdev;                 /*    72     8 */
      struct clk *               clkp;                 /*    80     8 */
      struct clk *               can_clk;              /*    88     8 */
      long unsigned int          channels_mask;        /*    96     8 */
      bool                       extclk;               /*   104     1 */
      bool                       fdmode;               /*   105     1 */

      /* XXX 6 bytes hole, try to pack */

      struct reset_control *     rstc1;                /*   112     8 */
      struct reset_control *     rstc2;                /*   120     8 */
      /* --- cacheline 2 boundary (128 bytes) --- */
      const struct rcar_canfd_hw_info  * info;         /*   128     8 */

      /* size: 136, cachelines: 3, members: 11 */
      /* sum members: 130, holes: 1, sum holes: 6 */
      /* last cacheline: 8 bytes */
  };

on a 64 bits architecture, you are currently using two booleans (two
bytes), followed by a hole of six bytes, which is a total of eight
bytes. This should be way enough to fit an unsigned int. Same would go
on 32 bits architecture in which you would use two bytes for the
booleans and have a hole of two bytes, which is four bytes: one more
time enough for an unsigned int.

In both scenarios, you are not consuming more bytes, nor are you
saving bytes. It is a neutral change.

Of course, the pahole above was done on x86_64, but as far as I know,
arm and arm64 paddings behave similarly.

> Using "unsigned int foo:1" bitfields would increase code size by 16
> (arm) or 12 (arm64) bytes.
> So as long as we can fit more bools inside the hole, it is more
> efficient to do so...

I do not get this either. Where did you get your 16 bytes from? If I do:

  struct foo {
          unsigned int foo1:1;
          unsigned int foo2:1;
  };

and

  struct bar {
          unsigned int flags;
  };

then I am pretty sure that sizeof(struct foo) is the same as
sizeof(struct bar). That's the point of the bitfields: as long as the
total of the bitfields fit in the type, the total size consumed by the
bitfield is the same as the type size.

But just to reiter my previous message, these are notwithstanding
comments. I am fine if you keep the patch as-is ;)

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>