lkml.org 
[lkml]   [2021]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 1/3] of: Move simple-framebuffer device handling from simplefb to of
From
Date
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.

--
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

\
 
 \ /
  Last update: 2021-12-13 11:47    [W:0.074 / U:0.440 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site