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-next>] [day] [month] [year] [list]
Message-ID: <aF6UOCLdO0fGHGA9@intel.com>
Date: Fri, 27 Jun 2025 15:53:12 +0300
From: Ville Syrjälä <ville.syrjala@...ux.intel.com>
To: Jani Nikula <jani.nikula@...el.com>
Cc: intel-gfx@...ts.freedesktop.org, intel-xe@...ts.freedesktop.org,
	Imre Deak <imre.deak@...el.com>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Matt Wagantall <mattw@...eaurora.org>,
	Dejin Zheng <zhengdejin5@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 18/18] drm/i915/ddi: prefer read_poll_timeout() over
 readx_poll_timeout()

On Fri, Jun 27, 2025 at 02:36:32PM +0300, Jani Nikula wrote:
> Unify on using read_poll_timeout() throughout instead of mixing with
> readx_poll_timeout(). While the latter can be ever so slightly simpler,
> they are both complicated enough that it's better to unify on one
> approach only.
> 
> While at it, better separate the handling of error returns from
> drm_dp_dpcd_readb() and the actual status byte. This is best achieved by
> inlining the read_fec_detected_status() function.
> 
> Cc: Imre Deak <imre.deak@...el.com>
> Signed-off-by: Jani Nikula <jani.nikula@...el.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 33 +++++++++---------------
>  1 file changed, 12 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 0405396c7750..fc4587311607 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -2339,34 +2339,25 @@ static void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
>  		drm_dbg_kms(display->drm, "Failed to clear FEC detected flags\n");
>  }
>  
> -static int read_fec_detected_status(struct drm_dp_aux *aux)
> -{
> -	int ret;
> -	u8 status;
> -
> -	ret = drm_dp_dpcd_readb(aux, DP_FEC_STATUS, &status);
> -	if (ret < 0)
> -		return ret;
> -
> -	return status;
> -}
> -
>  static int wait_for_fec_detected(struct drm_dp_aux *aux, bool enabled)
>  {
>  	struct intel_display *display = to_intel_display(aux->drm_dev);
>  	int mask = enabled ? DP_FEC_DECODE_EN_DETECTED : DP_FEC_DECODE_DIS_DETECTED;
> -	int status;
> -	int err;
> +	u8 status = 0;
> +	int ret, err;
>  
> -	err = readx_poll_timeout(read_fec_detected_status, aux, status,
> -				 status & mask || status < 0,
> -				 10000, 200000);
> +	ret = read_poll_timeout(drm_dp_dpcd_readb, err,
> +				err || (status & mask),
> +				10 * 1000, 200 * 1000, false,
> +				aux, DP_FEC_STATUS, &status);

I think I hate these macros. It's very hard to tell from this
soup what is actually being done here.

The 'val', 'op', and 'args' look very disconnected here even though
they are always part of the same thing. Is there a reason they can't
just be a single 'op' parameter like we have in wait_for() so you can
actually see the code?

Ie.
read_poll_timeout(err = drm_dp_dpcd_readb(aux, DP_FEC_STATUS, &status),
		  err || (status & mask),
                  10 * 1000, 200 * 1000, false);
?

>  
> -	if (err || status < 0) {
> +	/* Either can be non-zero, but not both */
> +	ret = ret ?: err;
> +	if (ret) {
>  		drm_dbg_kms(display->drm,
> -			    "Failed waiting for FEC %s to get detected: %d (status %d)\n",
> -			    str_enabled_disabled(enabled), err, status);
> -		return err ? err : status;
> +			    "Failed waiting for FEC %s to get detected: %d (status 0x%02x)\n",
> +			    str_enabled_disabled(enabled), ret, status);
> +		return ret;
>  	}
>  
>  	return 0;
> -- 
> 2.39.5

-- 
Ville Syrjälä
Intel

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ