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: <CAMuHMdV1+ftjpEcg4xYjBLH1BRJHkZcYB5W+p8WUWWLXT3DnUQ@mail.gmail.com>
Date: Fri, 7 Mar 2025 19:19:57 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: linux-kernel@...r.kernel.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,

On Fri, 7 Mar 2025 at 18:08, Andy Shevchenko
<andriy.shevchenko@...ux.intel.com> wrote:
> On Fri, Mar 07, 2025 at 10:14:48AM +0100, Geert Uytterhoeven wrote:
> > 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>
>
> Thanks I have corrected all grammar issues in the commit messages except one in
> the first patch which I do not understand.
>
> ...
>
> > 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?
>
> I don't like this prototype as it seems and feels confusing. Also note,
> the APIs are using struct charlcd while being in the hd44780 namespace.
> perhaps better to rename the function to hd44780_common_and_lcd_alloc()?

That is one option.

Another option would be to add a "charlcd *lcd" member to
struct hd44780_common.

That would allow to fix the other odd part in the API:

    -void hd44780_common_free(struct charlcd *lcd)
    +void hd44780_common_free(struct hd4480_common *hd)


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