linux-kernel - Re: [PATCH v2 08/11] drm: Rename plane->state variables in atomic update and disable

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] [day] [month] [year] [list]

Message-ID: <YA6xBuECFjzjY7gG@intel.com>
Date:   Mon, 25 Jan 2021 13:52:38 +0200
From:   Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:     Maxime Ripard <maxime@...no.tech>
Cc:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Daniel Vetter <daniel.vetter@...el.com>,
        David Airlie <airlied@...ux.ie>,
        Neil Armstrong <narmstrong@...libre.com>,
        Xinliang Liu <xinliang.liu@...aro.org>,
        Liviu Dudau <liviu.dudau@....com>,
        Philippe Cornu <philippe.cornu@...com>,
        Paul Cercueil <paul@...pouillou.net>,
        linux-tegra@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        linux-stm32@...md-mailman.stormreply.com,
        Jerome Brunet <jbrunet@...libre.com>,
        Marek Vasut <marex@...x.de>,
        Yannick Fertre <yannick.fertre@...com>,
        linux-samsung-soc@...r.kernel.org,
        Joonyoung Shim <jy0922.shim@...sung.com>,
        Kevin Hilman <khilman@...libre.com>,
        linux-mediatek@...ts.infradead.org,
        Russell King <linux@...linux.org.uk>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Jonathan Hunter <jonathanh@...dia.com>,
        linux-rockchip@...ts.infradead.org,
        Xinwei Kong <kong.kongxinwei@...ilicon.com>,
        NXP Linux Team <linux-imx@....com>,
        linux-arm-msm@...r.kernel.org, Dave Airlie <airlied@...hat.com>,
        linux-mips@...r.kernel.org,
        Chun-Kuang Hu <chunkuang.hu@...nel.org>,
        Alexandre Torgue <alexandre.torgue@...com>,
        Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        Chen Feng <puck.chen@...ilicon.com>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Alison Wang <alison.wang@....com>,
        dri-devel@...ts.freedesktop.org,
        Laurentiu Palcu <laurentiu.palcu@....nxp.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        linux-amlogic@...ts.infradead.org, freedreno@...ts.freedesktop.org,
        Sean Paul <sean@...rly.run>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        linux-arm-kernel@...ts.infradead.org,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Tomi Valkeinen <tomba@...nel.org>,
        Jyri Sarha <jyri.sarha@....fi>,
        Seung-Woo Kim <sw0312.kim@...sung.com>,
        Sandy Huang <hjc@...k-chips.com>, linux-kernel@...r.kernel.org,
        Vincent Abriou <vincent.abriou@...com>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Tian Tao <tiantao6@...ilicon.com>
Subject: Re: [PATCH v2 08/11] drm: Rename plane->state variables in atomic
 update and disable

On Mon, Jan 25, 2021 at 11:52:18AM +0100, Maxime Ripard wrote:
> Hi Ville,
> 
> On Fri, Jan 22, 2021 at 02:15:07PM +0200, Ville Syrjälä wrote:
> > On Thu, Jan 21, 2021 at 05:35:33PM +0100, Maxime Ripard wrote:
> > > Some drivers are storing the plane->state pointer in atomic_update and
> > > atomic_disable in a variable simply called state, while the state passed
> > > as an argument is called old_state.
> > > 
> > > In order to ease subsequent reworks and to avoid confusing or
> > > inconsistent names, let's rename those variables to new_state.
> > > 
> > > This was done using the following coccinelle script, plus some manual
> > > changes for mtk and tegra.
> > > 
> > > @ plane_atomic_func @
> > > identifier helpers;
> > > identifier func;
> > > @@
> > > 
> > > (
> > >  static const struct drm_plane_helper_funcs helpers = {
> > >  	...,
> > >  	.atomic_disable = func,
> > > 	...,
> > >  };
> > > |
> > >  static const struct drm_plane_helper_funcs helpers = {
> > >  	...,
> > >  	.atomic_update = func,
> > > 	...,
> > >  };
> > > )
> > > 
> > > @ moves_new_state_old_state @
> > > identifier plane_atomic_func.func;
> > > identifier plane;
> > > symbol old_state;
> > > symbol state;
> > > @@
> > > 
> > >  func(struct drm_plane *plane, struct drm_plane_state *old_state)
> > >  {
> > >  	...
> > > -	struct drm_plane_state *state = plane->state;
> > > +	struct drm_plane_state *new_state = plane->state;
> > > 	...
> > >  }
> > > 
> > > @ depends on moves_new_state_old_state @
> > > identifier plane_atomic_func.func;
> > > identifier plane;
> > > identifier old_state;
> > > symbol state;
> > > @@
> > > 
> > >  func(struct drm_plane *plane, struct drm_plane_state *old_state)
> > >  {
> > >  	<...
> > > -	state
> > > +	new_state
> > > 	...>
> > 
> > Was going to say that this migh eat something else, but I guess
> > the dependency prevents that?
> 
> Yeah, the dependency takes care of this
> 
> > Another way to avoid that I suppose would be to declare 'state'
> > as
> > symbol moves_new_state_old_state.state;
> > 
> > That would probably make the intent a bit more obvious, even with
> > the dependency. Or does a dependency somehow automagically imply
> > that?
> 
> I'm not sure if it does, but it's a symbol here not an identifier or an
> expression, so here moves_new_state_old_state.state would always resolve
> to state (and only state) anyway

Hm. Right. OK, cocci bits look good to me. Variable naming
bikeshed I'll leave to others :)

Reviewed-by: Ville Syrjälä <ville.syrjala@...ux.intel.com>

-- 
Ville Syrjälä
Intel