Re: [cocci] Searching for special function implementations with SmPL

[Luca Coelho <luca@coelho.fi>]

On Sat, 2023-01-28 at 14:33 +0100, Julia Lawall wrote:
> 
> On Sat, 28 Jan 2023, Luca Coelho wrote:
> 
> > On Sat, 2023-01-28 at 10:49 +0100, Julia Lawall wrote:
> > > 
> > > On Sat, 28 Jan 2023, Markus Elfring wrote:
> > > 
> > > > > @@
> > > > > identifier nested_macros.nm;
> > > > > function f;
> > > > > identifier dev_priv;
> > > > 
> > > > 
> > > > How do you think about to use the following SmPL code variant instead of
> > > > the metavariable type “function”?
> > > > 
> > > > 
> > > > identifier dev_priv, f;
> > > 
> > > Good suggestion,  Function is not really supported.  I think it behaves
> > > just like identifier, but it would be better to just use identifier.
> > > Function was an idea thta never actually got implemented.
> > 
> > Good idea, I generally don't use function, but I'd been trying lots of
> > different things before I reached out for help. 🙂 I guess I tried with
> > function at some point and then ended up leaving it there.
> > 
> > Now I tried with f as identifier, but it didn't make a difference.
> > 
> > I can actually match _one_ of the occurrences now, but there are many
> > other ones that are not matching...
> > 
> > This is what I'm using:
> > 
> > @macros_noargs@
> > identifier m;
> > expression e =~ "dev_priv";
> > @@
> > #define m <+...e...+>
> > 
> > @nested_macros@
> > identifier macros_noargs.m;
> > identifier nm;
> > @@
> > #define nm(...) <+...m...+>
> > 
> > @@
> > identifier nested_macros.nm;
> > identifier dev_priv, f;
> > expression e;
> > @@
> > f(...) {
> > 	...
> > -	struct drm_i915_private *dev_priv = e;
> > +	struct drm_i915_private *i915 = e;
> > 
> > 	<+...
> > 	nm(...)
> > 	...+>
> > }
> > 
> > ...and this is the command line:
> > 
> > spatch --sp-file ~/dev_priv_i915.spatch -I drivers/gpu/drm/i915/display \
> >  --recursive-includes --in-place --dir ./drivers/gpu/drm/i915/display
> > 
> > ...and this is the only match I get:
> > 
> > HANDLING: ./drivers/gpu/drm/i915/display/intel_display.c
> > diff =
> > diff -u -p a/intel_display.c b/intel_display.c
> > --- a/intel_display.c
> > +++ b/intel_display.c
> > @@ -3943,7 +3943,7 @@ static bool bxt_get_dsi_transcoder_state
> >  					 struct intel_display_power_domain_set *power_domain_set)
> >  {
> >  	struct drm_device *dev = crtc->base.dev;
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_i915_private *i915 = to_i915(dev);
> >  	enum transcoder cpu_transcoder;
> >  	enum port port;
> >  	u32 tmp;
> > 
> > Many other files are handled, but there are no matches, though I
> > believe there should be.
> 
> Sorry, but it's not obvious how to debug this without any idea of what
> should be found,  Maybe the proper macro definitions are not being found.
> Maybe the proper uses are not being found.  Do you have a concrete example
> where a change should happen?

Yes, sorry for not being clear enough.  The issue I'm trying to solve
is that we have bunch of macros, such as MIPI_CTRL(), that implicitly
uses dev_priv, so it must be declared as a local in the function
calling it.  I want to make this explicit, so I want to do exactly
this:


@macros_noargs@
identifier m;
expression e =~ "dev_priv";
@@
#define m <+...e...+>

@nested_macros@
identifier macros_noargs.m;
identifier nm;
identifier list il;
@@
#define nm(il) <+...m...+>

@@
identifier nested_macros.nm;
identifier list il;
expression e;
@@
-#define nm(il) e
+#define nm(dev_priv, il) e

@@
identifier nested_macros.nm;
identifier dev_priv, f;
expression e;
expression list il;
@@
f(...) {
	...
(
	struct drm_i915_private *dev_priv = e;
|
	struct drm_i915_private *dev_priv;
)

	<+...
-	nm(il)
+	nm(dev_priv, il)
	...+>
}


The idea is that all the macros that are implicitly using dev_priv,
will have a new argument where dev_priv is passed.

So, eg. in drivers/gpu/drm/i915/display/vlv_dsi.c, the following code:

static bool glk_dsi_enable_io(struct intel_encoder *encoder)
{
	struct drm_i915_private *dev_priv = to_i915(encoder-
>base.dev);
[...]
	for_each_dsi_port(port, intel_dsi->ports) {
		tmp = intel_de_read(dev_priv, MIPI_CTRL(port));
		intel_de_write(dev_priv, MIPI_CTRL(port),
			       tmp | GLK_MIPIIO_ENABLE);
	}
[...]
}

Would become:

static bool glk_dsi_enable_io(struct intel_encoder *encoder)
{
	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
[...]
	for_each_dsi_port(port, intel_dsi->ports) {
		tmp = intel_de_read(dev_priv, MIPI_CTRL(dev_priv, port));
		intel_de_write(dev_priv, MIPI_CTRL(dev_priv, port),
			       tmp | GLK_MIPIIO_ENABLE);
	}
[...]
}


And the macro definition:

#define MIPI_CTRL(port)			_MMIO_MIPI(port, _MIPIA_CTRL, _MIPIC_CTRL)

Would become:

#define MIPI_CTRL(dev_priv,port) _MMIO_MIPI(port, _MIPIA_CTRL, _MIPIC_CTRL)


After mangling a lot with the rules, I can see that this now works, but
only if I select the vlv_dsi.c file alone, like this:

spatch --sp-file ~/dev_priv_i915.spatch -I drivers/gpu/drm/i915/display \
 --all-includes --in-place ./drivers/gpu/drm/i915/display/vlv_dsi.c


If I try to run the rules in all files in that directory, only the
intel_display.c file is affected:

spatch --sp-file ~/dev_priv_i915.spatch -I drivers/gpu/drm/i915/display  \
 --all-includes --in-place ./drivers/gpu/drm/i915/display


Of course, I can work around this by using find and running spatch
individually in every file... But not ideal.

--
Cheers,
Luca.