clk: imx8m: Suppress bind attrs
diff mbox series

Message ID 9f2ac65bab203a943de947465d6534980585e144.1574116045.git.leonard.crestez@nxp.com
State Superseded, archived
Headers show
Series
  • clk: imx8m: Suppress bind attrs
Related show

Commit Message

Leonard Crestez Nov. 18, 2019, 10:28 p.m. UTC
The clock drivers on imx8m series are registered as platform devices and
this opens the possibility of reloading the driver at runtime:

This doesn't actually work: clocks are never removed and attempting to
bind again results in registration errors and a crash.

Fix this by explicitly suppressing bind attrs like several other
drivers.

Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

---
No cc: stable because because there are likely many other opportunities
to crash the system by echoing random stuff in sysfs as root.

 drivers/clk/imx/clk-imx8mm.c | 1 +
 drivers/clk/imx/clk-imx8mn.c | 1 +
 drivers/clk/imx/clk-imx8mq.c | 1 +
 3 files changed, 3 insertions(+)

Comments

Uwe Kleine-König Nov. 19, 2019, 7:09 a.m. UTC | #1
On Tue, Nov 19, 2019 at 12:28:16AM +0200, Leonard Crestez wrote:
> The clock drivers on imx8m series are registered as platform devices and
> this opens the possibility of reloading the driver at runtime:
> 
> This doesn't actually work: clocks are never removed and attempting to
> bind again results in registration errors and a crash.
> 
> Fix this by explicitly suppressing bind attrs like several other
> drivers.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> 
> ---
> No cc: stable because because there are likely many other opportunities
> to crash the system by echoing random stuff in sysfs as root.
> 
>  drivers/clk/imx/clk-imx8mm.c | 1 +
>  drivers/clk/imx/clk-imx8mn.c | 1 +
>  drivers/clk/imx/clk-imx8mq.c | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> index 9246e89bb5fd..3cb75ad4270d 100644
> --- a/drivers/clk/imx/clk-imx8mm.c
> +++ b/drivers/clk/imx/clk-imx8mm.c
> @@ -619,9 +619,10 @@ MODULE_DEVICE_TABLE(of, imx8mm_clk_of_match);
>  
>  static struct platform_driver imx8mm_clk_driver = {
>  	.probe = imx8mm_clocks_probe,
>  	.driver = {
>  		.name = "imx8mm-ccm",
> +		.suppress_bind_attrs = true,

Maybe add a comment similar to the motivation in the commit log here?
(And of course in the other files, too.)

Best regards
Uwe
Peng Fan Nov. 19, 2019, 9:11 a.m. UTC | #2
> Subject: [PATCH] clk: imx8m: Suppress bind attrs
> 
> The clock drivers on imx8m series are registered as platform devices and this
> opens the possibility of reloading the driver at runtime:
> 
> This doesn't actually work: clocks are never removed and attempting to bind
> again results in registration errors and a crash.
> 
> Fix this by explicitly suppressing bind attrs like several other drivers.
> 
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> 
> ---
> No cc: stable because because there are likely many other opportunities to
> crash the system by echoing random stuff in sysfs as root.
> 
>  drivers/clk/imx/clk-imx8mm.c | 1 +
>  drivers/clk/imx/clk-imx8mn.c | 1 +
>  drivers/clk/imx/clk-imx8mq.c | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> index 9246e89bb5fd..3cb75ad4270d 100644
> --- a/drivers/clk/imx/clk-imx8mm.c
> +++ b/drivers/clk/imx/clk-imx8mm.c
> @@ -619,9 +619,10 @@ MODULE_DEVICE_TABLE(of,
> imx8mm_clk_of_match);
> 
>  static struct platform_driver imx8mm_clk_driver = {
>  	.probe = imx8mm_clocks_probe,
>  	.driver = {
>  		.name = "imx8mm-ccm",
> +		.suppress_bind_attrs = true,
>  		.of_match_table = of_match_ptr(imx8mm_clk_of_match),
>  	},
>  };
>  module_platform_driver(imx8mm_clk_driver);
> diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
> index 4749beab9fc8..d16530430ac2 100644
> --- a/drivers/clk/imx/clk-imx8mn.c
> +++ b/drivers/clk/imx/clk-imx8mn.c
> @@ -576,9 +576,10 @@ MODULE_DEVICE_TABLE(of,
> imx8mn_clk_of_match);
> 
>  static struct platform_driver imx8mn_clk_driver = {
>  	.probe = imx8mn_clocks_probe,
>  	.driver = {
>  		.name = "imx8mn-ccm",
> +		.suppress_bind_attrs = true,
>  		.of_match_table = of_match_ptr(imx8mn_clk_of_match),
>  	},
>  };
>  module_platform_driver(imx8mn_clk_driver);
> diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
> index c8ab86fcba7c..0e0f69e881bd 100644
> --- a/drivers/clk/imx/clk-imx8mq.c
> +++ b/drivers/clk/imx/clk-imx8mq.c
> @@ -611,9 +611,10 @@ MODULE_DEVICE_TABLE(of,
> imx8mq_clk_of_match);
> 
>  static struct platform_driver imx8mq_clk_driver = {
>  	.probe = imx8mq_clocks_probe,
>  	.driver = {
>  		.name = "imx8mq-ccm",
> +		.suppress_bind_attrs = true,
>  		.of_match_table = of_match_ptr(imx8mq_clk_of_match),
>  	},
>  };
>  module_platform_driver(imx8mq_clk_driver);
> --
> 2.17.1
Leonard Crestez Nov. 19, 2019, 2:23 p.m. UTC | #3
On 2019-11-19 9:09 AM, Uwe Kleine-König wrote:
> On Tue, Nov 19, 2019 at 12:28:16AM +0200, Leonard Crestez wrote:
>> The clock drivers on imx8m series are registered as platform devices and
>> this opens the possibility of reloading the driver at runtime:
>>
>> This doesn't actually work: clocks are never removed and attempting to
>> bind again results in registration errors and a crash.
>>
>> Fix this by explicitly suppressing bind attrs like several other
>> drivers.
>>
>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>
>> ---
>> No cc: stable because because there are likely many other opportunities
>> to crash the system by echoing random stuff in sysfs as root.
>>
>>   drivers/clk/imx/clk-imx8mm.c | 1 +
>>   drivers/clk/imx/clk-imx8mn.c | 1 +
>>   drivers/clk/imx/clk-imx8mq.c | 1 +
>>   3 files changed, 3 insertions(+)
>>
>> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
>> index 9246e89bb5fd..3cb75ad4270d 100644
>> --- a/drivers/clk/imx/clk-imx8mm.c
>> +++ b/drivers/clk/imx/clk-imx8mm.c
>> @@ -619,9 +619,10 @@ MODULE_DEVICE_TABLE(of, imx8mm_clk_of_match);
>>   
>>   static struct platform_driver imx8mm_clk_driver = {
>>   	.probe = imx8mm_clocks_probe,
>>   	.driver = {
>>   		.name = "imx8mm-ccm",
>> +		.suppress_bind_attrs = true,
> 
> Maybe add a comment similar to the motivation in the commit log here?
> (And of course in the other files, too.)

Is it really useful to say "disable feature X because it doesn't work" 
right before disabling the feature?

--
Regards,
Leonard
Uwe Kleine-König Nov. 19, 2019, 3:06 p.m. UTC | #4
On Tue, Nov 19, 2019 at 02:23:08PM +0000, Leonard Crestez wrote:
> On 2019-11-19 9:09 AM, Uwe Kleine-König wrote:
> > On Tue, Nov 19, 2019 at 12:28:16AM +0200, Leonard Crestez wrote:
> >> The clock drivers on imx8m series are registered as platform devices and
> >> this opens the possibility of reloading the driver at runtime:
> >>
> >> This doesn't actually work: clocks are never removed and attempting to
> >> bind again results in registration errors and a crash.
> >>
> >> Fix this by explicitly suppressing bind attrs like several other
> >> drivers.
> >>
> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >>
> >> ---
> >> No cc: stable because because there are likely many other opportunities
> >> to crash the system by echoing random stuff in sysfs as root.
> >>
> >>   drivers/clk/imx/clk-imx8mm.c | 1 +
> >>   drivers/clk/imx/clk-imx8mn.c | 1 +
> >>   drivers/clk/imx/clk-imx8mq.c | 1 +
> >>   3 files changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> >> index 9246e89bb5fd..3cb75ad4270d 100644
> >> --- a/drivers/clk/imx/clk-imx8mm.c
> >> +++ b/drivers/clk/imx/clk-imx8mm.c
> >> @@ -619,9 +619,10 @@ MODULE_DEVICE_TABLE(of, imx8mm_clk_of_match);
> >>   
> >>   static struct platform_driver imx8mm_clk_driver = {
> >>   	.probe = imx8mm_clocks_probe,
> >>   	.driver = {
> >>   		.name = "imx8mm-ccm",
> >> +		.suppress_bind_attrs = true,
> > 
> > Maybe add a comment similar to the motivation in the commit log here?
> > (And of course in the other files, too.)
> 
> Is it really useful to say "disable feature X because it doesn't work" 
> right before disabling the feature?

No, but something like:

	/*
	 * disable bind attributes because clocks are never removed and
	 * attempting to rebind results in errors and a crash.
	 */

would be helpful. This way someone wondering about why this is disabled
gets at least an idea what to look for when changing something in that
area or while using the imx driver as a template for the next clock
driver.

Best regards
Uwe

Patch
diff mbox series

diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index 9246e89bb5fd..3cb75ad4270d 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -619,9 +619,10 @@  MODULE_DEVICE_TABLE(of, imx8mm_clk_of_match);
 
 static struct platform_driver imx8mm_clk_driver = {
 	.probe = imx8mm_clocks_probe,
 	.driver = {
 		.name = "imx8mm-ccm",
+		.suppress_bind_attrs = true,
 		.of_match_table = of_match_ptr(imx8mm_clk_of_match),
 	},
 };
 module_platform_driver(imx8mm_clk_driver);
diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
index 4749beab9fc8..d16530430ac2 100644
--- a/drivers/clk/imx/clk-imx8mn.c
+++ b/drivers/clk/imx/clk-imx8mn.c
@@ -576,9 +576,10 @@  MODULE_DEVICE_TABLE(of, imx8mn_clk_of_match);
 
 static struct platform_driver imx8mn_clk_driver = {
 	.probe = imx8mn_clocks_probe,
 	.driver = {
 		.name = "imx8mn-ccm",
+		.suppress_bind_attrs = true,
 		.of_match_table = of_match_ptr(imx8mn_clk_of_match),
 	},
 };
 module_platform_driver(imx8mn_clk_driver);
diff --git a/drivers/clk/imx/clk-imx8mq.c b/drivers/clk/imx/clk-imx8mq.c
index c8ab86fcba7c..0e0f69e881bd 100644
--- a/drivers/clk/imx/clk-imx8mq.c
+++ b/drivers/clk/imx/clk-imx8mq.c
@@ -611,9 +611,10 @@  MODULE_DEVICE_TABLE(of, imx8mq_clk_of_match);
 
 static struct platform_driver imx8mq_clk_driver = {
 	.probe = imx8mq_clocks_probe,
 	.driver = {
 		.name = "imx8mq-ccm",
+		.suppress_bind_attrs = true,
 		.of_match_table = of_match_ptr(imx8mq_clk_of_match),
 	},
 };
 module_platform_driver(imx8mq_clk_driver);