Re: [Intel-gfx] [PATCH v3 7/9] drm/i915: Factor out function to get/put AUX_IO power for main link

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 10 Nov 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Nov 08, 2022 at 05:18:26PM +0200, Imre Deak wrote:
>> +static void
>> +main_link_aux_power_domain_put(struct intel_digital_port *dig_port,
>> +			       const struct intel_crtc_state *crtc_state)
>> +{
>> +	struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev);
>> +	intel_wakeref_t wf = fetch_and_zero(&dig_port->aux_wakeref);
>
> Please don't call functions with side effects in variable
> declaration blocks. Far too easy to miss them and then you end up
> scratching your head for a day or two debugging the wrong thing.

Side note, if I got to decide, I'd probably nuke "fetch_and_zero" out of
existence.

It's not that I don't find the concept useful, it's the naming that
gives the impression of atomicity that the macro utterly lacks. I find
that dangerous.

It's a helper that feels like should be part of a core kernel header
(and you might mistakenly think it already is!) but I doubt would ever
pass muster because of the above.


BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center