Re: [PATCH 11/18] platform/x86: int3472: fix object shared between several modules

[Masahiro Yamada]

On Mon, Nov 21, 2022 at 5:12 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 11/21/22 00:45, Masahiro Yamada wrote:
> > On Mon, Nov 21, 2022 at 5:55 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> Hi,
> >>
> >> On 11/20/22 14:55, Andy Shevchenko wrote:
> >>> On Sat, Nov 19, 2022 at 11:08:17PM +0000, Alexander Lobakin wrote:
> >>>> common.o is linked to both intel_skl_int3472_{discrete,tps68470}:
> >>>>
> >>>>> scripts/Makefile.build:252: ./drivers/platform/x86/intel/int3472/Makefile:
> >>>>> common.o is added to multiple modules: intel_skl_int3472_discrete
> >>>>> intel_skl_int3472_tps68470
> >>>>
> >>>> Although both drivers share one Kconfig option
> >>>> (CONFIG_INTEL_SKL_INT3472), it's better to not link one object file
> >>>> into several modules (and/or vmlinux).
> >>>> Under certain circumstances, such can lead to the situation fixed by
> >>>> commit 637a642f5ca5 ("zstd: Fixing mixed module-builtin objects").
> >>>>
> >>>> Introduce the new module, intel_skl_int3472_common, to provide the
> >>>> functions from common.o to both discrete and tps68470 drivers. This
> >>>> adds only 3 exports and doesn't provide any changes to the actual
> >>>> code.
> >>
> >> Replying to Andy's reply here since I never saw the original submission
> >> which was not Cc-ed to platform-driver-x86@xxxxxxxxxxxxxxx .
> >>
> >> As you mention already in the commit msg, the issue from:
> >>
> >> commit 637a642f5ca5 ("zstd: Fixing mixed module-builtin objects")
> >>
> >> is not an issue here since both modules sharing the .o file are
> >> behind the same Kconfig option.
> >>
> >> So there is not really an issue here and common.o is tiny, so
> >> small chances are it does not ever increase the .ko size
> >> when looking a the .ko size rounded up to a multiple of
> >> the filesystem size.
> >>
> >> At the same time adding an extra module does come with significant
> >> costs, it will eat up at least 1 possibly more then 1 fs blocks
> >> (I don't know what the module header size overhead is).
> >>
> >> And it needs to be loaded separately and module loading is slow;
> >> and it will grow the /lib/modules/<kver>/modules.* sizes.
> >>
> >> So nack from me for this patch, since I really don't see
> >> it adding any value.
> >
> >
> >
> >
> > This does have a value.
> >
> > This clarifies the ownership of the common.o,
> > in other words, makes KBUILD_MODNAME deterministic.
> >
> >
> > If an object belongs to a module,
> > KBUILD_MODNAME is defined as the module name.
> >
> > If an object is always built-in,
> > KBUILD_MODNAME is defined as the basename of the object.
> >
> >
> >
> > Here is a question:
> > if common.o is shared by two modules intel_skl_int3472_discrete
> > and intel_skl_int3472_tps68470, what should KBUILD_MODNAME be?
> >
> >
> > I see some patch submissions relying on the assumption that
> > KBUILD_MODNAME is unique.
> > We cannot determine KBUILD_MODNAME correctly if an object is shared
> > by multiple modules.
> >
> >
> >
> >
> >
> >
> > BTW, this patch is not the way I suggested.
> > The Suggested-by should not have been there
> > (or at least Reported-by)
> >
> >
> > You argued "common.o is tiny", so I would vote for
> > making them inline functions, like
> >
> >
> > https://lore.kernel.org/linux-kbuild/20221119225650.1044591-2-alobakin@xxxxx/T/#u
>
> Yes just moving the contents of common.c to static inline helpers in common.h
> would be much better.
>
> If someone creates such a patch, please do not forget to Cc
> platform-driver-x86@xxxxxxxxxxxxxxx



I think this patch series should be split
and sent to each sub-system instead of kbuild.






> Regards,
>
> Hans
>
>
>
> >
> >
> >
> >
> >
> >
> >
> >
> >> Regards,
> >>
> >> Hans
> >>
> >>
> >>
> >>
> >>
> >>>
> >>> ...
> >>>
> >>>> +MODULE_IMPORT_NS(INTEL_SKL_INT3472);
> >>>> +
> >>>
> >>> Redundant blank line. You may put it to be last MODULE_*() in the file, if you
> >>> think it would be more visible.
> >>>
> >>>>  MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Discrete Device Driver");
> >>>>  MODULE_AUTHOR("Daniel Scally <djrscally@xxxxxxxxx>");
> >>>>  MODULE_LICENSE("GPL v2");
> >>>
> >>> ...
> >>>
> >>>> +MODULE_IMPORT_NS(INTEL_SKL_INT3472);
> >>>> +
> >>>>  MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI TPS68470 Device Driver");
> >>>>  MODULE_AUTHOR("Daniel Scally <djrscally@xxxxxxxxx>");
> >>>>  MODULE_LICENSE("GPL v2");
> >>>
> >>> Ditto. And the same to all your patches.
> >>>
> >>
> >
> >
>


-- 
Best Regards
Masahiro Yamada