watchdog: rn5t618: use proper module tables

[PATCH] watchdog: rn5t618: use proper module tables

Posted by Andreas Kemnade 2 months, 1 week ago

Avoid requiring MODULE_ALIASES by declaring proper device id tables.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/watchdog/rn5t618_wdt.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/rn5t618_wdt.c b/drivers/watchdog/rn5t618_wdt.c
index 87d06d210ac9..97ef54f01ed9 100644
--- a/drivers/watchdog/rn5t618_wdt.c
+++ b/drivers/watchdog/rn5t618_wdt.c
@@ -8,6 +8,7 @@
 #include <linux/device.h>
 #include <linux/mfd/rn5t618.h>
 #include <linux/module.h>
+#include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
 #include <linux/watchdog.h>
 
@@ -181,16 +182,25 @@ static int rn5t618_wdt_probe(struct platform_device *pdev)
 	return devm_watchdog_register_device(dev, &wdt->wdt_dev);
 }
 
+static const struct platform_device_id rn5t618_wdt_id[] = {
+	{
+		.name = "rn5t618-wdt",
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(platform, rn5t618_wdt_id);
+
 static struct platform_driver rn5t618_wdt_driver = {
 	.probe = rn5t618_wdt_probe,
 	.driver = {
 		.name	= DRIVER_NAME,
 	},
+	.id_table = rn5t618_wdt_id,
 };
 
 module_platform_driver(rn5t618_wdt_driver);
 
-MODULE_ALIAS("platform:rn5t618-wdt");
 MODULE_AUTHOR("Beniamino Galvani <b.galvani@gmail.com>");
 MODULE_DESCRIPTION("RN5T618 watchdog driver");
 MODULE_LICENSE("GPL v2");
-- 
2.39.2

Re: [PATCH] watchdog: rn5t618: use proper module tables

Posted by Guenter Roeck 2 months, 1 week ago

On 9/18/24 14:29, Andreas Kemnade wrote:
> Avoid requiring MODULE_ALIASES by declaring proper device id tables.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>

This needs a better rationale. There are more than 40 watchdog drivers
using MODULE_ALIAS. I would hate having to deal with 40+ patches just
for cosmetic reasons, not counting the thousands of instances of
MODULE_ALIAS in the kernel, including the more than 1,000 instances of
"MODULE_ALIAS.*platform:".

Guenter

> ---
>   drivers/watchdog/rn5t618_wdt.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/rn5t618_wdt.c b/drivers/watchdog/rn5t618_wdt.c
> index 87d06d210ac9..97ef54f01ed9 100644
> --- a/drivers/watchdog/rn5t618_wdt.c
> +++ b/drivers/watchdog/rn5t618_wdt.c
> @@ -8,6 +8,7 @@
>   #include <linux/device.h>
>   #include <linux/mfd/rn5t618.h>
>   #include <linux/module.h>
> +#include <linux/mod_devicetable.h>
>   #include <linux/platform_device.h>
>   #include <linux/watchdog.h>
>   
> @@ -181,16 +182,25 @@ static int rn5t618_wdt_probe(struct platform_device *pdev)
>   	return devm_watchdog_register_device(dev, &wdt->wdt_dev);
>   }
>   
> +static const struct platform_device_id rn5t618_wdt_id[] = {
> +	{
> +		.name = "rn5t618-wdt",
> +	}, {
> +		/* sentinel */
> +	}
> +};
> +MODULE_DEVICE_TABLE(platform, rn5t618_wdt_id);
> +
>   static struct platform_driver rn5t618_wdt_driver = {
>   	.probe = rn5t618_wdt_probe,
>   	.driver = {
>   		.name	= DRIVER_NAME,
>   	},
> +	.id_table = rn5t618_wdt_id,
>   };
>   
>   module_platform_driver(rn5t618_wdt_driver);
>   
> -MODULE_ALIAS("platform:rn5t618-wdt");
>   MODULE_AUTHOR("Beniamino Galvani <b.galvani@gmail.com>");
>   MODULE_DESCRIPTION("RN5T618 watchdog driver");
>   MODULE_LICENSE("GPL v2");

Re: [PATCH] watchdog: rn5t618: use proper module tables

Posted by Andreas Kemnade 2 months, 1 week ago

Am Wed, 18 Sep 2024 15:43:40 -0700
schrieb Guenter Roeck <linux@roeck-us.net>:

> On 9/18/24 14:29, Andreas Kemnade wrote:
> > Avoid requiring MODULE_ALIASES by declaring proper device id tables.
> > 
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info>  
> 
> This needs a better rationale. There are more than 40 watchdog drivers
> using MODULE_ALIAS. I would hate having to deal with 40+ patches just
> for cosmetic reasons, not counting the thousands of instances of
> MODULE_ALIAS in the kernel, including the more than 1,000 instances of
> "MODULE_ALIAS.*platform:".
>
basically reviewers were arguing against patches from me bringing in
MODULE_ALIASES. So I decided to clean up a bit in my backyard. Not
sure whether such things could by done by coccinelle but at least
it could be tested via output of modinfo.

This is one example for such a patch:
https://lore.kernel.org/linux-clk/119f56c8-5f38-eb48-7157-6033932f0430@linaro.org/

Citing Krzysztof:

> > Is there a general consensus that MODULE_ALIAS("platform:.*") should
> > be exorcised? Of course for this new driver I will avoid it now
> > anyways.

> Whether "general" I don't know, but I was removing it quite a lot in
>the
> past. I think I removed all at some point, now I guess we have them
> back :/.

> MODULE_ALIAS is not the correct way to solve module matching problem.
>ID
> table with the correct way. Alias is just a workaround which now
> works,
> but later might stop (e.g. ID table will come with additional
>features).

Regards,
Andreas

Re: [PATCH] watchdog: rn5t618: use proper module tables

Posted by Krzysztof Kozlowski 2 months, 1 week ago

On 19/09/2024 12:50, Andreas Kemnade wrote:
> Am Wed, 18 Sep 2024 15:43:40 -0700
> schrieb Guenter Roeck <linux@roeck-us.net>:
> 
>> On 9/18/24 14:29, Andreas Kemnade wrote:
>>> Avoid requiring MODULE_ALIASES by declaring proper device id tables.
>>>
>>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>  
>>
>> This needs a better rationale. There are more than 40 watchdog drivers
>> using MODULE_ALIAS. I would hate having to deal with 40+ patches just
>> for cosmetic reasons, not counting the thousands of instances of
>> MODULE_ALIAS in the kernel, including the more than 1,000 instances of
>> "MODULE_ALIAS.*platform:".
>>
> basically reviewers were arguing against patches from me bringing in
> MODULE_ALIASES. So I decided to clean up a bit in my backyard. Not
> sure whether such things could by done by coccinelle but at least
> it could be tested via output of modinfo.
> 
> This is one example for such a patch:
> https://lore.kernel.org/linux-clk/119f56c8-5f38-eb48-7157-6033932f0430@linaro.org/
> 

There are multiple aspects here:
1. People (including me) copy code which they do no understand. Or
without really digging into it, because they do not have time. They just
copy it, regardless whether the code is necessary or not. MODULE_ALIAS
is one of such examples. It got copied to new drivers just because some
other driver had it.

2. MODULE_ALIAS creates basically ABI - some user-space might depend on
it, so removal might affect user. I think I was not dropping it from the
drivers in cases it would actually drop an alias. I was only dropping
duplicated aliases. That's not the case here, I believe.

3. MODULE_ALIAS scales poor. I believe proper xxx_device_id table is better.

4. But it does not mean that one single line - MODULE_ALIAS - should be
replaced in existing drivers into full-blown ID table. I think I never
proposed such patches for existing drivers. Why? Because if there was no
such need so far, means there were no scalability issues.

5. For new drivers I would propose to use ID table instead of
MODULE_ALIAS, even if it has one entry, because of above scalability.
But that's just my opinion and other person still might prefer might
concise ALIAS.

That's said, considering (4) above, I would not propose such patch. I
agree here with Guenter that you need proper rationale.

Best regards,
Krzysztof

Re: [PATCH] watchdog: rn5t618: use proper module tables

Posted by Andreas Kemnade 2 months, 1 week ago

Am Thu, 19 Sep 2024 13:02:55 +0200
schrieb Krzysztof Kozlowski <krzk@kernel.org>:

> On 19/09/2024 12:50, Andreas Kemnade wrote:
> > Am Wed, 18 Sep 2024 15:43:40 -0700
> > schrieb Guenter Roeck <linux@roeck-us.net>:
> >   
> >> On 9/18/24 14:29, Andreas Kemnade wrote:  
> >>> Avoid requiring MODULE_ALIASES by declaring proper device id
> >>> tables.
> >>>
> >>> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>    
> >>
> >> This needs a better rationale. There are more than 40 watchdog
> >> drivers using MODULE_ALIAS. I would hate having to deal with 40+
> >> patches just for cosmetic reasons, not counting the thousands of
> >> instances of MODULE_ALIAS in the kernel, including the more than
> >> 1,000 instances of "MODULE_ALIAS.*platform:".
> >>  
> > basically reviewers were arguing against patches from me bringing in
> > MODULE_ALIASES. So I decided to clean up a bit in my backyard. Not
> > sure whether such things could by done by coccinelle but at least
> > it could be tested via output of modinfo.
> > 
> > This is one example for such a patch:
> > https://lore.kernel.org/linux-clk/119f56c8-5f38-eb48-7157-6033932f0430@linaro.org/
> >   
> 
> There are multiple aspects here:
> 1. People (including me) copy code which they do no understand. Or
> without really digging into it, because they do not have time. They
> just copy it, regardless whether the code is necessary or not.
> MODULE_ALIAS is one of such examples. It got copied to new drivers
> just because some other driver had it.
> 
and copy nowadays unaccepted design patterns. Probably best to look at
the newest example.

> 2. MODULE_ALIAS creates basically ABI - some user-space might depend
> on it, so removal might affect user. I think I was not dropping it
> from the drivers in cases it would actually drop an alias. I was only
> dropping duplicated aliases. That's not the case here, I believe.
> 
> 3. MODULE_ALIAS scales poor. I believe proper xxx_device_id table is
> better.
> 
> 4. But it does not mean that one single line - MODULE_ALIAS - should
> be replaced in existing drivers into full-blown ID table. I think I
> never proposed such patches for existing drivers. Why? Because if
> there was no such need so far, means there were no scalability issues.
> 
Thanks for the long explanation.

Regards,
Andreas