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: <Z8soDV0U2LG2KX9J@smile.fi.intel.com>
Date: Fri, 7 Mar 2025 19:08:29 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
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()

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()?

-- 
With Best Regards,
Andy Shevchenko