linux-kernel - Re: [PATCH RFC 2/7] drm/display: dp: implement new access helpers

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <oimolivajra4a7jmeloa5g4kuw2t54whmvy2gpeayo5htkcyb4@ryev34rq2m4j>
Date: Thu, 23 Jan 2025 13:04:31 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Jani Nikula <jani.nikula@...ux.intel.com>
Cc: 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>, Rob Clark <robdclark@...il.com>, 
	Abhinav Kumar <quic_abhinavk@...cinc.com>, Sean Paul <sean@...rly.run>, 
	Marijn Suijten <marijn.suijten@...ainline.org>, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	linux-arm-msm@...r.kernel.org, freedreno@...ts.freedesktop.org
Subject: Re: [PATCH RFC 2/7] drm/display: dp: implement new access helpers

On Thu, Jan 23, 2025 at 12:26:25PM +0200, Jani Nikula wrote:
> On Fri, 17 Jan 2025, Dmitry Baryshkov <dmitry.baryshkov@...aro.org> wrote:
> > Existing DPCD access functions return an error code or the number of
> > bytes being read / write in case of partial access. However a lot of
> > drivers either (incorrectly) ignore partial access or mishandle error
> > codes. In other cases this results in a boilerplate code which compares
> > returned value with the size.
> >
> > Implement new set of DPCD access helpers, which ignore partial access,
> > always return 0 or an error code. Implement existing helpers using the
> > new functions to ensure backwards compatibility.
> >
> > Suggested-by: Jani Nikula <jani.nikula@...ux.intel.com>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> > ---
> >  drivers/gpu/drm/display/drm_dp_helper.c       | 42 +++++++-------
> >  drivers/gpu/drm/display/drm_dp_mst_topology.c | 27 +++++----
> >  include/drm/display/drm_dp_helper.h           | 81 ++++++++++++++++++++++++++-
> >  include/drm/display/drm_dp_mst_helper.h       | 10 ++--
> >  4 files changed, 119 insertions(+), 41 deletions(-)
> >
> > +
> > +/**
> > + * drm_dp_dpcd_write() - write a series of bytes from the DPCD
> > + * @aux: DisplayPort AUX channel (SST or MST)
> > + * @offset: address of the (first) register to write
> > + * @buffer: buffer containing the values to write
> > + * @size: number of bytes in @buffer
> > + *
> > + * Deprecated wrapper around drm_dp_dpcd_write().
> > + * Returns the number of bytes transferred on success, or a negative error
> > + * code on failure.
> > + */
> > +static inline ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux,
> > +					unsigned int offset,
> > +					void *buffer, size_t size)
> > +{
> > +	int ret = drm_dp_dpcd_write_data(aux, offset, buffer, size);
> > +
> > +	if (ret < 0)
> > +		return ret;
> 
> I just realized this changes behaviour. This no longer returns the
> number of bytes transferred when it's less than size. It'll always be an
> error.
> 
> Now, if we were to accept this change, I wonder if we could do that as a
> standalone change first, within the current functions? Return either
> size or negative error, nothing between [0..size).
> 
> After that, we could change all the return checks for "!= size" or "<
> size" to "< 0" (because they would be the same thing). When all the
> places have been changed, we could eventually switch from returning size
> to returning 0 on success when nobody depends on it anymore, and keep
> the same function names.
> 
> I think this does have a certain appeal to it. Thoughts?

I thought about it while working on the series. There is an obvious and
huge problem: with the changed function names you actually have to
review usage patterns and verify that the return comparison is correct.
If the name is unchanged, it is easy to miss such usage patterns. For
example, i915 / amd / msm drivers are being developed in their own
trees. Even if we review those drivers at some point, nothing stops them
from adding new code points, checking for size instead of 0. Likewise
patches-in-flight also can't be properly reviewed if we just change the
return value.

Thus, I think, while the idea of keeping function names sounds
appealing, it doesn't help in a longer term and can potentially create
even more confusion.

-- 
With best wishes
Dmitry