linux-kernel - Re: [PATCH v1 6/7] auxdisplay: hd44780: Call charlcd_alloc() from hd44780_common

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdXP1=7YJzYp=_WJsqx2mtBYcwAjpOGK2_9SH+r4w6v2Ug@mail.gmail.com>
Date: Fri, 7 Mar 2025 10:14:48 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: linux-kernel@...r.kernel.org, Andy Shevchenko <andy@...nel.org>, 
	Willy Tarreau <willy@...roxy.com>, Ksenija Stanojevic <ksenija.stanojevic@...il.com>
Subject: Re: [PATCH v1 6/7] auxdisplay: hd44780: Call charlcd_alloc() from hd44780_common_alloc()

Hi Andy,

Thanks for your patch!

On Mon, 24 Feb 2025 at 18:30, Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
> HD44780 APIs are operate on struct charlcd object. Moreover, the current users

s/are/all/
s/object/objects/

> always call charlcd_alloc() and hd44780_common_alloc(). Make the latter call
> the former, so eliminate the additional allocation, make it consistent with

either s/make/making/, or s/make/to make/

> the rest of API and avoid duplication.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

As the code looks correct to me:
Reviewed-by: Geert Uytterhoeven <geert@...ux-m68k.org>

> --- a/drivers/auxdisplay/hd44780_common.c
> +++ b/drivers/auxdisplay/hd44780_common.c
> @@ -351,24 +351,26 @@ int hd44780_common_redefine_char(struct charlcd *lcd, char *esc)
>  }
>  EXPORT_SYMBOL_GPL(hd44780_common_redefine_char);
>
> -struct hd44780_common *hd44780_common_alloc(void)
> +struct charlcd *hd44780_common_alloc(void)
>  {
>         struct hd44780_common *hd;
> +       struct charlcd *lcd;
>
> -       hd = kzalloc(sizeof(*hd), GFP_KERNEL);
> -       if (!hd)
> +       lcd = charlcd_alloc(sizeof(*hd));
> +       if (!lcd)
>                 return NULL;
>
> +       hd = lcd->drvdata;
>         hd->ifwidth = 8;
>         hd->bwidth = DEFAULT_LCD_BWIDTH;
>         hd->hwidth = DEFAULT_LCD_HWIDTH;
> -       return hd;
> +       return lcd;
>  }
>  EXPORT_SYMBOL_GPL(hd44780_common_alloc);

While I like the general idea, there are two things in the API I do
not like:
  1. The function is called "hd44780_common_alloc()", but returns
     a pointer to a different struct type than the name suggests,
  2. The real "struct hd44780_common" must be obtained by the caller
     from charlcd.drvdata, which is of type "void *", i.e. unsafe.

What about changing it to e.g.?

    struct hd44780_common *hd44780_common_alloc(struct charlcd **lcd)

so you can return pointers to both structs?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds