Hi,

On 11/21/22 10:06, Masahiro Yamada wrote:

On Mon, Nov 21, 2022 at 5:12 PM Hans de Goede [off-list ref] wrote:

quoted

Hi,

On 11/21/22 00:45, Masahiro Yamada wrote:

quoted

On Mon, Nov 21, 2022 at 5:55 AM Hans de Goede [off-list ref] wrote:

quoted

Hi,

On 11/20/22 14:55, Andy Shevchenko wrote:

quoted

On Sat, Nov 19, 2022 at 11:08:17PM +0000, Alexander Lobakin wrote:

quoted

common.o is linked to both intel_skl_int3472_{discrete,tps68470}:

quoted

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@vger.kernel.org .

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@pm.me/T/#u (local)

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@vger.kernel.org

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

Yes definitely, the changes are big enough that not merging
this through the subsystem trees is going to cause conflicts.

Regards,

Hans

quoted

quoted

quoted

quoted

...

quoted

+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.

quoted

 MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Discrete Device Driver");
 MODULE_AUTHOR("Daniel Scally [off-list ref]");
 MODULE_LICENSE("GPL v2");

...

quoted

+MODULE_IMPORT_NS(INTEL_SKL_INT3472);
+
 MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI TPS68470 Device Driver");
 MODULE_AUTHOR("Daniel Scally [off-list ref]");
 MODULE_LICENSE("GPL v2");

Ditto. And the same to all your patches.