| Message ID | 9f2ac65bab203a943de947465d6534980585e144.1574116045.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | clk: imx8m: Suppress bind attrs | expand |
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
> 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
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
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
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);
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(+)