| Message ID | 20221119225650.1044591-12-alobakin@pm.me (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | treewide: fix object files shared between several modules | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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. ... > +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@gmail.com>"); > MODULE_LICENSE("GPL v2"); ... > +MODULE_IMPORT_NS(INTEL_SKL_INT3472); > + > MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI TPS68470 Device Driver"); > MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); > MODULE_LICENSE("GPL v2"); Ditto. And the same to all your patches.
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@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. 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@gmail.com>"); >> MODULE_LICENSE("GPL v2"); > > ... > >> +MODULE_IMPORT_NS(INTEL_SKL_INT3472); >> + >> MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI TPS68470 Device Driver"); >> MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); >> MODULE_LICENSE("GPL v2"); > > Ditto. And the same to all your patches. >
On Mon, Nov 21, 2022 at 5:55 AM Hans de Goede <hdegoede@redhat.com> 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@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 > 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@gmail.com>"); > >> MODULE_LICENSE("GPL v2"); > > > > ... > > > >> +MODULE_IMPORT_NS(INTEL_SKL_INT3472); > >> + > >> MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI TPS68470 Device Driver"); > >> MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); > >> MODULE_LICENSE("GPL v2"); > > > > Ditto. And the same to all your patches. > > >
Hi, On 11/21/22 00:45, Masahiro Yamada wrote: > On Mon, Nov 21, 2022 at 5:55 AM Hans de Goede <hdegoede@redhat.com> 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@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 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 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@gmail.com>"); >>>> MODULE_LICENSE("GPL v2"); >>> >>> ... >>> >>>> +MODULE_IMPORT_NS(INTEL_SKL_INT3472); >>>> + >>>> MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI TPS68470 Device Driver"); >>>> MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); >>>> MODULE_LICENSE("GPL v2"); >>> >>> Ditto. And the same to all your patches. >>> >> > >
On Mon, Nov 21, 2022 at 5:12 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 11/21/22 00:45, Masahiro Yamada wrote: > > On Mon, Nov 21, 2022 at 5:55 AM Hans de Goede <hdegoede@redhat.com> 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@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 > > 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. > 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@gmail.com>"); > >>>> MODULE_LICENSE("GPL v2"); > >>> > >>> ... > >>> > >>>> +MODULE_IMPORT_NS(INTEL_SKL_INT3472); > >>>> + > >>>> MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI TPS68470 Device Driver"); > >>>> MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); > >>>> MODULE_LICENSE("GPL v2"); > >>> > >>> Ditto. And the same to all your patches. > >>> > >> > > > > >
Hi, On 11/21/22 10:06, Masahiro Yamada wrote: > On Mon, Nov 21, 2022 at 5:12 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 11/21/22 00:45, Masahiro Yamada wrote: >>> On Mon, Nov 21, 2022 at 5:55 AM Hans de Goede <hdegoede@redhat.com> 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@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 >> >> 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 >>>>> ... >>>>> >>>>>> +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@gmail.com>"); >>>>>> MODULE_LICENSE("GPL v2"); >>>>> >>>>> ... >>>>> >>>>>> +MODULE_IMPORT_NS(INTEL_SKL_INT3472); >>>>>> + >>>>>> MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI TPS68470 Device Driver"); >>>>>> MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); >>>>>> MODULE_LICENSE("GPL v2"); >>>>> >>>>> Ditto. And the same to all your patches. >>>>> >>>> >>> >>> >> > >
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Date: Sun, 20 Nov 2022 15:55:21 +0200 > 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. > > ... > > > +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. My intention was that it's not "standard" module info like license or description, rather something like exports or initcalls, that's why I did separate them. But I haven't been using module namespaces a lot previously, so if it should be in one block with the rest MODULE_*(), sure, I'll fix. > > > MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Discrete Device Driver"); > > MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); > > MODULE_LICENSE("GPL v2"); > > ... > > > +MODULE_IMPORT_NS(INTEL_SKL_INT3472); > > + > > MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI TPS68470 Device Driver"); > > MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); > > MODULE_LICENSE("GPL v2"); > > Ditto. And the same to all your patches. > > -- > With Best Regards, > Andy Shevchenko Thanks, Olek
From: Hans de Goede <hdegoede@redhat.com> Date: Mon, 21 Nov 2022 09:12:41 +0100 > Hi, > > On 11/21/22 00:45, Masahiro Yamada wrote: > > On Mon, Nov 21, 2022 at 5:55 AM Hans de Goede <hdegoede@redhat.com> wrote: [...] > >> 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. +1 > > > > > > > > > > > > > > BTW, this patch is not the way I suggested. > > The Suggested-by should not have been there > > (or at least Reported-by) (to Masahiro) Ah, you're right, sorry. Will replace all the tags to Reported-by, that would look more correct. > > > > > > 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 > > Yes just moving the contents of common.c to static inline helpers in common.h > would be much better. Sure, why not. There probably are more of shared object files which would feel better being static inlines in the series, will see. > > If someone creates such a patch, please do not forget to Cc > platform-driver-x86@vger.kernel.org Got it, sure. get_maintainer.pl dropped a wall on me, so I was picking stuff manually from it and missed that one. > > Regards, > > Hans [...] > >>> Ditto. And the same to all your patches. Thanks, Olek
From: Hans de Goede <hdegoede@redhat.com> Date: Mon, 21 Nov 2022 10:34:11 +0100 > Hi, > > On 11/21/22 10:06, Masahiro Yamada wrote: > > On Mon, Nov 21, 2022 at 5:12 PM Hans de Goede <hdegoede@redhat.com> wrote: [...] > > 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. Makes sense! I'll collect the feedback here and then will be sending stuff to the corresponding MLs with the reference to the original commits which introduce Kbuild warnings. > > Regards, > > Hans [...] Thanks! Olek
diff --git a/drivers/platform/x86/intel/int3472/Makefile b/drivers/platform/x86/intel/int3472/Makefile index cfec7784c5c9..53cc0e7db749 100644 --- a/drivers/platform/x86/intel/int3472/Makefile +++ b/drivers/platform/x86/intel/int3472/Makefile @@ -1,4 +1,6 @@ -obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472_discrete.o \ +obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472_common.o \ + intel_skl_int3472_discrete.o \ intel_skl_int3472_tps68470.o -intel_skl_int3472_discrete-y := discrete.o clk_and_regulator.o common.o -intel_skl_int3472_tps68470-y := tps68470.o tps68470_board_data.o common.o +intel_skl_int3472_common-y := common.o +intel_skl_int3472_discrete-y := discrete.o clk_and_regulator.o +intel_skl_int3472_tps68470-y := tps68470.o tps68470_board_data.o diff --git a/drivers/platform/x86/intel/int3472/common.c b/drivers/platform/x86/intel/int3472/common.c index 9db2bb0bbba4..bd573ff46610 100644 --- a/drivers/platform/x86/intel/int3472/common.c +++ b/drivers/platform/x86/intel/int3472/common.c @@ -2,6 +2,7 @@ /* Author: Dan Scally <djrscally@gmail.com> */ #include <linux/acpi.h> +#include <linux/module.h> #include <linux/slab.h> #include "common.h" @@ -29,6 +30,7 @@ union acpi_object *skl_int3472_get_acpi_buffer(struct acpi_device *adev, char *i return obj; } +EXPORT_SYMBOL_NS_GPL(skl_int3472_get_acpi_buffer, INTEL_SKL_INT3472); int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb) { @@ -52,6 +54,7 @@ int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb) kfree(obj); return ret; } +EXPORT_SYMBOL_NS_GPL(skl_int3472_fill_cldb, INTEL_SKL_INT3472); /* sensor_adev_ret may be NULL, name_ret must not be NULL */ int skl_int3472_get_sensor_adev_and_name(struct device *dev, @@ -80,3 +83,8 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev, return ret; } +EXPORT_SYMBOL_NS_GPL(skl_int3472_get_sensor_adev_and_name, INTEL_SKL_INT3472); + +MODULE_DESCRIPTION("Intel SkyLake INT3472 Common Module"); +MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); +MODULE_LICENSE("GPL"); diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index 974a132db651..a1f3b593cea6 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -414,6 +414,8 @@ static struct platform_driver int3472_discrete = { }; module_platform_driver(int3472_discrete); +MODULE_IMPORT_NS(INTEL_SKL_INT3472); + MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Discrete Device Driver"); MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); MODULE_LICENSE("GPL v2"); diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c index 5b8d1a9620a5..3c983aa7731f 100644 --- a/drivers/platform/x86/intel/int3472/tps68470.c +++ b/drivers/platform/x86/intel/int3472/tps68470.c @@ -255,6 +255,8 @@ static struct i2c_driver int3472_tps68470 = { }; module_i2c_driver(int3472_tps68470); +MODULE_IMPORT_NS(INTEL_SKL_INT3472); + MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI TPS68470 Device Driver"); MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); MODULE_LICENSE("GPL v2");
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. Fixes: a2f9fbc247ee ("platform/x86: int3472: Split into 2 drivers") Suggested-by: Masahiro Yamada <masahiroy@kernel.org> Signed-off-by: Alexander Lobakin <alobakin@pm.me> --- drivers/platform/x86/intel/int3472/Makefile | 8 +++++--- drivers/platform/x86/intel/int3472/common.c | 8 ++++++++ drivers/platform/x86/intel/int3472/discrete.c | 2 ++ drivers/platform/x86/intel/int3472/tps68470.c | 2 ++ 4 files changed, 17 insertions(+), 3 deletions(-) -- 2.38.1