A one character difference in the name supplied to MODULE_DEVICE_TABLE
breaks a future patch set, so fix the typo.
Cc: [email protected]
Fixes: f70ed3b5dc8b ("mailbox: rockchip: Add Rockchip mailbox driver")
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Allen Webb <[email protected]>
---
drivers/mailbox/rockchip-mailbox.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mailbox/rockchip-mailbox.c b/drivers/mailbox/rockchip-mailbox.c
index 979acc810f30..ca50f7f176f6 100644
--- a/drivers/mailbox/rockchip-mailbox.c
+++ b/drivers/mailbox/rockchip-mailbox.c
@@ -159,7 +159,7 @@ static const struct of_device_id rockchip_mbox_of_match[] = {
{ .compatible = "rockchip,rk3368-mailbox", .data = &rk3368_drv_data},
{ },
};
-MODULE_DEVICE_TABLE(of, rockchp_mbox_of_match);
+MODULE_DEVICE_TABLE(of, rockchip_mbox_of_match);
static int rockchip_mbox_probe(struct platform_device *pdev)
{
--
2.37.3
On Mon, Dec 19, 2022 at 02:46:10PM -0600, Allen Webb wrote:
> A one character difference in the name supplied to MODULE_DEVICE_TABLE
> breaks a future patch set, so fix the typo.
>
> Cc: [email protected]
> Fixes: f70ed3b5dc8b ("mailbox: rockchip: Add Rockchip mailbox driver")
How has this been an issue since the 4.6 kernel and no one has noticed
it? Can this code not be built as a module? If not, then please
explain this.
thanks,
greg k-h
On Tue, Dec 20, 2022 at 12:46 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Mon, Dec 19, 2022 at 02:46:10PM -0600, Allen Webb wrote:
> > A one character difference in the name supplied to MODULE_DEVICE_TABLE
> > breaks a future patch set, so fix the typo.
> >
> > Cc: [email protected]
> > Fixes: f70ed3b5dc8b ("mailbox: rockchip: Add Rockchip mailbox driver")
>
> How has this been an issue since the 4.6 kernel and no one has noticed
> it? Can this code not be built as a module? If not, then please
> explain this.
As mentioned in a different sub-thread this cannot be built as a
module so I updated the commit message to:
imx: Fix typo
A one character difference in the name supplied to MODULE_DEVICE_TABLE
breaks compilation for SOC_IMX8M after built-in modules can generate
match-id based module aliases, so fix the typo.
This was not caught earlier because SOC_IMX8M can not be built as a
module and MODULE_DEVICE_TABLE is a no-op for built-in modules.
Fixes: 556f5cf9568a ("soc: imx: add i.MX8MP HSIO blk-ctrl")
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Allen Webb <[email protected]>
>
> thanks,
>
> greg k-h
On Tue, Dec 20, 2022 at 08:58:36AM -0600, Allen Webb wrote:
> As mentioned in a different sub-thread this cannot be built as a
> module so I updated the commit message to:
>
> imx: Fix typo
>
> A one character difference in the name supplied to MODULE_DEVICE_TABLE
> breaks compilation for SOC_IMX8M after built-in modules can generate
> match-id based module aliases, so fix the typo.
Are you saying that it is a typo *now* only, and fixing it does not fix
compilation now, but that fixing the typo will fix a future compilation
issue once your patches get merged for built-in module aliases?
> This was not caught earlier because SOC_IMX8M can not be built as a
> module and MODULE_DEVICE_TABLE is a no-op for built-in modules.
Odd, so why did it use MODULE_DEVICE_TABLE then? What was the reason for
the driver having MODULE_DEVICE_TABLE if it was a no-op?
Luis
On Tue, Dec 20, 2022 at 12:19:49PM -0600, Allen Webb wrote:
> On Tue, Dec 20, 2022 at 12:12 PM Luis Chamberlain <[email protected]> wrote:
> >
> > On Tue, Dec 20, 2022 at 08:58:36AM -0600, Allen Webb wrote:
> > > As mentioned in a different sub-thread this cannot be built as a
> > > module so I updated the commit message to:
> > >
> > > imx: Fix typo
> > >
> > > A one character difference in the name supplied to MODULE_DEVICE_TABLE
> > > breaks compilation for SOC_IMX8M after built-in modules can generate
> > > match-id based module aliases, so fix the typo.
> >
> > Are you saying that it is a typo *now* only, and fixing it does not fix
> > compilation now, but that fixing the typo will fix a future compilation
> > issue once your patches get merged for built-in module aliases?
>
> It was always a typo, it just doesn't affect the build because
> MODULE_DEVICE_TABLE is not doing anything for built-in modules before
> this patch series.
>
> >
> > > This was not caught earlier because SOC_IMX8M can not be built as a
> > > module and MODULE_DEVICE_TABLE is a no-op for built-in modules.
> >
> > Odd, so why did it use MODULE_DEVICE_TABLE then? What was the reason for
> > the driver having MODULE_DEVICE_TABLE if it was a no-op?
>
> That is a good question. I can only speculate as to the answer
You can use git blame to trace back to the original commit that added
it, then use git blame foo.c commit-id~1 on the file to keep going
back until you get to the first commit that added that entry, check out
that as a branch and see if the driver was still not a module then
(tristate). If so then your speculation is very likely accurate and
can be spelled out in the commit log message.
It begs the inverse question too though, you are finding uses of
built-in-always code (never tristate) which uses MODULE_DEVICE_TABLE().
Although today that's a no-op, after your changes this becomes useful
information, so do you need to scrape to see what built-in-aways code
*do* not use MODULE_DEVICE_TABLE() where after your patches it would
have become useful?
Determing if there is value to that endeavour should be easily grasped by
reading the description you are adding to MODULE_DEVICE_TABLE() --
in your patch 5 "module.h: MODULE_DEVICE_TABLE for built-in modules".
Driver developers for built-in-always code should read that description
and be able to decide if they should use it or not. But even your latest
replies to Greg do not make that clear, *I personally gather* rather that
this would in no way shape or form be useful. But is that true?
So why not just remove MODULE_DEVICE_TABLE() from code we know is
built-in-always code instead of fixing a typo just to fix a future
compile issue?
Then your commit log is not about "fix typo", but rather remove a no-op
macro as the driver is always built-in and keeping that macro would not
help built-in code.
> but it
> is plausible people copied a common pattern and since no breakage was
> noticed left it as is.
This level of clarity is important to spell out in the commit log
message, specially if you are suggesting it is just a typo fix. Because
I will take it for granted that it is just that.
If it fixes a future use case where the typo would be more of an issue,
you can mention that in a secondary paragraph or sentence.
> It also raises the question how many modules have device tables, but
> do not call MODULE_DEVICE_TABLE since they are only ever built-in.
> Maybe there should be some build time enforcement mechanism to make
> sure that these are consistent.
Definitely, Nick Alcock is doing some related work where the semantics
of built-in modules needs to be clearer, he for instance is now removing
a few MODULE_() macros for things which *are never* modules, and this is
because after commit 8b41fc4454e ("kbuild: create modules.builtin
without Makefile.modbuiltin or tristate.conf") we rely on the module
license tag to generate the modules.builtin file. Without that commit
we end up traversing the source tree twice. Nick's work builds on
that work and futher clarifies these semantics by adding tooling which
complains when something which is *never* capable of being a module
uses module macros. The macro you are extending, MODULE_DEVICE_TABLE(),
today is a no-op for built-in, but you are adding support to extend it
for built-in stuff. Nick's work will help with clarifying symbol locality
and so he may be interested in your association for the data in
MODULE_DEVICE_TABLE and how you associate to a respective would-be
module. His work is useful for making tracing more accurate with respect
to symbol associations, so the data in MODULE_DEVICE_TABLE() may be
useful as well to him.
You folks may want to Cc each other on your patches.
If we know for certain things *will never* be used or *should not be
used*, as in the case of the module license tag, we should certainly
have tooling to pick up on that crap to help us tidy it up.
If you determine MODULE_DEVICE_TABLE() *should* not be used for built-in
always code (never tristate) then you and Nick likely have overlap of
macros to tidy up and tooling to share to spot these sort of issues.
Luis
On Tue, Dec 20, 2022 at 12:12 PM Luis Chamberlain <[email protected]> wrote:
>
> On Tue, Dec 20, 2022 at 08:58:36AM -0600, Allen Webb wrote:
> > As mentioned in a different sub-thread this cannot be built as a
> > module so I updated the commit message to:
> >
> > imx: Fix typo
> >
> > A one character difference in the name supplied to MODULE_DEVICE_TABLE
> > breaks compilation for SOC_IMX8M after built-in modules can generate
> > match-id based module aliases, so fix the typo.
>
> Are you saying that it is a typo *now* only, and fixing it does not fix
> compilation now, but that fixing the typo will fix a future compilation
> issue once your patches get merged for built-in module aliases?
It was always a typo, it just doesn't affect the build because
MODULE_DEVICE_TABLE is not doing anything for built-in modules before
this patch series.
>
> > This was not caught earlier because SOC_IMX8M can not be built as a
> > module and MODULE_DEVICE_TABLE is a no-op for built-in modules.
>
> Odd, so why did it use MODULE_DEVICE_TABLE then? What was the reason for
> the driver having MODULE_DEVICE_TABLE if it was a no-op?
That is a good question. I can only speculate as to the answer but it
is plausible people copied a common pattern and since no breakage was
noticed left it as is.
It also raises the question how many modules have device tables, but
do not call MODULE_DEVICE_TABLE since they are only ever built-in.
Maybe there should be some build time enforcement mechanism to make
sure that these are consistent.
>
> Luis
On Tue, Dec 20, 2022 at 12:47 PM Luis Chamberlain <[email protected]> wrote:
>
> On Tue, Dec 20, 2022 at 12:19:49PM -0600, Allen Webb wrote:
> > On Tue, Dec 20, 2022 at 12:12 PM Luis Chamberlain <[email protected]> wrote:
> > >
> > > On Tue, Dec 20, 2022 at 08:58:36AM -0600, Allen Webb wrote:
> > > > As mentioned in a different sub-thread this cannot be built as a
> > > > module so I updated the commit message to:
> > > >
> > > > imx: Fix typo
> > > >
> > > > A one character difference in the name supplied to MODULE_DEVICE_TABLE
> > > > breaks compilation for SOC_IMX8M after built-in modules can generate
> > > > match-id based module aliases, so fix the typo.
> > >
> > > Are you saying that it is a typo *now* only, and fixing it does not fix
> > > compilation now, but that fixing the typo will fix a future compilation
> > > issue once your patches get merged for built-in module aliases?
> >
> > It was always a typo, it just doesn't affect the build because
> > MODULE_DEVICE_TABLE is not doing anything for built-in modules before
> > this patch series.
> >
> > >
> > > > This was not caught earlier because SOC_IMX8M can not be built as a
> > > > module and MODULE_DEVICE_TABLE is a no-op for built-in modules.
> > >
> > > Odd, so why did it use MODULE_DEVICE_TABLE then? What was the reason for
> > > the driver having MODULE_DEVICE_TABLE if it was a no-op?
> >
> > That is a good question. I can only speculate as to the answer
>
> You can use git blame to trace back to the original commit that added
> it, then use git blame foo.c commit-id~1 on the file to keep going
> back until you get to the first commit that added that entry, check out
> that as a branch and see if the driver was still not a module then
> (tristate). If so then your speculation is very likely accurate and
> can be spelled out in the commit log message.
All three cases are bool configs.
>
> It begs the inverse question too though, you are finding uses of
> built-in-always code (never tristate) which uses MODULE_DEVICE_TABLE().
> Although today that's a no-op, after your changes this becomes useful
> information, so do you need to scrape to see what built-in-aways code
> *do* not use MODULE_DEVICE_TABLE() where after your patches it would
> have become useful?
>
> Determing if there is value to that endeavour should be easily grasped by
> reading the description you are adding to MODULE_DEVICE_TABLE() --
> in your patch 5 "module.h: MODULE_DEVICE_TABLE for built-in modules".
> Driver developers for built-in-always code should read that description
> and be able to decide if they should use it or not. But even your latest
> replies to Greg do not make that clear, *I personally gather* rather that
> this would in no way shape or form be useful. But is that true?
I took another stab at clarifying (and also dropped the ifdev since
the same macro works both for separate and built-in modules:
/*
* Creates an alias so file2alias.c can find device table.
*
* Use this in cases where a device table is used to match devices because it
* surfaces match-id based module aliases to userspace for:
* - Automatic module loading.
* - Tools like USBGuard which allow or block devices based on policy such as
* which modules match a device.
*
* The module name is included in the alias for two reasons:
* - It avoids creating two aliases with the same name for built-in modules.
* Historically MODULE_DEVICE_TABLE was a no-op for built-in modules, so
* there was nothing to stop different modules from having the same device
* table name and consequently the same alias when building as a module.
* - The module name is needed by files2alias.c to associate a particular
* device table with its associated module for built-in modules since
* files2alias would otherwise see the module name as `vmlinuz.o`.
*/
#define MODULE_DEVICE_TABLE(type, name) \
extern void *CONCATENATE( \
CONCATENATE(__mod_##type##__##name##__, \
__KBUILD_MODNAME), \
_device_table) \
__attribute__ ((unused, alias(__stringify(name))))
>
> So why not just remove MODULE_DEVICE_TABLE() from code we know is
> built-in-always code instead of fixing a typo just to fix a future
> compile issue?
>
> Then your commit log is not about "fix typo", but rather remove a no-op
> macro as the driver is always built-in and keeping that macro would not
> help built-in code.
The deciding factor in whether it makes sense to remove these vs fix
them seems to be, "How complete do we want modules.builtin.alias to
be?"
Arguably we should just drop these in cases where there isn't an
"authorized" sysfs attribute but following that logic there is not any
reason to generate built-in aliases for anything except USB and
thunderbolt.
On the flip side, if we are going to the effort to make this a generic
solution that covers everything, the built-in aliases are only as
useful as they are complete, so we would want everything that defines
a device table to call the macro correctly.
>
> > but it
> > is plausible people copied a common pattern and since no breakage was
> > noticed left it as is.
>
> This level of clarity is important to spell out in the commit log
> message, specially if you are suggesting it is just a typo fix. Because
> I will take it for granted that it is just that.
>
> If it fixes a future use case where the typo would be more of an issue,
> you can mention that in a secondary paragraph or sentence.
>
> > It also raises the question how many modules have device tables, but
> > do not call MODULE_DEVICE_TABLE since they are only ever built-in.
> > Maybe there should be some build time enforcement mechanism to make
> > sure that these are consistent.
>
> Definitely, Nick Alcock is doing some related work where the semantics
> of built-in modules needs to be clearer, he for instance is now removing
> a few MODULE_() macros for things which *are never* modules, and this is
> because after commit 8b41fc4454e ("kbuild: create modules.builtin
> without Makefile.modbuiltin or tristate.conf") we rely on the module
> license tag to generate the modules.builtin file. Without that commit
> we end up traversing the source tree twice. Nick's work builds on
> that work and futher clarifies these semantics by adding tooling which
> complains when something which is *never* capable of being a module
> uses module macros. The macro you are extending, MODULE_DEVICE_TABLE(),
> today is a no-op for built-in, but you are adding support to extend it
> for built-in stuff. Nick's work will help with clarifying symbol locality
> and so he may be interested in your association for the data in
> MODULE_DEVICE_TABLE and how you associate to a respective would-be
> module. His work is useful for making tracing more accurate with respect
> to symbol associations, so the data in MODULE_DEVICE_TABLE() may be
> useful as well to him.
Thanks, I will look through what I can find.
>
> You folks may want to Cc each other on your patches.
>
> If we know for certain things *will never* be used or *should not be
> used*, as in the case of the module license tag, we should certainly
> have tooling to pick up on that crap to help us tidy it up.
>
> If you determine MODULE_DEVICE_TABLE() *should* not be used for built-in
> always code (never tristate) then you and Nick likely have overlap of
> macros to tidy up and tooling to share to spot these sort of issues.
It definitely is needed for never-tristate modules that match devices
in subsystems that surface the authorized attribute.
>
> Luis
On Tue, Dec 20, 2022 at 01:49:04PM -0600, Allen Webb wrote:
> I took another stab at clarifying (and also dropped the ifdev since
> the same macro works both for separate and built-in modules:
>
> /*
> * Creates an alias so file2alias.c can find device table.
> *
> * Use this in cases where a device table is used to match devices because it
> * surfaces match-id based module aliases to userspace for:
> * - Automatic module loading.
> * - Tools like USBGuard which allow or block devices based on policy such as
> * which modules match a device.
> *
> * The module name is included in the alias for two reasons:
> * - It avoids creating two aliases with the same name for built-in modules.
> * Historically MODULE_DEVICE_TABLE was a no-op for built-in modules, so
> * there was nothing to stop different modules from having the same device
> * table name and consequently the same alias when building as a module.
> * - The module name is needed by files2alias.c to associate a particular
> * device table with its associated module for built-in modules since
> * files2alias would otherwise see the module name as `vmlinuz.o`.
> */
This is still weak in light of the questions I had. It does not make it
easy for a driver developer who is going to support only built-in only
if they need to define this or not. And it seems we're still discussing
the merits of this, so I'd wait until this is fleshed out, but I think
we are on the right track finally.
> The deciding factor in whether it makes sense to remove these vs fix
> them seems to be, "How complete do we want modules.builtin.alias to
> be?"
>
> Arguably we should just drop these in cases where there isn't an
> "authorized" sysfs attribute but following that logic there is not any
> reason to generate built-in aliases for anything except USB and
> thunderbolt.
There we go, now we have a *real* use case for this for built-in stuff
to consider. Is USBGuard effective even for built-in stuff?
Given everything discussed so far I'd like to get clarification if it
even help for built-in USB / thunderbolt. Does it? If so how? What could
userspace do with this information if the driver is already built-in?
> On the flip side, if we are going to the effort to make this a generic
> solution that covers everything, the built-in aliases are only as
> useful as they are complete, so we would want everything that defines
> a device table to call the macro correctly.
It is the ambiguity which is terrible to add. If the only use case is
for USB and Thunderbolt then we can spell it out, then only those driver
developers would care to consider it if the driver is bool. And, a
respective tooling would scrape only those drivers to verify if the
table is missing for built-in too.
> It definitely is needed for never-tristate modules that match devices
> in subsystems that surface the authorized attribute.
What is this "authorized attribute" BTW exactly? Do have some
documentation reference?
Luis
On Tue, Dec 20, 2022 at 2:03 PM Luis Chamberlain <[email protected]> wrote:
>
> On Tue, Dec 20, 2022 at 01:49:04PM -0600, Allen Webb wrote:
> > I took another stab at clarifying (and also dropped the ifdev since
> > the same macro works both for separate and built-in modules:
> >
> > /*
> > * Creates an alias so file2alias.c can find device table.
> > *
> > * Use this in cases where a device table is used to match devices because it
> > * surfaces match-id based module aliases to userspace for:
> > * - Automatic module loading.
> > * - Tools like USBGuard which allow or block devices based on policy such as
> > * which modules match a device.
> > *
> > * The module name is included in the alias for two reasons:
> > * - It avoids creating two aliases with the same name for built-in modules.
> > * Historically MODULE_DEVICE_TABLE was a no-op for built-in modules, so
> > * there was nothing to stop different modules from having the same device
> > * table name and consequently the same alias when building as a module.
> > * - The module name is needed by files2alias.c to associate a particular
> > * device table with its associated module for built-in modules since
> > * files2alias would otherwise see the module name as `vmlinuz.o`.
> > */
>
> This is still weak in light of the questions I had. It does not make it
> easy for a driver developer who is going to support only built-in only
> if they need to define this or not. And it seems we're still discussing
> the merits of this, so I'd wait until this is fleshed out, but I think
> we are on the right track finally.
>
> > The deciding factor in whether it makes sense to remove these vs fix
> > them seems to be, "How complete do we want modules.builtin.alias to
> > be?"
> >
> > Arguably we should just drop these in cases where there isn't an
> > "authorized" sysfs attribute but following that logic there is not any
> > reason to generate built-in aliases for anything except USB and
> > thunderbolt.
>
> There we go, now we have a *real* use case for this for built-in stuff
> to consider. Is USBGuard effective even for built-in stuff?
Yes, just because a module is loaded doesn't mean a specific device
has probed the driver yet.
>
> Given everything discussed so far I'd like to get clarification if it
> even help for built-in USB / thunderbolt. Does it? If so how? What could
> userspace do with this information if the driver is already built-in?
We are not trying to stop the module from being loaded (which is
always the case for built-in modules) and in fact it is possible to
have devices already using the module and still not authorize (and by
extension probe the module for) newly connected devices.
For example someone might have an unattended computer downloading
installation media to a USB drive. Presumably this computer would be
locked to make it more difficult for a bad actor to access the
computer. Since USB storage devices are not needed to interact with
the lock screen, we can use the authorized_default sysfs attribute to
not allow new USB devices to probe modules by default and have
USBGuard vet the devices. Mice, keyboards, etc can be allowed so that
the lock screen can still be used (this important in cases like
suspend+resume or docks).
>
> > On the flip side, if we are going to the effort to make this a generic
> > solution that covers everything, the built-in aliases are only as
> > useful as they are complete, so we would want everything that defines
> > a device table to call the macro correctly.
>
> It is the ambiguity which is terrible to add. If the only use case is
> for USB and Thunderbolt then we can spell it out, then only those driver
> developers would care to consider it if the driver is bool. And, a
> respective tooling would scrape only those drivers to verify if the
> table is missing for built-in too.
I was aiming to write it so that it wouldn't easily become obsolete by
later changes, so tying it to the authorized and authorized_default
sysfs attributes is probably the ideal deciding factor and listing USB
and thunderbolt as examples makes sense.
>
> > It definitely is needed for never-tristate modules that match devices
> > in subsystems that surface the authorized attribute.
>
> What is this "authorized attribute" BTW exactly? Do have some
> documentation reference?
There are sysfs attributes called authorized and authorized_default
that together can prevent devices from being fully enumerated and
probed. authorized_default gets set to 0 for the hub and any devices
connected after that will show in sysfs, but not fully enumerate or
probe until the device's authorized attribute is set to 1. There are
some edge cases like internal devices which have some extra
complexity.
As for documentation, I wasn't able to find much other than:
https://github.com/torvalds/linux/blob/v6.1/drivers/usb/core/hcd.c#L370
/* authorized_default behaviour:
* -1 is authorized for all devices except wireless (old behaviour)
* 0 is unauthorized for all devices
* 1 is authorized for all devices
* 2 is authorized for internal devices
*/
...
and
https://github.com/torvalds/linux/blob/v6.1/Documentation/admin-guide/kernel-parameters.txt#L6424
usbcore.authorized_default=
[USB] Default USB device authorization:
(default -1 = authorized except for wireless USB,
0 = not authorized, 1 = authorized, 2 = authorized
if device connected to internal port)
...
The feature looks like it was originally introduced for wireless USB in:
https://www.mail-archive.com/[email protected]/msg54289.html
and later adapted for use cases like USBGuard here:
https://github.com/torvalds/linux/commit/c4fc2342cb611f945fa468e742759e25984005ad
>
> Luis
On Tue, Dec 20, 2022 at 03:57:57PM -0600, Allen Webb wrote:
> On Tue, Dec 20, 2022 at 2:03 PM Luis Chamberlain <[email protected]> wrote:
> >
> > On Tue, Dec 20, 2022 at 01:49:04PM -0600, Allen Webb wrote:
> > > I took another stab at clarifying (and also dropped the ifdev since
> > > the same macro works both for separate and built-in modules:
> > >
> > > /*
> > > * Creates an alias so file2alias.c can find device table.
> > > *
> > > * Use this in cases where a device table is used to match devices because it
> > > * surfaces match-id based module aliases to userspace for:
> > > * - Automatic module loading.
> > > * - Tools like USBGuard which allow or block devices based on policy such as
> > > * which modules match a device.
> > > *
> > > * The module name is included in the alias for two reasons:
> > > * - It avoids creating two aliases with the same name for built-in modules.
> > > * Historically MODULE_DEVICE_TABLE was a no-op for built-in modules, so
> > > * there was nothing to stop different modules from having the same device
> > > * table name and consequently the same alias when building as a module.
> > > * - The module name is needed by files2alias.c to associate a particular
> > > * device table with its associated module for built-in modules since
> > > * files2alias would otherwise see the module name as `vmlinuz.o`.
> > > */
> >
> > This is still weak in light of the questions I had. It does not make it
> > easy for a driver developer who is going to support only built-in only
> > if they need to define this or not. And it seems we're still discussing
> > the merits of this, so I'd wait until this is fleshed out, but I think
> > we are on the right track finally.
> >
> > > The deciding factor in whether it makes sense to remove these vs fix
> > > them seems to be, "How complete do we want modules.builtin.alias to
> > > be?"
> > >
> > > Arguably we should just drop these in cases where there isn't an
> > > "authorized" sysfs attribute but following that logic there is not any
> > > reason to generate built-in aliases for anything except USB and
> > > thunderbolt.
> >
> > There we go, now we have a *real* use case for this for built-in stuff
> > to consider. Is USBGuard effective even for built-in stuff?
>
> Yes, just because a module is loaded doesn't mean a specific device
> has probed the driver yet.
>
> >
> > Given everything discussed so far I'd like to get clarification if it
> > even help for built-in USB / thunderbolt. Does it? If so how? What could
> > userspace do with this information if the driver is already built-in?
>
> We are not trying to stop the module from being loaded (which is
> always the case for built-in modules) and in fact it is possible to
> have devices already using the module and still not authorize (and by
> extension probe the module for) newly connected devices.
>
> For example someone might have an unattended computer downloading
> installation media to a USB drive. Presumably this computer would be
> locked to make it more difficult for a bad actor to access the
> computer. Since USB storage devices are not needed to interact with
> the lock screen, we can use the authorized_default sysfs attribute to
> not allow new USB devices to probe modules by default and have
> USBGuard vet the devices. Mice, keyboards, etc can be allowed so that
> the lock screen can still be used (this important in cases like
> suspend+resume or docks).
I see thanks!
> > > On the flip side, if we are going to the effort to make this a generic
> > > solution that covers everything, the built-in aliases are only as
> > > useful as they are complete, so we would want everything that defines
> > > a device table to call the macro correctly.
> >
> > It is the ambiguity which is terrible to add. If the only use case is
> > for USB and Thunderbolt then we can spell it out, then only those driver
> > developers would care to consider it if the driver is bool. And, a
> > respective tooling would scrape only those drivers to verify if the
> > table is missing for built-in too.
>
> I was aiming to write it so that it wouldn't easily become obsolete by
> later changes, so tying it to the authorized and authorized_default
> sysfs attributes is probably the ideal deciding factor and listing USB
> and thunderbolt as examples makes sense.
I think it would make sense then to be explicit about this for now, even
if it seems we can obsolete this. Right now the justification for having
this for built-in is *very* specific to this feature for USB, which
makes use of special USB sysfs attributes which as you explained, allows
to restrict probe of devices even though the respective driver is already
loaded.
> There are sysfs attributes called authorized and authorized_default
> that together can prevent devices from being fully enumerated and
> probed.
Although these attributes are USB specfic today it gets me wondering if
other subsystems may benefit from a similar feature.
> authorized_default gets set to 0 for the hub and any devices
> connected after that will show in sysfs, but not fully enumerate or
> probe until the device's authorized attribute is set to 1. There are
> some edge cases like internal devices which have some extra
> complexity.
>
> As for documentation, I wasn't able to find much other than:
> https://github.com/torvalds/linux/blob/v6.1/drivers/usb/core/hcd.c#L370
> /* authorized_default behaviour:
> * -1 is authorized for all devices except wireless (old behaviour)
> * 0 is unauthorized for all devices
> * 1 is authorized for all devices
> * 2 is authorized for internal devices
> */
> ...
> and
> https://github.com/torvalds/linux/blob/v6.1/Documentation/admin-guide/kernel-parameters.txt#L6424
> usbcore.authorized_default=
> [USB] Default USB device authorization:
> (default -1 = authorized except for wireless USB,
> 0 = not authorized, 1 = authorized, 2 = authorized
> if device connected to internal port)
> ...
> The feature looks like it was originally introduced for wireless USB in:
> https://www.mail-archive.com/[email protected]/msg54289.html
> and later adapted for use cases like USBGuard here:
> https://github.com/torvalds/linux/commit/c4fc2342cb611f945fa468e742759e25984005ad
Thanks for digging all this up. Can you extend the docs on
Documentation/driver-api/usb/ somewhere about this attribute as part of
your changes so its clear the motivation, *then* you make your changes.
The documentation for MODULE_DEVICE_TABLE() can just say:
The only use-case for built-in drivers today is enable userspace to
prevent / allow probe for devices on certain subsystems even if the
driver is already loaded. An example is the USB subsystem with its
authorized_default sysfs attribute. For more details refer to the
kernel's Documentation for USB about authorized_default.
That should be clear enough for both USB driver writers and others.
Please also extend the docs for MODULE_DEVICE_TABLE() on
Documentation/driver-api/usb/writing_usb_driver.rst or where you see
fit for your changes. That can go into depth about the USBGuard stuff.
Luis
On Tue, Dec 20, 2022 at 5:09 PM Luis Chamberlain <[email protected]> wrote:
>
> On Tue, Dec 20, 2022 at 03:57:57PM -0600, Allen Webb wrote:
> > On Tue, Dec 20, 2022 at 2:03 PM Luis Chamberlain <[email protected]> wrote:
> > >
> > > On Tue, Dec 20, 2022 at 01:49:04PM -0600, Allen Webb wrote:
> > > > I took another stab at clarifying (and also dropped the ifdev since
> > > > the same macro works both for separate and built-in modules:
> > > >
> > > > /*
> > > > * Creates an alias so file2alias.c can find device table.
> > > > *
> > > > * Use this in cases where a device table is used to match devices because it
> > > > * surfaces match-id based module aliases to userspace for:
> > > > * - Automatic module loading.
> > > > * - Tools like USBGuard which allow or block devices based on policy such as
> > > > * which modules match a device.
> > > > *
> > > > * The module name is included in the alias for two reasons:
> > > > * - It avoids creating two aliases with the same name for built-in modules.
> > > > * Historically MODULE_DEVICE_TABLE was a no-op for built-in modules, so
> > > > * there was nothing to stop different modules from having the same device
> > > > * table name and consequently the same alias when building as a module.
> > > > * - The module name is needed by files2alias.c to associate a particular
> > > > * device table with its associated module for built-in modules since
> > > > * files2alias would otherwise see the module name as `vmlinuz.o`.
> > > > */
> > >
> > > This is still weak in light of the questions I had. It does not make it
> > > easy for a driver developer who is going to support only built-in only
> > > if they need to define this or not. And it seems we're still discussing
> > > the merits of this, so I'd wait until this is fleshed out, but I think
> > > we are on the right track finally.
> > >
> > > > The deciding factor in whether it makes sense to remove these vs fix
> > > > them seems to be, "How complete do we want modules.builtin.alias to
> > > > be?"
> > > >
> > > > Arguably we should just drop these in cases where there isn't an
> > > > "authorized" sysfs attribute but following that logic there is not any
> > > > reason to generate built-in aliases for anything except USB and
> > > > thunderbolt.
> > >
> > > There we go, now we have a *real* use case for this for built-in stuff
> > > to consider. Is USBGuard effective even for built-in stuff?
> >
> > Yes, just because a module is loaded doesn't mean a specific device
> > has probed the driver yet.
> >
> > >
> > > Given everything discussed so far I'd like to get clarification if it
> > > even help for built-in USB / thunderbolt. Does it? If so how? What could
> > > userspace do with this information if the driver is already built-in?
> >
> > We are not trying to stop the module from being loaded (which is
> > always the case for built-in modules) and in fact it is possible to
> > have devices already using the module and still not authorize (and by
> > extension probe the module for) newly connected devices.
> >
> > For example someone might have an unattended computer downloading
> > installation media to a USB drive. Presumably this computer would be
> > locked to make it more difficult for a bad actor to access the
> > computer. Since USB storage devices are not needed to interact with
> > the lock screen, we can use the authorized_default sysfs attribute to
> > not allow new USB devices to probe modules by default and have
> > USBGuard vet the devices. Mice, keyboards, etc can be allowed so that
> > the lock screen can still be used (this important in cases like
> > suspend+resume or docks).
>
> I see thanks!
>
> > > > On the flip side, if we are going to the effort to make this a generic
> > > > solution that covers everything, the built-in aliases are only as
> > > > useful as they are complete, so we would want everything that defines
> > > > a device table to call the macro correctly.
> > >
> > > It is the ambiguity which is terrible to add. If the only use case is
> > > for USB and Thunderbolt then we can spell it out, then only those driver
> > > developers would care to consider it if the driver is bool. And, a
> > > respective tooling would scrape only those drivers to verify if the
> > > table is missing for built-in too.
> >
> > I was aiming to write it so that it wouldn't easily become obsolete by
> > later changes, so tying it to the authorized and authorized_default
> > sysfs attributes is probably the ideal deciding factor and listing USB
> > and thunderbolt as examples makes sense.
>
> I think it would make sense then to be explicit about this for now, even
> if it seems we can obsolete this. Right now the justification for having
> this for built-in is *very* specific to this feature for USB, which
> makes use of special USB sysfs attributes which as you explained, allows
> to restrict probe of devices even though the respective driver is already
> loaded.
The thing we might obsolete is limiting it to just the USB subsystem.
I am fine with expanding the documentation and limiting the scope of
the feature to USB/thunderbolt for now.
>
> > There are sysfs attributes called authorized and authorized_default
> > that together can prevent devices from being fully enumerated and
> > probed.
>
> Although these attributes are USB specfic today it gets me wondering if
> other subsystems may benefit from a similar feature.
The subsystems that would likely benefit the most are ones that are
externally reachable. The external ports that come to mind are USB /
thunderbolt, firewire, PCMCIA / expresscard, eSATA, serial and
parallel ports. Supporting PCMCIA / expresscard seems like it would
require adding the authorized sysfs attribute to pci. eSATA would be
covered by ata.
>
> > authorized_default gets set to 0 for the hub and any devices
> > connected after that will show in sysfs, but not fully enumerate or
> > probe until the device's authorized attribute is set to 1. There are
> > some edge cases like internal devices which have some extra
> > complexity.
> >
> > As for documentation, I wasn't able to find much other than:
> > https://github.com/torvalds/linux/blob/v6.1/drivers/usb/core/hcd.c#L370
> > /* authorized_default behaviour:
> > * -1 is authorized for all devices except wireless (old behaviour)
> > * 0 is unauthorized for all devices
> > * 1 is authorized for all devices
> > * 2 is authorized for internal devices
> > */
> > ...
> > and
> > https://github.com/torvalds/linux/blob/v6.1/Documentation/admin-guide/kernel-parameters.txt#L6424
> > usbcore.authorized_default=
> > [USB] Default USB device authorization:
> > (default -1 = authorized except for wireless USB,
> > 0 = not authorized, 1 = authorized, 2 = authorized
> > if device connected to internal port)
> > ...
> > The feature looks like it was originally introduced for wireless USB in:
> > https://www.mail-archive.com/[email protected]/msg54289.html
> > and later adapted for use cases like USBGuard here:
> > https://github.com/torvalds/linux/commit/c4fc2342cb611f945fa468e742759e25984005ad
>
> Thanks for digging all this up. Can you extend the docs on
> Documentation/driver-api/usb/ somewhere about this attribute as part of
> your changes so its clear the motivation, *then* you make your changes.
> The documentation for MODULE_DEVICE_TABLE() can just say:
>
> The only use-case for built-in drivers today is enable userspace to
> prevent / allow probe for devices on certain subsystems even if the
> driver is already loaded. An example is the USB subsystem with its
> authorized_default sysfs attribute. For more details refer to the
> kernel's Documentation for USB about authorized_default.
>
> That should be clear enough for both USB driver writers and others.
>
> Please also extend the docs for MODULE_DEVICE_TABLE() on
> Documentation/driver-api/usb/writing_usb_driver.rst or where you see
> fit for your changes. That can go into depth about the USBGuard stuff.
>
> Luis
How do you feel about only having one version of the macro for both
cases and merging the documentation so things are kept simple? Here is
what I have locally for the macro without the ifdef and the updated
documentation:
/*
* Creates an alias so file2alias.c can find device table.
*
* Use this in cases where a device table is used to match devices because it
* surfaces match-id based module aliases to userspace for:
* - Automatic module loading through modules.alias.
* - Tools like USBGuard which allow or block devices based on policy such as
* which modules match a device.
*
* The only use-case for built-in drivers today is enable userspace to prevent /
* allow probe for devices on certain subsystems even if the driver is already
* loaded. An example is the USB subsystem with its authorized_default sysfs
* attribute. For more details refer to the kernel's Documentation for USB about
* authorized_default.
*
* The module name is included in the alias for two reasons:
* - It avoids creating two aliases with the same name for built-in modules.
* Historically MODULE_DEVICE_TABLE was a no-op for built-in modules, so
* there was nothing to stop different modules from having the same device
* table name and consequently the same alias when building as a module.
* - The module name is needed by files2alias.c to associate a particular
* device table with its associated module for built-in modules since
* files2alias would otherwise see the module name as `vmlinuz.o`.
*/
#define MODULE_DEVICE_TABLE(type, name) \
extern void *CONCATENATE( \
CONCATENATE(__mod_##type##__##name##__, \
__KBUILD_MODNAME), \
_device_table) \
__attribute__ ((unused, alias(__stringify(name))))
Here is a draft version for an updated to
Documentation/driver-api/usb/ (I will add the 80 char line breaks
later) in case you have feedback:
# Authorization
Authorization provides userspace a way to allow or block configuring
devices early during enumeration before any modules are probed for the
device. While it is possible to block a device by not loading the
required modules, this also prevents other devices from using the
module as well. For example someone might have an unattended computer
downloading installation media to a USB drive. Presumably this
computer would be locked to make it more difficult for a bad actor to
access the computer. Since USB storage devices are not needed to
interact with the lock screen, the authorized_default sysfs attribute
can be set to not authorize new USB devices by default. A userspace
tool like USBGuard can then vet the devices. Mice, keyboards, etc can
be allowed by writing to their authorized sysfs attribute so that the
lock screen can still be used (this important in cases like
suspend+resume or docks) while other devices can be blocked as long as
the lock screen is shown.
## Sysfs Attributes
Userspace can control USB device authorization through the
authorized_default and authorized sysfs attributes.
### authorized_default
.. kernel-doc:: drivers/usb/core/hcd.c
:export:
The authorized_default sysfs attribute is only present for host
controllers. It determines the initial state of the authorized sysfs
attribute of USB devices newly connected to the corresponding host
controller. It can take on the following values:
+---------------------------------------------------+
| Value | Behavior |
+=======+===========================================+
| -1 | Authorize all devices except wireless USB |
+-------+-------------------------------------------+
| 0 | Do not authorize new devices |
+-------+-------------------------------------------+
| 1 | Authorize new devices |
+-------+-------------------------------------------+
| 2 | Authorize new internal devices only |
+---------------------------------------------------+
Note that firmware platform code determines if a device is internal or
not and this is reported as the connect_type sysfs attribute of the
USB port. This is currently supported by ACPI, but device tree still
needs an implementation. Authorizing new internal devices only can be
useful to work around issues with devices that misbehave if there are
delays in probing their module.
### authorized
.. kernel-doc:: drivers/usb/core/sysfs.c
:export:
Every USB device has an authorized sysfs attribute which can take the
values 0 and 1. When authorized is 0, the device still is present in
sysfs, but none of its interfaces can be associated with drivers and
modules will not be probed. When authorized is 1 (or set to one) a
configuration is chosen for the device and its interfaces are
registered allowing drivers to bind to them.
On 20 Dec 2022, Luis Chamberlain uttered the following:
>> It also raises the question how many modules have device tables, but
>> do not call MODULE_DEVICE_TABLE since they are only ever built-in.
>> Maybe there should be some build time enforcement mechanism to make
>> sure that these are consistent.
>
> Definitely, Nick Alcock is doing some related work where the semantics
> of built-in modules needs to be clearer, he for instance is now removing
> a few MODULE_() macros for things which *are never* modules, and this is
> because after commit 8b41fc4454e ("kbuild: create modules.builtin
> without Makefile.modbuiltin or tristate.conf") we rely on the module
> license tag to generate the modules.builtin file. Without that commit
> we end up traversing the source tree twice. Nick's work builds on
> that work and futher clarifies these semantics by adding tooling which
> complains when something which is *never* capable of being a module
> uses module macros. The macro you are extending, MODULE_DEVICE_TABLE(),
> today is a no-op for built-in, but you are adding support to extend it
> for built-in stuff. Nick's work will help with clarifying symbol locality
> and so he may be interested in your association for the data in
> MODULE_DEVICE_TABLE and how you associate to a respective would-be
> module. His work is useful for making tracing more accurate with respect
> to symbol associations, so the data in MODULE_DEVICE_TABLE() may be
> useful as well to him.
The kallmodsyms module info (and, thus, modules.builtin) and
MODULE_DEVICE_TABLE do seem interestingly related. I wonder if we can in
future reuse at least the module names so we can save a few KiB more
space... (in this case, the canonical copy should probably be the one in
kallmodsyms, because that lets kallmodsyms reuse strings where modules
and their source file have similar names. Something for the future...)
> You folks may want to Cc each other on your patches.
I'd welcome that.
btw, do you want another kallmodsyms patch series from me just arranging
to drop fewer MODULE_ entries from non-modules (just MODULE_LICENSE) or
would this be considered noise for now? (Are we deadlocked on each
other, or are you still looking at the last series I sent, which I think
was v10 in late November?)
--
NULL && (void)
On Tue, Dec 27, 2022 at 11:42:36AM -0600, Allen Webb wrote:
> On Tue, Dec 20, 2022 at 5:09 PM Luis Chamberlain <[email protected]> wrote:
> > I think it would make sense then to be explicit about this for now, even
> > if it seems we can obsolete this. Right now the justification for having
> > this for built-in is *very* specific to this feature for USB, which
> > makes use of special USB sysfs attributes which as you explained, allows
> > to restrict probe of devices even though the respective driver is already
> > loaded.
>
> The thing we might obsolete is limiting it to just the USB subsystem.
> I am fine with expanding the documentation and limiting the scope of
> the feature to USB/thunderbolt for now.
Great let's do that as otherwise it can leave a few folks scratchign
their head.
> > > There are sysfs attributes called authorized and authorized_default
> > > that together can prevent devices from being fully enumerated and
> > > probed.
> >
> > Although these attributes are USB specfic today it gets me wondering if
> > other subsystems may benefit from a similar feature.
>
> The subsystems that would likely benefit the most are ones that are
> externally reachable.
Makes sense.
> The external ports that come to mind are USB /
> thunderbolt, firewire, PCMCIA / expresscard, eSATA, serial and
> parallel ports. Supporting PCMCIA / expresscard seems like it would
> require adding the authorized sysfs attribute to pci. eSATA would be
> covered by ata.
Makes sense, I'd personally ignore anything legacy such as PCMCIA though.
> > > authorized_default gets set to 0 for the hub and any devices
> > > connected after that will show in sysfs, but not fully enumerate or
> > > probe until the device's authorized attribute is set to 1. There are
> > > some edge cases like internal devices which have some extra
> > > complexity.
> > >
> > > As for documentation, I wasn't able to find much other than:
> > > https://github.com/torvalds/linux/blob/v6.1/drivers/usb/core/hcd.c#L370
> > > /* authorized_default behaviour:
> > > * -1 is authorized for all devices except wireless (old behaviour)
> > > * 0 is unauthorized for all devices
> > > * 1 is authorized for all devices
> > > * 2 is authorized for internal devices
> > > */
> > > ...
> > > and
> > > https://github.com/torvalds/linux/blob/v6.1/Documentation/admin-guide/kernel-parameters.txt#L6424
> > > usbcore.authorized_default=
> > > [USB] Default USB device authorization:
> > > (default -1 = authorized except for wireless USB,
> > > 0 = not authorized, 1 = authorized, 2 = authorized
> > > if device connected to internal port)
> > > ...
> > > The feature looks like it was originally introduced for wireless USB in:
> > > https://www.mail-archive.com/[email protected]/msg54289.html
> > > and later adapted for use cases like USBGuard here:
> > > https://github.com/torvalds/linux/commit/c4fc2342cb611f945fa468e742759e25984005ad
> >
> > Thanks for digging all this up. Can you extend the docs on
> > Documentation/driver-api/usb/ somewhere about this attribute as part of
> > your changes so its clear the motivation, *then* you make your changes.
> > The documentation for MODULE_DEVICE_TABLE() can just say:
> >
> > The only use-case for built-in drivers today is enable userspace to
> > prevent / allow probe for devices on certain subsystems even if the
> > driver is already loaded. An example is the USB subsystem with its
> > authorized_default sysfs attribute. For more details refer to the
> > kernel's Documentation for USB about authorized_default.
> >
> > That should be clear enough for both USB driver writers and others.
> >
> > Please also extend the docs for MODULE_DEVICE_TABLE() on
> > Documentation/driver-api/usb/writing_usb_driver.rst or where you see
> > fit for your changes. That can go into depth about the USBGuard stuff.
> >
> > Luis
>
> How do you feel about only having one version of the macro for both
> cases and merging the documentation so things are kept simple? Here is
> what I have locally for the macro without the ifdef and the updated
> documentation:
>
> /*
> * Creates an alias so file2alias.c can find device table.
> *
> * Use this in cases where a device table is used to match devices because it
> * surfaces match-id based module aliases to userspace for:
> * - Automatic module loading through modules.alias.
> * - Tools like USBGuard which allow or block devices based on policy such as
^ allow to
> * which modules match a device.
> *
> * The only use-case for built-in drivers today is enable userspace to prevent /
^ is to
> * allow probe for devices on certain subsystems even if the driver is already
> * loaded. An example is the USB subsystem with its authorized_default sysfs
> * attribute. For more details refer to the kernel's Documentation for USB about
> * authorized_default.
> *
> * The module name is included in the alias for two reasons:
> * - It avoids creating two aliases with the same name for built-in modules.
> * Historically MODULE_DEVICE_TABLE was a no-op for built-in modules, so
> * there was nothing to stop different modules from having the same device
> * table name and consequently the same alias when building as a module.
> * - The module name is needed by files2alias.c to associate a particular
> * device table with its associated module for built-in modules since
> * files2alias would otherwise see the module name as `vmlinuz.o`.
Yeah sure this reads much better.
> */
> #define MODULE_DEVICE_TABLE(type, name) \
> extern void *CONCATENATE( \
> CONCATENATE(__mod_##type##__##name##__, \
> __KBUILD_MODNAME), \
> _device_table) \
> __attribute__ ((unused, alias(__stringify(name))))
>
>
> Here is a draft version for an updated to
> Documentation/driver-api/usb/ (I will add the 80 char line breaks
> later) in case you have feedback:
>
>
> # Authorization
>
> Authorization provides userspace a way to allow or block configuring
> devices early during enumeration before any modules are probed for the
> device. While it is possible to block a device by not loading the
> required modules, this also prevents other devices from using the
> module as well. For example someone might have an unattended computer
> downloading installation media to a USB drive. Presumably this
> computer would be locked to make it more difficult for a bad actor to
> access the computer. Since USB storage devices are not needed to
> interact with the lock screen, the authorized_default sysfs attribute
> can be set to not authorize new USB devices by default. A userspace
> tool like USBGuard can then vet the devices. Mice, keyboards, etc can
> be allowed by writing to their authorized sysfs attribute so that the
> lock screen can still be used (this important in cases like
> suspend+resume or docks) while other devices can be blocked as long as
> the lock screen is shown.
>
> ## Sysfs Attributes
>
> Userspace can control USB device authorization through the
> authorized_default and authorized sysfs attributes.
>
> ### authorized_default
>
> .. kernel-doc:: drivers/usb/core/hcd.c
> :export:
>
> The authorized_default sysfs attribute is only present for host
> controllers. It determines the initial state of the authorized sysfs
> attribute of USB devices newly connected to the corresponding host
> controller. It can take on the following values:
>
> +---------------------------------------------------+
> | Value | Behavior |
> +=======+===========================================+
> | -1 | Authorize all devices except wireless USB |
> +-------+-------------------------------------------+
> | 0 | Do not authorize new devices |
> +-------+-------------------------------------------+
> | 1 | Authorize new devices |
> +-------+-------------------------------------------+
> | 2 | Authorize new internal devices only |
> +---------------------------------------------------+
>
> Note that firmware platform code determines if a device is internal or
> not and this is reported as the connect_type sysfs attribute of the
> USB port. This is currently supported by ACPI, but device tree still
> needs an implementation. Authorizing new internal devices only can be
> useful to work around issues with devices that misbehave if there are
> delays in probing their module.
>
> ### authorized
>
> .. kernel-doc:: drivers/usb/core/sysfs.c
> :export:
>
> Every USB device has an authorized sysfs attribute which can take the
> values 0 and 1. When authorized is 0, the device still is present in
> sysfs, but none of its interfaces can be associated with drivers and
> modules will not be probed. When authorized is 1 (or set to one) a
> configuration is chosen for the device and its interfaces are
> registered allowing drivers to bind to them.
Good stuff!
Luis
On Mon, Jan 9, 2023 at 5:54 AM Nick Alcock <[email protected]> wrote:
>
> On 20 Dec 2022, Luis Chamberlain uttered the following:
> >> It also raises the question how many modules have device tables, but
> >> do not call MODULE_DEVICE_TABLE since they are only ever built-in.
> >> Maybe there should be some build time enforcement mechanism to make
> >> sure that these are consistent.
> >
> > Definitely, Nick Alcock is doing some related work where the semantics
> > of built-in modules needs to be clearer, he for instance is now removing
> > a few MODULE_() macros for things which *are never* modules, and this is
> > because after commit 8b41fc4454e ("kbuild: create modules.builtin
> > without Makefile.modbuiltin or tristate.conf") we rely on the module
> > license tag to generate the modules.builtin file. Without that commit
> > we end up traversing the source tree twice. Nick's work builds on
> > that work and futher clarifies these semantics by adding tooling which
> > complains when something which is *never* capable of being a module
> > uses module macros. The macro you are extending, MODULE_DEVICE_TABLE(),
> > today is a no-op for built-in, but you are adding support to extend it
> > for built-in stuff. Nick's work will help with clarifying symbol locality
> > and so he may be interested in your association for the data in
> > MODULE_DEVICE_TABLE and how you associate to a respective would-be
> > module. His work is useful for making tracing more accurate with respect
> > to symbol associations, so the data in MODULE_DEVICE_TABLE() may be
> > useful as well to him.
>
> The kallmodsyms module info (and, thus, modules.builtin) and
> MODULE_DEVICE_TABLE do seem interestingly related. I wonder if we can in
> future reuse at least the module names so we can save a few KiB more
> space... (in this case, the canonical copy should probably be the one in
> kallmodsyms, because that lets kallmodsyms reuse strings where modules
> and their source file have similar names. Something for the future...)
It appeared to me like the symbols added for MODULE_DEVICE_TABLE are
only needed temporarily and could be stripped as part of the final
linking step. This would make space less of a concern, but extern
variables don't support the visibility attribute and in the build I am
using the space difference is less than 1MB out of 613MB for the
uncompressed kernel.
>
> > You folks may want to Cc each other on your patches.
>
> I'd welcome that.
>
> btw, do you want another kallmodsyms patch series from me just arranging
> to drop fewer MODULE_ entries from non-modules (just MODULE_LICENSE) or
> would this be considered noise for now? (Are we deadlocked on each
> other, or are you still looking at the last series I sent, which I think
> was v10 in late November?)
For now I just need MODULE_DEVICE_TABLE to stick around for USB and
thunderbolt related modules (including built-in modules), so if you
aren't removing it for any then I don't think we are blocking each
other.
Longer term it makes sense to have MODULE_DEVICE_TABLE for any module
that makes use of a subsystem that had the authorized attribute. While
this is currently just USB/thunderbolt it could expand in the future,
but there are subsystems where it is likely to make no difference.
We might have a tiny amount of redundancy in our patch sets because
there are some cases of invalid MODULE_DEVICE_TABLE entries I fixed in
my patch series, but that could be dropped. These have the potential
for conflicts / blocking each other, but it should be easy to resolve
them if I change my fixes to a removal of the MODULE_DEVICE_TABLE
entries.
>
> --
> NULL && (void)