Hello Andy, On Tue, Feb 11, 2025 at 04:35:34PM +0200, Andy Shevchenko wrote: > On Tue, Feb 11, 2025 at 04:31:45PM +0200, Andy Shevchenko wrote: > > On Tue, Feb 11, 2025 at 04:29:33PM +0200, Andy Shevchenko wrote: > > > On Tue, Feb 11, 2025 at 04:20:50PM +0200, Andy Shevchenko wrote: > > > > On Tue, Feb 11, 2025 at 01:00:08PM +0000, Mark Brown wrote: > > > > > On Mon, Feb 10, 2025 at 10:33:31PM +0200, Andy Shevchenko wrote: > > > > > > On Mon, Feb 10, 2025 at 05:48:00PM +0000, Mark Brown wrote: > > > > > > > On Mon, Feb 10, 2025 at 11:11:23AM -0600, David Lechner wrote: > > > > > > > > > > > > > In this case, we specifically split up the headers so that the only time you > > > > > > > > would ever include this header is if you need to call functions in this > > > > > > > > namespace (i.e. struct definitions are in linux/spi/offload/types.h which > > > > > > > > doesn't import the namespace). So this doesn't actually seem like a problem > > > > > > > > to me. > > > > > > > > > > > > Indeed - I can't see any case where a user would need the header without > > > > > > > needing the namespace. > > > > > > > > > > > You are looking from the other end. What I'm telling is that anyone who adds > > > > > > a header, automatically gets a namespace. What's the point to have namespace > > > > > > if it won't easily prevent from (ab)using it in the code. I consider putting > > > > > > MODULE_IMPORT_NS() in the headers a bit weird. There was a similar discussion some time ago about the lpss pwm driver (https://lore.kernel.org/linux-pwm/Z09YJGifvpENYNPy@xxxxxxxxxxxxxxxxxx/). The arguments that you didn't accept back then already are similar to the ones that were brought forward here. The TL;DR; is: Adding MODULE_IMPORT_NS() to a header makes it easier for code to use the exported symbols. Yes, that includes abusers of the code. But if you mostly care about the regular users of an API/ABI, making things easy for those is the thing that matters. Agreed, if you think that module namespaces are primarily a line of defence against abusers, adding the import to the header weakens that defence (a bit). However a typical header includes function prototypes and macros. Those also make it easier for abusers. With your argumentation we better don't create headers at all? There are other benefits of module namespaces like reducing the set of globally available symbols which speeds up module loading or the ability to see in the module meta data that a namespace is used. > > > > > Sure, but there's no case where anyone should ever be adding the header > > > > > without adding the namespace which does rather sound like the sort of > > > > > thing where you should just move the namespace addition to the header. > > > > > > > > $ git grep -lw MODULE_IMPORT_NS | wc -l > > > > 651 > > > > > > > > $ git grep -lw MODULE_IMPORT_NS | grep '\.h$' > > > > > > > > drivers/base/firmware_loader/sysfs.h > > > > drivers/iio/adc/ltc2497.h > > > > drivers/pwm/pwm-dwc.h > > > > ^^^ These ones are probably fine as they are not in include/ > > > > > > > > include/kunit/visibility.h > > > > include/linux/module.h > > > > include/linux/pwm.h > > > > > > > > I believe these three are misuses of MODULE_IMPORT_NS(). Because one may add > > > > > > _Two_, of course, module.h provides the macro :-) > > > > And after looking into include/kunit/visibility.h it becomes only a single one. > > So, PWM is abuser of MODULE_IMPORT_NS() and this series added one more. > > > > > a header just as a "proxy" one (copy'n'paste, for example) and we know that is > > > > real as we saw a lot of code that has semi-random header inclusion blocks. > > And thinking of more realistic example when we want header and do *not* want a > namespace is the simple use of the macro / or data type from it without > actually relying on the APIs. The problem of your more realistic example is that it doesn't apply here. A user of include/linux/pwm.h (or the header under discussion here) won't only use a macro or two and so not benefit from the imported module namespace. Nobody intends to import all possible namespaces in <linux/kernel.h>. > So, in case of the header structure like > > foo_constants.h > foo_types.h > foo_api.h > foo_uplevel_something.h > > The MODULE_IMPORT_NS() would make sense only to foo_api.h. And I still would > question that. As I explained that header may simply become a stale one or > being used by a mistake. I have no problem here. If the header becomes stale we will most probably notice that eventually and remove it. Maybe the unused namespace even makes it easier to spot that issue. See https://lore.kernel.org/r/20250123103939.357160-2-u.kleine-koenig@xxxxxxxxxxxx for an example which I found exactly like that. Best regards Uwe
Attachment:
signature.asc
Description: PGP signature
