| Message ID | 20211212062407.138309-2-marcan@marcan.st |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | drm/simpledrm: Apple M1 / DT platform support fixes | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dtbs-check | success | |
| robh/dt-meta-schema | success |
On Sun, Dec 12, 2021 at 12:24 AM Hector Martin <marcan@marcan.st> wrote: > > This code is required for both simplefb and simpledrm, so let's move it > into the OF core instead of having it as an ad-hoc initcall in the > drivers. > > Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/of/platform.c | 4 ++++ > drivers/video/fbdev/simplefb.c | 21 +-------------------- > 2 files changed, 5 insertions(+), 20 deletions(-) Reviewed-by: Rob Herring <robh@kernel.org>
Hi Am 12.12.21 um 22:29 schrieb Rob Herring: > On Sun, Dec 12, 2021 at 12:24 AM Hector Martin <marcan@marcan.st> wrote: >> >> This code is required for both simplefb and simpledrm, so let's move it >> into the OF core instead of having it as an ad-hoc initcall in the >> drivers. >> >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> drivers/of/platform.c | 4 ++++ >> drivers/video/fbdev/simplefb.c | 21 +-------------------- >> 2 files changed, 5 insertions(+), 20 deletions(-) > > Reviewed-by: Rob Herring <robh@kernel.org> > Can I merge this patch through DRM trees? Best regards Thomas
Hello Hector, On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <marcan@marcan.st> wrote: > > This code is required for both simplefb and simpledrm, so let's move it > into the OF core instead of having it as an ad-hoc initcall in the > drivers. > > Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/of/platform.c | 4 ++++ > drivers/video/fbdev/simplefb.c | 21 +-------------------- > 2 files changed, 5 insertions(+), 20 deletions(-) > This is indeed a much better approach than what I suggested. I just have one comment. > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index b3faf89744aa..793350028906 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void) > of_node_put(node); > } > > + node = of_get_compatible_child(of_chosen, "simple-framebuffer"); You have to check if the node variable is NULL here. > + of_platform_device_create(node, NULL, NULL); Otherwise this could lead to a NULL pointer dereference if debug output is enabled (the node->full_name is printed). Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Best regards, Javier
On 13/12/2021 17.44, Javier Martinez Canillas wrote: > Hello Hector, > > On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <marcan@marcan.st> wrote: >> >> This code is required for both simplefb and simpledrm, so let's move it >> into the OF core instead of having it as an ad-hoc initcall in the >> drivers. >> >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> drivers/of/platform.c | 4 ++++ >> drivers/video/fbdev/simplefb.c | 21 +-------------------- >> 2 files changed, 5 insertions(+), 20 deletions(-) >> > > This is indeed a much better approach than what I suggested. I just > have one comment. > >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c >> index b3faf89744aa..793350028906 100644 >> --- a/drivers/of/platform.c >> +++ b/drivers/of/platform.c >> @@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void) >> of_node_put(node); >> } >> >> + node = of_get_compatible_child(of_chosen, "simple-framebuffer"); > > You have to check if the node variable is NULL here. > >> + of_platform_device_create(node, NULL, NULL); > > Otherwise this could lead to a NULL pointer dereference if debug > output is enabled (the node->full_name is printed). Where is it printed? I thought I might need a NULL check, but this code was suggested verbatim by Rob in v2 without the NULL check and digging through I found that the NULL codepath is safe. of_platform_device_create calls of_platform_device_create_pdata directly, and: static struct platform_device *of_platform_device_create_pdata( struct device_node *np, const char *bus_id, void *platform_data, struct device *parent) { struct platform_device *dev; if (!of_device_is_available(np) || of_node_test_and_set_flag(np, OF_POPULATED)) return NULL; of_device_is_available takes a global spinlock and then calls __of_device_is_available, and that does: static bool __of_device_is_available(const struct device_node *device) { const char *status; int statlen; if (!device) return false; ... so I don't see how this can do anything but immediately return false if node is NULL.
On Mon, Dec 13, 2021 at 11:46 AM Hector Martin <marcan@marcan.st> wrote: > > On 13/12/2021 17.44, Javier Martinez Canillas wrote: > > Hello Hector, > > > > On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <marcan@marcan.st> wrote: > >> > >> This code is required for both simplefb and simpledrm, so let's move it > >> into the OF core instead of having it as an ad-hoc initcall in the > >> drivers. > >> > >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > >> Signed-off-by: Hector Martin <marcan@marcan.st> > >> --- > >> drivers/of/platform.c | 4 ++++ > >> drivers/video/fbdev/simplefb.c | 21 +-------------------- > >> 2 files changed, 5 insertions(+), 20 deletions(-) > >> > > > > This is indeed a much better approach than what I suggested. I just > > have one comment. > > > >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c > >> index b3faf89744aa..793350028906 100644 > >> --- a/drivers/of/platform.c > >> +++ b/drivers/of/platform.c > >> @@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void) > >> of_node_put(node); > >> } > >> > >> + node = of_get_compatible_child(of_chosen, "simple-framebuffer"); > > > > You have to check if the node variable is NULL here. > > > >> + of_platform_device_create(node, NULL, NULL); > > > > Otherwise this could lead to a NULL pointer dereference if debug > > output is enabled (the node->full_name is printed). > > Where is it printed? I thought I might need a NULL check, but this code Sorry, I misread of_amba_device_create() as of_platform_device_create(), which uses the "%pOF" printk format specifier [0] to print the node's full name as a debug output [1]. [0]: https://elixir.bootlin.com/linux/v5.16-rc5/source/Documentation/core-api/printk-formats.rst#L462 [1]: https://elixir.bootlin.com/linux/v5.16-rc5/source/drivers/of/platform.c#L233 > was suggested verbatim by Rob in v2 without the NULL check and digging > through I found that the NULL codepath is safe. > You are right that passing NULL is a safe code path for now due the of_device_is_available(node) check, but that seems fragile to me since just adding a similar debug output to of_platform_device_create() could trigger the NULL pointer dereference. Best regards, Javier
On 13/12/2021 20.30, Javier Martinez Canillas wrote: > On Mon, Dec 13, 2021 at 11:46 AM Hector Martin <marcan@marcan.st> wrote: >> >> On 13/12/2021 17.44, Javier Martinez Canillas wrote: >>> Hello Hector, >>> >>> On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <marcan@marcan.st> wrote: >>>> >>>> This code is required for both simplefb and simpledrm, so let's move it >>>> into the OF core instead of having it as an ad-hoc initcall in the >>>> drivers. >>>> >>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> >>>> Signed-off-by: Hector Martin <marcan@marcan.st> >>>> --- >>>> drivers/of/platform.c | 4 ++++ >>>> drivers/video/fbdev/simplefb.c | 21 +-------------------- >>>> 2 files changed, 5 insertions(+), 20 deletions(-) >>>> >>> >>> This is indeed a much better approach than what I suggested. I just >>> have one comment. >>> >>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c >>>> index b3faf89744aa..793350028906 100644 >>>> --- a/drivers/of/platform.c >>>> +++ b/drivers/of/platform.c >>>> @@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void) >>>> of_node_put(node); >>>> } >>>> >>>> + node = of_get_compatible_child(of_chosen, "simple-framebuffer"); >>> >>> You have to check if the node variable is NULL here. >>> >>>> + of_platform_device_create(node, NULL, NULL); >>> >>> Otherwise this could lead to a NULL pointer dereference if debug >>> output is enabled (the node->full_name is printed). >> >> Where is it printed? I thought I might need a NULL check, but this code > > Sorry, I misread of_amba_device_create() as > of_platform_device_create(), which uses the "%pOF" printk format > specifier [0] to print the node's full name as a debug output [1]. > > [0]: https://elixir.bootlin.com/linux/v5.16-rc5/source/Documentation/core-api/printk-formats.rst#L462 > [1]: https://elixir.bootlin.com/linux/v5.16-rc5/source/drivers/of/platform.c#L233 > >> was suggested verbatim by Rob in v2 without the NULL check and digging >> through I found that the NULL codepath is safe. >> > > You are right that passing NULL is a safe code path for now due the > of_device_is_available(node) check, but that seems fragile to me since > just adding a similar debug output to of_platform_device_create() > could trigger the NULL pointer dereference. Since Rob is the DT maintainer, I'm going to defer to his opinion on this one :-)
On Mon, Dec 13, 2021 at 5:30 AM Javier Martinez Canillas <javier@dowhile0.org> wrote: > > On Mon, Dec 13, 2021 at 11:46 AM Hector Martin <marcan@marcan.st> wrote: > > > > On 13/12/2021 17.44, Javier Martinez Canillas wrote: > > > Hello Hector, > > > > > > On Sun, Dec 12, 2021 at 7:24 AM Hector Martin <marcan@marcan.st> wrote: > > >> > > >> This code is required for both simplefb and simpledrm, so let's move it > > >> into the OF core instead of having it as an ad-hoc initcall in the > > >> drivers. > > >> > > >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > > >> Signed-off-by: Hector Martin <marcan@marcan.st> > > >> --- > > >> drivers/of/platform.c | 4 ++++ > > >> drivers/video/fbdev/simplefb.c | 21 +-------------------- > > >> 2 files changed, 5 insertions(+), 20 deletions(-) > > >> > > > > > > This is indeed a much better approach than what I suggested. I just > > > have one comment. > > > > > >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c > > >> index b3faf89744aa..793350028906 100644 > > >> --- a/drivers/of/platform.c > > >> +++ b/drivers/of/platform.c > > >> @@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void) > > >> of_node_put(node); > > >> } > > >> > > >> + node = of_get_compatible_child(of_chosen, "simple-framebuffer"); > > > > > > You have to check if the node variable is NULL here. > > > > > >> + of_platform_device_create(node, NULL, NULL); > > > > > > Otherwise this could lead to a NULL pointer dereference if debug > > > output is enabled (the node->full_name is printed). > > > > Where is it printed? I thought I might need a NULL check, but this code > > Sorry, I misread of_amba_device_create() as > of_platform_device_create(), which uses the "%pOF" printk format > specifier [0] to print the node's full name as a debug output [1]. > > [0]: https://elixir.bootlin.com/linux/v5.16-rc5/source/Documentation/core-api/printk-formats.rst#L462 > [1]: https://elixir.bootlin.com/linux/v5.16-rc5/source/drivers/of/platform.c#L233 > > > was suggested verbatim by Rob in v2 without the NULL check and digging > > through I found that the NULL codepath is safe. > > > > You are right that passing NULL is a safe code path for now due the > of_device_is_available(node) check, but that seems fragile to me since > just adding a similar debug output to of_platform_device_create() > could trigger the NULL pointer dereference. All/most DT functions work with a NULL node ptr, so why should this one be different? Rob
On Mon, Dec 13, 2021 at 2:16 AM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 12.12.21 um 22:29 schrieb Rob Herring: > > On Sun, Dec 12, 2021 at 12:24 AM Hector Martin <marcan@marcan.st> wrote: > >> > >> This code is required for both simplefb and simpledrm, so let's move it > >> into the OF core instead of having it as an ad-hoc initcall in the > >> drivers. > >> > >> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > >> Signed-off-by: Hector Martin <marcan@marcan.st> > >> --- > >> drivers/of/platform.c | 4 ++++ > >> drivers/video/fbdev/simplefb.c | 21 +-------------------- > >> 2 files changed, 5 insertions(+), 20 deletions(-) > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > > Can I merge this patch through DRM trees? Yes. Rob
On Mon, Dec 13, 2021 at 3:50 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Mon, Dec 13, 2021 at 5:30 AM Javier Martinez Canillas > <javier@dowhile0.org> wrote: [snip] > > > > You are right that passing NULL is a safe code path for now due the > > of_device_is_available(node) check, but that seems fragile to me since > > just adding a similar debug output to of_platform_device_create() > > could trigger the NULL pointer dereference. > > All/most DT functions work with a NULL node ptr, so why should this > one be different? > If you are OK with the patch as is, then I won't object :) > Rob Best regards, Javier
diff --git a/drivers/of/platform.c b/drivers/of/platform.c index b3faf89744aa..793350028906 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -540,6 +540,10 @@ static int __init of_platform_default_populate_init(void) of_node_put(node); } + node = of_get_compatible_child(of_chosen, "simple-framebuffer"); + of_platform_device_create(node, NULL, NULL); + of_node_put(node); + /* Populate everything else. */ of_platform_default_populate(NULL, NULL, NULL); diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c index b63074fd892e..57541887188b 100644 --- a/drivers/video/fbdev/simplefb.c +++ b/drivers/video/fbdev/simplefb.c @@ -541,26 +541,7 @@ static struct platform_driver simplefb_driver = { .remove = simplefb_remove, }; -static int __init simplefb_init(void) -{ - int ret; - struct device_node *np; - - ret = platform_driver_register(&simplefb_driver); - if (ret) - return ret; - - if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) { - for_each_child_of_node(of_chosen, np) { - if (of_device_is_compatible(np, "simple-framebuffer")) - of_platform_device_create(np, NULL, NULL); - } - } - - return 0; -} - -fs_initcall(simplefb_init); +module_platform_driver(simplefb_driver); MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>"); MODULE_DESCRIPTION("Simple framebuffer driver");