[RFCv2] common: Drop remaining includes in common.h

[Simon Glass]

Hi Tom,

(adding Heinrich as he mentioned this series on irc)

On Wed, 19 Aug 2020 at 07:09, Tom Rini <trini at konsulko.com> wrote:
>
> I've picked up Simon's v1 of this series and moved it to an RFC with
> this v2.  I don't intend for this series to go in as-is but rather since
> I spent a good bit of time iterating over the problems of trying a
> conversion (in a few places) where we only selectively add back in the
> header being removed from common.h in the case of a fail to build, I
> didn't want the work lost.
>
> What I think needs to be done moving forward is even smaller series here
> where we focus on removing one or two headers, but then only re-add them
> where required.
>
> Also note that this series shows a few funny issues.  The patch to
> remove <linux/kernel.h> and selectively re-add it shows:
>             bcm958712k     : all -4 text -4
>                u-boot: add: 1/0, grow: 0/-3 bytes: 24/-28 (-4)
>                  function                                   old     new   delta
>                  blk_dread                                    -      24     +24
>                  part_test_efi                              184     180      -4
>                  is_gpt_valid                               736     724     -12
>                  fs_devread                                 600     588     -12
>
> everywhere that code is used.  I don't see why, but there's some
> underlying problem exposed in the move.  I believe it's also that patch

After an hour's ceaseless searching I narrowed this down to 'inline'.
In linux/compiler.h it  defines it to __gnu_inline
__inline_maybe_unused, etc which changes the inlining behaviour of the
compiler.

I should have guessed this from what you said below.

> which shows, for every big-endian platform something like:
>             T4160RDB       : all -592 text -592
>                u-boot: add: 2/0, grow: 1/-14 bytes: 68/-656 (-588)
>                  function                                   old     new   delta
>                  ___arch__swab32                              -      48     +48
>                  blk_dread                                    -      12     +12
>                  ext4fs_bg_get_inode_table_id                76      84      +8
>                  ehci_td_buffer                             164     156      -8
>                  _ehci_destroy_int_queue                    244     236      -8
>                  static.ehci_update_endpt2_dev_n_port       116     104     -12
>                  ext4fs_open                                200     188     -12
>                  _ehci_poll_int_queue                       236     224     -12
>                  usb_lowlevel_init                          916     892     -24
>                  ext4fs_mount                               332     304     -28
>                  fs_devread                                 572     540     -32
>                  ext4fs_find_file1                          756     724     -32
>                  ext4fs_iterate_dir                         848     812     -36
>                  ext4fs_read_inode                          520     452     -68
>                  static._ehci_create_int_queue             1000     908     -92
>                  ehci_submit_async                         1632    1520    -112
>                  read_allocated_block                      2532    2352    -180
>
> and I lack hardware to see (and it looks like qemu-ppce500 can't be
> given a disk atm) if the problem is that ext2/4 is broken before and
> fixed now, or fixed now and broken with this patch, as that's my first
> concern on seeing ___arch__swab32 show up.  But maybe it's a harmless
> "no, don't inline ..." decision the compiler is now able to make.  But
> very non-obvious and needing a run-time sanity check to be sure.
>

I suppose this is the same.

Basically we need to include <compiler.h> whenever inline is used. I
should have guessed that. It is annoying that 'inline' is defined to
something else when that header is included. This is another gotcha.

It would be great to fix up this series and get it applied before the
new merge window makes everything harder. I don't think we have a lot
of unnecessary header inclusions, and here we have just found more we
need to add.

Regards,
Simon