| Message ID | 20200802225940.51457-1-sjg@chromium.org |
|---|---|
| Headers | show |
| Series | common: Drop remaining includes in common.h | expand |
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
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.
Hi Tom, (adding Heinrich as he mentioned this series on irc) On Wed, 19 Aug 2020 at 07:09, Tom Rini <trini@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