linux-kernel - Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct drm_*_helper

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]

Date:   Thu, 21 Sep 2023 10:12:24 +0200
From:   Maxime Ripard <mripard@...nel.org>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Javier Martinez Canillas <javierm@...hat.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm/ssd130x: Drop _helper prefix from struct
 drm_*_helper_funcs callbacks

On Thu, Sep 21, 2023 at 09:57:22AM +0200, Geert Uytterhoeven wrote:
> Hi Maxime,
> 
> On Thu, Sep 21, 2023 at 9:44 AM Maxime Ripard <mripard@...nel.org> wrote:
> > On Mon, Sep 18, 2023 at 09:19:07AM +0200, Javier Martinez Canillas wrote:
> > > Thomas Zimmermann <tzimmermann@...e.de> writes:
> > > > Am 14.09.23 um 21:51 schrieb Javier Martinez Canillas:
> > > >> The driver uses a naming convention where functions for struct drm_*_funcs
> > > >> callbacks are named ssd130x_$object_$operation, while the callbacks for
> > > >> struct drm_*_helper_funcs are named ssd130x_$object_helper_$operation.
> > > >>
> > > >> The idea is that this helper_ prefix in the function names denote that are
> > > >> for struct drm_*_helper_funcs callbacks. This convention was copied from
> > > >> other drivers, when ssd130x was written but Maxime pointed out that is the
> > > >> exception rather than the norm.
> > > >
> > > > I guess you found this in my code. I want to point out that I use the
> > > > _helper infix to signal that these are callback for
> > > > drm_primary_plane_helper_funcs and *not* drm_primary_plane_funcs. The
> > > > naming is intentional.
> > >
> > > Yes, that's what tried to say in the commit message and indeed I got the
> > > convention from drivers in drivers/gpu/drm/tiny. In fact I believe these
> > > function names are since first iteration of the driver, when was meant to
> > > be a tiny driver.
> > >
> > > According to Maxime it's the exception rather than the rule and suggested
> > > to change it, I don't really have a strong opinion on either naming TBH.
> >
> > Maybe that's just me, but the helper in the name indeed throws me off. In my
> > mind, it's supposed to be used only for helpers, not functions implementing the
> > helpers hooks.
> 
> With several callbacks using the same (field) name, it is very helpful
> to name the actual implementation by combining the struct type name
> and the field name.

I can't think of any (at least for a given object). Which one do you have in
mind?

> Anything else confuses the casual reader. Perhaps the real question is whether
> the structures should have "helper" in their name in the first place?

Those structures are meant for functions used by the helpers, they are not
helper functions.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)