| Message ID | 20190107173132.27316-1-robh@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fbdev: offb: Fix OF node name handling | expand |
On Mon, Jan 7, 2019 at 6:31 PM Rob Herring <robh@kernel.org> 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+ (malat@debian.org) (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 <malat@debian.org> Thanks > Reported-by: Mathieu Malaterre <malat@debian.org> > Cc: stable@vger.kernel.org # v4.19+ > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > Cc: dri-devel@lists.freedesktop.org > Cc: linux-fbdev@vger.kernel.org > Signed-off-by: Rob Herring <robh@kernel.org> > --- > 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 >
Hi Rob, On Mon, Jan 7, 2019 at 9:11 PM Mathieu Malaterre <malat@debian.org> wrote: > > On Mon, Jan 7, 2019 at 6:31 PM Rob Herring <robh@kernel.org> 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+ (malat@debian.org) (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 <malat@debian.org> Just for curiosity, why would you keep: if (strcmp(dp->name, "valkyrie") == 0) > > Reported-by: Mathieu Malaterre <malat@debian.org> > > Cc: stable@vger.kernel.org # v4.19+ > > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > Cc: dri-devel@lists.freedesktop.org > > Cc: linux-fbdev@vger.kernel.org > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > 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 > >
On Mon, Jan 7, 2019 at 2:36 PM Mathieu Malaterre <malat@debian.org> wrote: > > Hi Rob, > > On Mon, Jan 7, 2019 at 9:11 PM Mathieu Malaterre <malat@debian.org> wrote: > > > > On Mon, Jan 7, 2019 at 6:31 PM Rob Herring <robh@kernel.org> 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+ (malat@debian.org) (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 <malat@debian.org> > > 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
On 01/07/2019 09:11 PM, Mathieu Malaterre wrote: > On Mon, Jan 7, 2019 at 6:31 PM Rob Herring <robh@kernel.org> 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+ (malat@debian.org) (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 <malat@debian.org> Patch queued for 5.0, thanks. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics
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;
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 <malat@debian.org> Cc: stable@vger.kernel.org # v4.19+ Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> Cc: dri-devel@lists.freedesktop.org Cc: linux-fbdev@vger.kernel.org Signed-off-by: Rob Herring <robh@kernel.org> --- drivers/video/fbdev/offb.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)