linux-kernel - Re: [PATCH v2 1/4] drm/dp: Add helper to set LTTPRs in transparent mode

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: <87y0zxnm1p.fsf@intel.com>
Date: Mon, 30 Dec 2024 19:00:18 +0200
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Abel Vesa <abel.vesa@...aro.org>, Johan Hovold <johan@...nel.org>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
 <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David
 Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Karol Herbst
 <kherbst@...hat.com>, Lyude Paul <lyude@...hat.com>, Danilo Krummrich
 <dakr@...hat.com>, Rodrigo Vivi <rodrigo.vivi@...el.com>, Joonas Lahtinen
 <joonas.lahtinen@...ux.intel.com>, Tvrtko Ursulin <tursulin@...ulin.net>,
 Rob Clark <robdclark@...il.com>, Abhinav Kumar
 <quic_abhinavk@...cinc.com>, Sean Paul <sean@...rly.run>, Marijn Suijten
 <marijn.suijten@...ainline.org>, Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konradybcio@...nel.org>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, nouveau@...ts.freedesktop.org,
 intel-gfx@...ts.freedesktop.org, intel-xe@...ts.freedesktop.org,
 linux-arm-msm@...r.kernel.org, freedreno@...ts.freedesktop.org
Subject: Re: [PATCH v2 1/4] drm/dp: Add helper to set LTTPRs in transparent
 mode

On Mon, 30 Dec 2024, Dmitry Baryshkov <dmitry.baryshkov@...aro.org> wrote:
> On Mon, Dec 30, 2024 at 03:18:35PM +0200, Jani Nikula wrote:
>> On Thu, 26 Dec 2024, Abel Vesa <abel.vesa@...aro.org> wrote:
>> > On 24-12-11 15:42:27, Johan Hovold wrote:
>> >> On Wed, Dec 11, 2024 at 03:04:12PM +0200, Abel Vesa wrote:
>> >>  
>> >> > +/**
>> >> > + * drm_dp_lttpr_set_transparent_mode - set the LTTPR in transparent mode
>> >> > + * @aux: DisplayPort AUX channel
>> >> > + * @enable: Enable or disable transparent mode
>> >> > + *
>> >> > + * Returns 0 on success or a negative error code on failure.
>> >> > + */
>> >> > +int drm_dp_lttpr_set_transparent_mode(struct drm_dp_aux *aux, bool enable)
>> >> > +{
>> >> > +	u8 val = enable ? DP_PHY_REPEATER_MODE_TRANSPARENT :
>> >> > +			  DP_PHY_REPEATER_MODE_NON_TRANSPARENT;
>> >> > +	int ret = drm_dp_dpcd_writeb(aux, DP_PHY_REPEATER_MODE, val);
>> >> > +
>> >> > +	return ret == 1 ? 0 : ret;
>> >> 
>> >> This looks correct, but I had to go look at drm_dp_dpcd_writeb() to make
>> >> sure it never returns 0 (for short transfers).
>> >
>> > Will follow Dmitry's proposal here.
>> >
>> > 	if (ret < 0)
>> >         	return ret;
>> >
>> > 	return (ret == 1) ? 0 : -EIO;
>> 
>> Arguably this (well, with ret == len) is what we should've done with
>> *all* of the drm_dp_dpcd_*() functions. I don't think there's a single
>> case where we'd actually need to know that some but not all data was
>> transferred. And if there are, they could be special cased. Now we have
>> hundreds of cases where we check against length and it's just cumbersome
>> all over the place.
>> 
>> The question is, how confusing is it going to be to have some of the new
>> functions return 0 instead of len? Very? Extremely?
>> 
>> As painful as it would be, I'd be in favor of changing them all to
>> return 0 on ret == len. If we find a volunteer.
>
> Maybe a correct Coccinelle script can do a significant part of such a
> conversion for us?
>
> Anyway, I think it a right thing to do. Could you possibly add a new set
> of API and use it inside i915 driver? Then during the next cycle we can
> start using new functions for all other drivers. Or would you rather add
> new API through drm-misc? Then we can concert e.g. existing helpers in
> the first place and then start working on the drivers.

There are hundreds of drm_dp_dpcd_{read,readb,write,writeb} uses across
drm, and then all the higher level helpers on top. I'm not sure adding a
new API and using it in i915 achieves much.

BR,
Jani.


-- 
Jani Nikula, Intel