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

  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
  

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