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

[Mathieu Malaterre]

Hi Rob,

On Mon, Jan 7, 2019 at 9:11 PM Mathieu Malaterre <malat@xxxxxxxxxx> wrote:
>
> On Mon, Jan 7, 2019 at 6:31 PM Rob Herring <robh@xxxxxxxxxx> 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@xxxxxxxxxx) (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@xxxxxxxxxx>

Just for curiosity, why would you keep:

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



> > Reported-by: Mathieu Malaterre <malat@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx # v4.19+
> > Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > Cc: linux-fbdev@xxxxxxxxxxxxxxx
> > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> > ---
> >  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
> >