2019-06-07 16:26:53

by Paul Cercueil

[permalink] [raw]
Subject: [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module

This allows the probe function to be dropped after the kernel finished
its initialization, in the case where the driver was not compiled as a
module.

Signed-off-by: Paul Cercueil <[email protected]>
---

Notes:
v2: New patch

drivers/watchdog/jz4740_wdt.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 7519d80c5d05..2061788c1939 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -157,7 +157,7 @@ static const struct of_device_id jz4740_wdt_of_matches[] = {
MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);
#endif

-static int jz4740_wdt_probe(struct platform_device *pdev)
+static int __init_or_module jz4740_wdt_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct jz4740_wdt_drvdata *drvdata;
--
2.21.0.593.g511ec345e18


2019-06-07 18:58:50

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module

On Fri, Jun 07, 2019 at 06:24:29PM +0200, Paul Cercueil wrote:
> This allows the probe function to be dropped after the kernel finished
> its initialization, in the case where the driver was not compiled as a
> module.
>
> Signed-off-by: Paul Cercueil <[email protected]>

Reviewed-by: Guenter Roeck <[email protected]>

> ---
>
> Notes:
> v2: New patch
>
> drivers/watchdog/jz4740_wdt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 7519d80c5d05..2061788c1939 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -157,7 +157,7 @@ static const struct of_device_id jz4740_wdt_of_matches[] = {
> MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);
> #endif
>
> -static int jz4740_wdt_probe(struct platform_device *pdev)
> +static int __init_or_module jz4740_wdt_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct jz4740_wdt_drvdata *drvdata;

2019-06-08 07:56:37

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module

Hi Paul,

Le 07/06/2019 à 18:24, Paul Cercueil a écrit :
> This allows the probe function to be dropped after the kernel finished
> its initialization, in the case where the driver was not compiled as a
> module.

I'm not sure that's what __init_or_module flag does.

As far as I understand, this flag makes the function being dropped only
when the kernel is built without modules support, ie without
CONFIG_MODULES. See
https://elixir.bootlin.com/linux/latest/source/include/linux/module.h#L145

In addition, I'm not sure you can simply define a probe function as
__init. What if someone tries to unbind and rebind the device through
sysfs for instance ?

It seems there is a special function called __platform_driver_probe()
for registering devices when the probe function is to be in __init, see
https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L684

Christophe

>
> Signed-off-by: Paul Cercueil <[email protected]>
> ---
>
> Notes:
> v2: New patch
>
> drivers/watchdog/jz4740_wdt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 7519d80c5d05..2061788c1939 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -157,7 +157,7 @@ static const struct of_device_id jz4740_wdt_of_matches[] = {
> MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);
> #endif
>
> -static int jz4740_wdt_probe(struct platform_device *pdev)
> +static int __init_or_module jz4740_wdt_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct jz4740_wdt_drvdata *drvdata;
>

2019-06-08 09:59:14

by Paul Cercueil

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module

Hi Christophe,

Le sam. 8 juin 2019 ? 9:51, Christophe Leroy <[email protected]>
a ?crit :
> Hi Paul,
>
> Le 07/06/2019 ? 18:24, Paul Cercueil a ?crit :
>> This allows the probe function to be dropped after the kernel
>> finished
>> its initialization, in the case where the driver was not compiled as
>> a
>> module.
>
> I'm not sure that's what __init_or_module flag does.
>
> As far as I understand, this flag makes the function being dropped
> only when the kernel is built without modules support, ie without
> CONFIG_MODULES. See
> https://elixir.bootlin.com/linux/latest/source/include/linux/module.h#L145

So it doesn't depend on the driver being built-in or compiled as a
module?

> In addition, I'm not sure you can simply define a probe function as
> __init. What if someone tries to unbind and rebind the device through
> sysfs for instance ?

Ouch. I feel stupid now.

> It seems there is a special function called __platform_driver_probe()
> for registering devices when the probe function is to be in __init,
> see
> https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L684

Yes, but only usable by drivers that won't defer probe, and it removes
the bind/unbind attributes from sysfs,
so it shouldn't be used for non-critical drivers, I think.

> Christophe
>
>>
>> Signed-off-by: Paul Cercueil <[email protected]>
>> ---
>>
>> Notes:
>> v2: New patch
>>
>> drivers/watchdog/jz4740_wdt.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/watchdog/jz4740_wdt.c
>> b/drivers/watchdog/jz4740_wdt.c
>> index 7519d80c5d05..2061788c1939 100644
>> --- a/drivers/watchdog/jz4740_wdt.c
>> +++ b/drivers/watchdog/jz4740_wdt.c
>> @@ -157,7 +157,7 @@ static const struct of_device_id
>> jz4740_wdt_of_matches[] = {
>> MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);
>> #endif
>> -static int jz4740_wdt_probe(struct platform_device *pdev)
>> +static int __init_or_module jz4740_wdt_probe(struct platform_device
>> *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> struct jz4740_wdt_drvdata *drvdata;
>>


2019-06-11 05:17:15

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module

Hi Paul,

Le 08/06/2019 à 11:57, Paul Cercueil a écrit :
> Hi Christophe,
>
> Le sam. 8 juin 2019 à 9:51, Christophe Leroy <[email protected]> a
> écrit :
>> Hi Paul,
>>
>> Le 07/06/2019 à 18:24, Paul Cercueil a écrit :
>>> This allows the probe function to be dropped after the kernel finished
>>> its initialization, in the case where the driver was not compiled as a
>>> module.
>>
>> I'm not sure that's what  __init_or_module flag does.
>>
>> As far as I understand, this flag makes the function being dropped
>> only when the kernel is built without modules support, ie without
>> CONFIG_MODULES. See
>> https://elixir.bootlin.com/linux/latest/source/include/linux/module.h#L145
>>
>
> So it doesn't depend on the driver being built-in or compiled as a module?

No it doesn't. This flag is for built-in functions that are needed by
init and modules init. If the kernel doesn't support modules, it can
drop the function after init. If the kernel supports modules, it has to
keep the function. That's what this flag is made for.

If your need is to mark a function so that it gets discarded after init
or module init, just mark it __init. If it is built in, it will be
dropped after init. If it is in a module, it will be dropped after the
module is initialised.

>
>> In addition, I'm not sure you can simply define a probe function as
>> __init. What if someone tries to unbind and rebind the device through
>> sysfs for instance ?
>
> Ouch. I feel stupid now.
>
>> It seems there is a special function called __platform_driver_probe()
>> for registering devices when the probe function is to be in __init,
>> see
>> https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L684
>>
>
> Yes, but only usable by drivers that won't defer probe, and it removes
> the bind/unbind attributes from sysfs,
> so it shouldn't be used for non-critical drivers, I think.

I guess it would make sense for watchdog drivers, we don't expect this
kind of driver to be unbinded, do we ?

Christophe

>
>> Christophe
>>
>>>
>>> Signed-off-by: Paul Cercueil <[email protected]>
>>> ---
>>>
>>> Notes:
>>>      v2: New patch
>>>
>>>   drivers/watchdog/jz4740_wdt.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/watchdog/jz4740_wdt.c
>>> b/drivers/watchdog/jz4740_wdt.c
>>> index 7519d80c5d05..2061788c1939 100644
>>> --- a/drivers/watchdog/jz4740_wdt.c
>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>> @@ -157,7 +157,7 @@ static const struct of_device_id
>>> jz4740_wdt_of_matches[] = {
>>>   MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);
>>>   #endif
>>>   -static int jz4740_wdt_probe(struct platform_device *pdev)
>>> +static int __init_or_module jz4740_wdt_probe(struct platform_device
>>> *pdev)
>>>   {
>>>       struct device *dev = &pdev->dev;
>>>       struct jz4740_wdt_drvdata *drvdata;
>>>
>

2019-06-11 12:19:20

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] watchdog: jz4740: Make probe function __init_or_module

On 6/10/19 10:16 PM, Christophe Leroy wrote:
> Hi Paul,
>
> Le 08/06/2019 à 11:57, Paul Cercueil a écrit :
>> Hi Christophe,
>>
>> Le sam. 8 juin 2019 à 9:51, Christophe Leroy <[email protected]> a écrit :
>>> Hi Paul,
>>>
>>> Le 07/06/2019 à 18:24, Paul Cercueil a écrit :
>>>> This allows the probe function to be dropped after the kernel finished
>>>> its initialization, in the case where the driver was not compiled as a
>>>> module.
>>>
>>> I'm not sure that's what  __init_or_module flag does.
>>>
>>> As far as I understand, this flag makes the function being dropped only when the kernel is built without modules support, ie without CONFIG_MODULES. See https://elixir.bootlin.com/linux/latest/source/include/linux/module.h#L145
>>
>> So it doesn't depend on the driver being built-in or compiled as a module?
>
> No it doesn't. This flag is for built-in functions that are needed by init and modules init. If the kernel doesn't support modules, it can drop the function after init. If the kernel supports modules, it has to keep the function. That's what this flag is made for.
>
> If your need is to mark a function so that it gets discarded after init or module init, just mark it __init. If it is built in, it will be dropped after init. If it is in a module, it will be dropped after the module is initialised.
>
>>
>>> In addition, I'm not sure you can simply define a probe function as __init. What if someone tries to unbind and rebind the device through sysfs for instance ?
>>
>> Ouch. I feel stupid now.
>>
>>> It seems there is a special function called __platform_driver_probe() for registering devices when the probe function is to be in __init, see https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L684
>>
>> Yes, but only usable by drivers that won't defer probe, and it removes the bind/unbind attributes from sysfs,
>> so it shouldn't be used for non-critical drivers, I think.
>
> I guess it would make sense for watchdog drivers, we don't expect this kind of driver to be unbinded, do we ?
>

Except for debugging, where it can be extremely useful to be able to do it.

Guenter

> Christophe
>
>>
>>> Christophe
>>>
>>>>
>>>> Signed-off-by: Paul Cercueil <[email protected]>
>>>> ---
>>>>
>>>> Notes:
>>>>      v2: New patch
>>>>
>>>>   drivers/watchdog/jz4740_wdt.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
>>>> index 7519d80c5d05..2061788c1939 100644
>>>> --- a/drivers/watchdog/jz4740_wdt.c
>>>> +++ b/drivers/watchdog/jz4740_wdt.c
>>>> @@ -157,7 +157,7 @@ static const struct of_device_id jz4740_wdt_of_matches[] = {
>>>>   MODULE_DEVICE_TABLE(of, jz4740_wdt_of_matches);
>>>>   #endif
>>>>   -static int jz4740_wdt_probe(struct platform_device *pdev)
>>>> +static int __init_or_module jz4740_wdt_probe(struct platform_device *pdev)
>>>>   {
>>>>       struct device *dev = &pdev->dev;
>>>>       struct jz4740_wdt_drvdata *drvdata;
>>>>
>>
>