2019-01-07 18:11:24

by Rob Herring (Arm)

Subject: [PATCH] fbdev: offb: Fix OF node name handling

Commit 5c63e407aaab ("fbdev: Convert to using %pOFn instead of
device_node.name") changed how the OF FB driver handles the OF node
name. This missed the case where the node name is passed to
offb_init_palette_hacks(). This results in a NULL ptr dereference
in strncmp and breaks any system except ones using bootx with no display
node.

Fix this by making offb_init_palette_hacks() use the OF node pointer and
use of_node_name_prefix() helper function instead for node name
comparisons. This helps in moving all OF node name accesses to helper
functions in preparation to remove struct device_node.name pointer.

Fixes: 5c63e407aaab ("fbdev: Convert to using %pOFn instead of device_node.name")

Reported-by: Mathieu Malaterre <[email protected]>
Cc: [email protected] # v4.19+
Cc: Bartlomiej Zolnierkiewicz <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Rob Herring <[email protected]>
---
drivers/video/fbdev/offb.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/video/fbdev/offb.c b/drivers/video/fbdev/offb.c
index 31f769d67195..057d3cdef92e 100644
--- a/drivers/video/fbdev/offb.c
+++ b/drivers/video/fbdev/offb.c
@@ -318,28 +318,28 @@ static void __iomem *offb_map_reg(struct device_node *np, int index,
}

static void offb_init_palette_hacks(struct fb_info *info, struct device_node *dp,
- const char *name, unsigned long address)
+ unsigned long address)
{
struct offb_par *par = (struct offb_par *) info->par;

- if (dp && !strncmp(name, "ATY,Rage128", 11)) {
+ if (of_node_name_prefix(dp, "ATY,Rage128")) {
par->cmap_adr = offb_map_reg(dp, 2, 0, 0x1fff);
if (par->cmap_adr)
par->cmap_type = cmap_r128;
- } else if (dp && (!strncmp(name, "ATY,RageM3pA", 12)
- || !strncmp(name, "ATY,RageM3p12A", 14))) {
+ } else if (of_node_name_prefix(dp, "ATY,RageM3pA") ||
+ of_node_name_prefix(dp, "ATY,RageM3p12A")) {
par->cmap_adr = offb_map_reg(dp, 2, 0, 0x1fff);
if (par->cmap_adr)
par->cmap_type = cmap_M3A;
- } else if (dp && !strncmp(name, "ATY,RageM3pB", 12)) {
+ } else if (of_node_name_prefix(dp, "ATY,RageM3pB")) {
par->cmap_adr = offb_map_reg(dp, 2, 0, 0x1fff);
if (par->cmap_adr)
par->cmap_type = cmap_M3B;
- } else if (dp && !strncmp(name, "ATY,Rage6", 9)) {
+ } else if (of_node_name_prefix(dp, "ATY,Rage6")) {
par->cmap_adr = offb_map_reg(dp, 1, 0, 0x1fff);
if (par->cmap_adr)
par->cmap_type = cmap_radeon;
- } else if (!strncmp(name, "ATY,", 4)) {
+ } else if (of_node_name_prefix(dp, "ATY,")) {
unsigned long base = address & 0xff000000UL;
par->cmap_adr =
ioremap(base + 0x7ff000, 0x1000) + 0xcc0;
@@ -350,7 +350,7 @@ static void offb_init_palette_hacks(struct fb_info *info, struct device_node *dp
par->cmap_adr = offb_map_reg(dp, 0, 0x6000, 0x1000);
if (par->cmap_adr)
par->cmap_type = cmap_gxt2000;
- } else if (dp && !strncmp(name, "vga,Display-", 12)) {
+ } else if (of_node_name_prefix(dp, "vga,Display-")) {
/* Look for AVIVO initialized by SLOF */
struct device_node *pciparent = of_get_parent(dp);
const u32 *vid, *did;
@@ -438,7 +438,7 @@ static void __init offb_init_fb(const char *name,

par->cmap_type = cmap_unknown;
if (depth == 8)
- offb_init_palette_hacks(info, dp, name, address);
+ offb_init_palette_hacks(info, dp, address);
else
fix->visual = FB_VISUAL_TRUECOLOR;

--
2.19.1

2019-01-07 20:15:37

by Mathieu Malaterre

Subject: Re: [PATCH] fbdev: offb: Fix OF node name handling

On Mon, Jan 7, 2019 at 6:31 PM Rob Herring <[email protected]> wrote:
>
> Commit 5c63e407aaab ("fbdev: Convert to using %pOFn instead of
> device_node.name") changed how the OF FB driver handles the OF node
> name. This missed the case where the node name is passed to
> offb_init_palette_hacks(). This results in a NULL ptr dereference
> in strncmp and breaks any system except ones using bootx with no display
> node.
>
> Fix this by making offb_init_palette_hacks() use the OF node pointer and
> use of_node_name_prefix() helper function instead for node name
> comparisons. This helps in moving all OF node name accesses to helper
> functions in preparation to remove struct device_node.name pointer.
>
> Fixes: 5c63e407aaab ("fbdev: Convert to using %pOFn instead of device_node.name")

Looks good to me:

...
[ 0.000000] Linux version 5.0.0-rc1+ ([email protected]) (gcc
version 6.3.0 20170516 (Debian 6.3.0-18)) #21 Mon Jan 7 21:03:53 CET
2019
...

So here is my :

Tested-by: Mathieu Malaterre <[email protected]>

Thanks

> Reported-by: Mathieu Malaterre <[email protected]>
> Cc: [email protected] # v4.19+
> Cc: Bartlomiej Zolnierkiewicz <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Rob Herring <[email protected]>
> ---
> drivers/video/fbdev/offb.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/video/fbdev/offb.c b/drivers/video/fbdev/offb.c
> index 31f769d67195..057d3cdef92e 100644
> --- a/drivers/video/fbdev/offb.c
> +++ b/drivers/video/fbdev/offb.c
> @@ -318,28 +318,28 @@ static void __iomem *offb_map_reg(struct device_node *np, int index,
> }
>
> static void offb_init_palette_hacks(struct fb_info *info, struct device_node *dp,
> - const char *name, unsigned long address)
> + unsigned long address)
> {
> struct offb_par *par = (struct offb_par *) info->par;
>
> - if (dp && !strncmp(name, "ATY,Rage128", 11)) {
> + if (of_node_name_prefix(dp, "ATY,Rage128")) {
> par->cmap_adr = offb_map_reg(dp, 2, 0, 0x1fff);
> if (par->cmap_adr)
> par->cmap_type = cmap_r128;
> - } else if (dp && (!strncmp(name, "ATY,RageM3pA", 12)
> - || !strncmp(name, "ATY,RageM3p12A", 14))) {
> + } else if (of_node_name_prefix(dp, "ATY,RageM3pA") ||
> + of_node_name_prefix(dp, "ATY,RageM3p12A")) {
> par->cmap_adr = offb_map_reg(dp, 2, 0, 0x1fff);
> if (par->cmap_adr)
> par->cmap_type = cmap_M3A;
> - } else if (dp && !strncmp(name, "ATY,RageM3pB", 12)) {
> + } else if (of_node_name_prefix(dp, "ATY,RageM3pB")) {
> par->cmap_adr = offb_map_reg(dp, 2, 0, 0x1fff);
> if (par->cmap_adr)
> par->cmap_type = cmap_M3B;
> - } else if (dp && !strncmp(name, "ATY,Rage6", 9)) {
> + } else if (of_node_name_prefix(dp, "ATY,Rage6")) {
> par->cmap_adr = offb_map_reg(dp, 1, 0, 0x1fff);
> if (par->cmap_adr)
> par->cmap_type = cmap_radeon;
> - } else if (!strncmp(name, "ATY,", 4)) {
> + } else if (of_node_name_prefix(dp, "ATY,")) {
> unsigned long base = address & 0xff000000UL;
> par->cmap_adr =
> ioremap(base + 0x7ff000, 0x1000) + 0xcc0;
> @@ -350,7 +350,7 @@ static void offb_init_palette_hacks(struct fb_info *info, struct device_node *dp
> par->cmap_adr = offb_map_reg(dp, 0, 0x6000, 0x1000);
> if (par->cmap_adr)
> par->cmap_type = cmap_gxt2000;
> - } else if (dp && !strncmp(name, "vga,Display-", 12)) {
> + } else if (of_node_name_prefix(dp, "vga,Display-")) {
> /* Look for AVIVO initialized by SLOF */
> struct device_node *pciparent = of_get_parent(dp);
> const u32 *vid, *did;
> @@ -438,7 +438,7 @@ static void __init offb_init_fb(const char *name,
>
> par->cmap_type = cmap_unknown;
> if (depth == 8)
> - offb_init_palette_hacks(info, dp, name, address);
> + offb_init_palette_hacks(info, dp, address);
> else
> fix->visual = FB_VISUAL_TRUECOLOR;
>
> --
> 2.19.1
>

2019-01-07 21:11:04

by Mathieu Malaterre

Subject: Re: [PATCH] fbdev: offb: Fix OF node name handling

Hi Rob,

On Mon, Jan 7, 2019 at 9:11 PM Mathieu Malaterre <[email protected]> wrote:
>
> On Mon, Jan 7, 2019 at 6:31 PM Rob Herring <[email protected]> wrote:
> >
> > Commit 5c63e407aaab ("fbdev: Convert to using %pOFn instead of
> > device_node.name") changed how the OF FB driver handles the OF node
> > name. This missed the case where the node name is passed to
> > offb_init_palette_hacks(). This results in a NULL ptr dereference
> > in strncmp and breaks any system except ones using bootx with no display
> > node.
> >
> > Fix this by making offb_init_palette_hacks() use the OF node pointer and
> > use of_node_name_prefix() helper function instead for node name
> > comparisons. This helps in moving all OF node name accesses to helper
> > functions in preparation to remove struct device_node.name pointer.
> >
> > Fixes: 5c63e407aaab ("fbdev: Convert to using %pOFn instead of device_node.name")
>
> Looks good to me:
>
> ...
> [ 0.000000] Linux version 5.0.0-rc1+ ([email protected]) (gcc
> version 6.3.0 20170516 (Debian 6.3.0-18)) #21 Mon Jan 7 21:03:53 CET
> 2019
> ...
>
> So here is my :
>
> Tested-by: Mathieu Malaterre <[email protected]>

Just for curiosity, why would you keep:

if (strcmp(dp->name, "valkyrie") == 0)



> > Reported-by: Mathieu Malaterre <[email protected]>
> > Cc: [email protected] # v4.19+
> > Cc: Bartlomiej Zolnierkiewicz <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Rob Herring <[email protected]>
> > ---
> > drivers/video/fbdev/offb.c | 18 +++++++++---------
> > 1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/offb.c b/drivers/video/fbdev/offb.c
> > index 31f769d67195..057d3cdef92e 100644
> > --- a/drivers/video/fbdev/offb.c
> > +++ b/drivers/video/fbdev/offb.c
> > @@ -318,28 +318,28 @@ static void __iomem *offb_map_reg(struct device_node *np, int index,
> > }
> >
> > static void offb_init_palette_hacks(struct fb_info *info, struct device_node *dp,
> > - const char *name, unsigned long address)
> > + unsigned long address)
> > {
> > struct offb_par *par = (struct offb_par *) info->par;
> >
> > - if (dp && !strncmp(name, "ATY,Rage128", 11)) {
> > + if (of_node_name_prefix(dp, "ATY,Rage128")) {
> > par->cmap_adr = offb_map_reg(dp, 2, 0, 0x1fff);
> > if (par->cmap_adr)
> > par->cmap_type = cmap_r128;
> > - } else if (dp && (!strncmp(name, "ATY,RageM3pA", 12)
> > - || !strncmp(name, "ATY,RageM3p12A", 14))) {
> > + } else if (of_node_name_prefix(dp, "ATY,RageM3pA") ||
> > + of_node_name_prefix(dp, "ATY,RageM3p12A")) {
> > par->cmap_adr = offb_map_reg(dp, 2, 0, 0x1fff);
> > if (par->cmap_adr)
> > par->cmap_type = cmap_M3A;
> > - } else if (dp && !strncmp(name, "ATY,RageM3pB", 12)) {
> > + } else if (of_node_name_prefix(dp, "ATY,RageM3pB")) {
> > par->cmap_adr = offb_map_reg(dp, 2, 0, 0x1fff);
> > if (par->cmap_adr)
> > par->cmap_type = cmap_M3B;
> > - } else if (dp && !strncmp(name, "ATY,Rage6", 9)) {
> > + } else if (of_node_name_prefix(dp, "ATY,Rage6")) {
> > par->cmap_adr = offb_map_reg(dp, 1, 0, 0x1fff);
> > if (par->cmap_adr)
> > par->cmap_type = cmap_radeon;
> > - } else if (!strncmp(name, "ATY,", 4)) {
> > + } else if (of_node_name_prefix(dp, "ATY,")) {
> > unsigned long base = address & 0xff000000UL;
> > par->cmap_adr =
> > ioremap(base + 0x7ff000, 0x1000) + 0xcc0;
> > @@ -350,7 +350,7 @@ static void offb_init_palette_hacks(struct fb_info *info, struct device_node *dp
> > par->cmap_adr = offb_map_reg(dp, 0, 0x6000, 0x1000);
> > if (par->cmap_adr)
> > par->cmap_type = cmap_gxt2000;
> > - } else if (dp && !strncmp(name, "vga,Display-", 12)) {
> > + } else if (of_node_name_prefix(dp, "vga,Display-")) {
> > /* Look for AVIVO initialized by SLOF */
> > struct device_node *pciparent = of_get_parent(dp);
> > const u32 *vid, *did;
> > @@ -438,7 +438,7 @@ static void __init offb_init_fb(const char *name,
> >
> > par->cmap_type = cmap_unknown;
> > if (depth == 8)
> > - offb_init_palette_hacks(info, dp, name, address);
> > + offb_init_palette_hacks(info, dp, address);
> > else
> > fix->visual = FB_VISUAL_TRUECOLOR;
> >
> > --
> > 2.19.1
> >

2019-01-07 23:53:03

by Rob Herring (Arm)

Subject: Re: [PATCH] fbdev: offb: Fix OF node name handling

On Mon, Jan 7, 2019 at 2:36 PM Mathieu Malaterre <[email protected]> wrote:
>
> Hi Rob,
>
> On Mon, Jan 7, 2019 at 9:11 PM Mathieu Malaterre <[email protected]> wrote:
> >
> > On Mon, Jan 7, 2019 at 6:31 PM Rob Herring <[email protected]> wrote:
> > >
> > > Commit 5c63e407aaab ("fbdev: Convert to using %pOFn instead of
> > > device_node.name") changed how the OF FB driver handles the OF node
> > > name. This missed the case where the node name is passed to
> > > offb_init_palette_hacks(). This results in a NULL ptr dereference
> > > in strncmp and breaks any system except ones using bootx with no display
> > > node.
> > >
> > > Fix this by making offb_init_palette_hacks() use the OF node pointer and
> > > use of_node_name_prefix() helper function instead for node name
> > > comparisons. This helps in moving all OF node name accesses to helper
> > > functions in preparation to remove struct device_node.name pointer.
> > >
> > > Fixes: 5c63e407aaab ("fbdev: Convert to using %pOFn instead of device_node.name")
> >
> > Looks good to me:
> >
> > ...
> > [ 0.000000] Linux version 5.0.0-rc1+ ([email protected]) (gcc
> > version 6.3.0 20170516 (Debian 6.3.0-18)) #21 Mon Jan 7 21:03:53 CET
> > 2019
> > ...
> >
> > So here is my :
> >
> > Tested-by: Mathieu Malaterre <[email protected]>
>
> Just for curiosity, why would you keep:
>
> if (strcmp(dp->name, "valkyrie") == 0)

Mainly just because I already have another patch to do that which I
haven't sent out.

Rob

2019-01-11 14:19:54

by Bartlomiej Zolnierkiewicz

Subject: Re: [PATCH] fbdev: offb: Fix OF node name handling

On 01/07/2019 09:11 PM, Mathieu Malaterre wrote:
> On Mon, Jan 7, 2019 at 6:31 PM Rob Herring <[email protected]> wrote:
>>
>> Commit 5c63e407aaab ("fbdev: Convert to using %pOFn instead of
>> device_node.name") changed how the OF FB driver handles the OF node
>> name. This missed the case where the node name is passed to
>> offb_init_palette_hacks(). This results in a NULL ptr dereference
>> in strncmp and breaks any system except ones using bootx with no display
>> node.
>>
>> Fix this by making offb_init_palette_hacks() use the OF node pointer and
>> use of_node_name_prefix() helper function instead for node name
>> comparisons. This helps in moving all OF node name accesses to helper
>> functions in preparation to remove struct device_node.name pointer.
>>
>> Fixes: 5c63e407aaab ("fbdev: Convert to using %pOFn instead of device_node.name")
>
> Looks good to me:
>
> ...
> [ 0.000000] Linux version 5.0.0-rc1+ ([email protected]) (gcc
> version 6.3.0 20170516 (Debian 6.3.0-18)) #21 Mon Jan 7 21:03:53 CET
> 2019
> ...
>
> So here is my :
>
> Tested-by: Mathieu Malaterre <[email protected]>

Patch queued for 5.0, thanks.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics