[Intel-gfx] [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Chris Wilson]

On Wed, Dec 02, 2015 at 01:29:21PM +0000, Dave Gordon wrote:
> On 25/11/15 09:23, Daniel Vetter wrote:
> >On Tue, Nov 24, 2015 at 11:47:26PM +0000, Chris Wilson wrote:
> >>On Tue, Nov 24, 2015 at 10:26:01PM +0000, Chris Wilson wrote:
> >>>On Tue, Nov 24, 2015 at 07:36:25PM +0200, Jani Nikula wrote:
> >>>>  /* Iterate over initialised rings */
> >>>>  #define for_each_ring(ring__, dev_priv__, i__) \
> >>>>  	for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
> >>>>-		if (((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__)))
> >>>>+		for_each_if ((((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__))))
> >>>
> >>>Idly wondering if we would be happy with
> >>>
> >>>for_each_ring(ring__, dev_priv__)
> >>>	for ((ring__) = &(dev_priv__)->ring[0];
> >>>	     (ring__) <= &(dev_priv__)->ring[I915_NUM_RINGS];
> >>>	     (ring__)++)
> >>>	     for_each_if(intel_ring_initialized(ring__))
> >>>
> >>>?
> >>>
> >>>The downside is that we have used i__ in several places rather than
> >>>ring->id.
> >>
> >>Fwiw, 13 files changed, 113 insertions(+), 140 deletions(-)
> >>
> >>Seems a reasonable shrinkage.
> >
> >Maybe for_each_engine even, and phase out for_each_ring completely?
> >-Daniel
> >
> 
> Wouldn't it be nicer (and safer) not to build macros that fold the
> loop structure into the macro (in contravention of kernel
> programming guidelines).
> 
> So how about NOT including the actual for() inside the macro, so
> that instead of writing
> 
> 	for_each_engine(engine, dev_priv)
> 		do_stuff(engine)
> 
> we would write it as
> 
> 	for (EACH_ENGINE(engine, dev_priv))
> 		initialise(engine)
> 
> 	for (EACH_ACTIVE_ENGINE(engine, dev_priv)) {
> 		service(engine)
> 		restart(engine)
> 	}
> 
> etc. The for-loop is visible and the scope doesn't give you any surprises.
> 
> [The EACH_ENGINE() macros expands to a semicolon-separated triplet
> of expressions; still a violation of the "don't use macros to
> redefine C syntax" guideline, but much less egregious than macros
> that contain embedded 'for's and 'if's.

for_each() is common practice in the kernel, so hiding the for() inside
the macro isn't that egregious. The problem is defining EACH_ACTIVE_ENGINE()
simply, preferrably without the use of another loop inside the for(;;).
One is to pack the i915->engines[] and have i915->num_engines and
intel_lookup_engine (or an i915->engine_for_id[]) for the occasional
case where we look up e.g. &i915->engines[BCS].
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre