linux-kernel - Re: [PATCH v2] drm/panel: boe-bf060y8m-aj0: transition to mipi

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: <mz4axwltt6zhm2hykenerz2k6hp5qb4tqa3seui2vnztsldpoo@hejaeukdu2tg>
Date: Tue, 1 Apr 2025 04:52:20 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Doug Anderson <dianders@...omium.org>
Cc: Tejas Vipin <tejasvipin76@...il.com>, neil.armstrong@...aro.org,
        maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
        tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch,
        quic_jesszhan@...cinc.com, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, asrivats@...hat.com
Subject: Re: [PATCH v2] drm/panel: boe-bf060y8m-aj0: transition to mipi_dsi
 wrapped functions

On Tue, Apr 01, 2025 at 04:01:03AM +0300, Dmitry Baryshkov wrote:
> On Mon, Mar 31, 2025 at 03:40:27PM -0700, Doug Anderson wrote:
> > Hi,
> > 
> > On Mon, Mar 31, 2025 at 1:28 PM Dmitry Baryshkov
> > <dmitry.baryshkov@....qualcomm.com> wrote:
> > >
> > > On Mon, Mar 31, 2025 at 08:06:36AM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Sun, Mar 30, 2025 at 11:18 PM Tejas Vipin <tejasvipin76@...il.com> wrote:
> > > > >
> > > > > @@ -157,7 +137,6 @@ static int boe_bf060y8m_aj0_prepare(struct drm_panel *panel)
> > > > >
> > > > >         ret = boe_bf060y8m_aj0_on(boe);
> > > > >         if (ret < 0) {
> > > > > -               dev_err(dev, "Failed to initialize panel: %d\n", ret);
> > > > >                 gpiod_set_value_cansleep(boe->reset_gpio, 1);
> > > > >                 return ret;
> > > >
> > > > It's not new, but the error handling here looks wrong to me. Instead
> > > > of just returning after setting the GPIO, this should be turning off
> > > > the regulators, shouldn't it? That would mean adding a new error label
> > > > for turning off "BF060Y8M_VREG_VCI" and then jumping to that.
> > >
> > > We should not be turning off the regulator in _prepare(), there will be
> > > an unmatched regulator disable call happening in _unprepare(). Of course
> > > it can be handled by adding a boolean, etc, but I think keeping them on
> > > is a saner thing.
> > 
> > Hrmmmm.
> > 
> > The issue is that if we're returning an error from a function the
> > caller should expect that the function undid anything that it did
> > partially. It _has_ to work that way, right? Otherwise we've lost the
> > context of exactly how far we got through the function so we don't
> > know which things to later undo and which things to later not undo.
> 
> Kind of yes. I'd rather make drm_panel functions return void here, as
> that matches panel bridge behaviour. The only driver that actually uses
> return values of those functions is analogix_dp, see
> analogix_dp_prepare_panel(). However most of invocations of that
> function can go away. I'll send a patchset.
> 
> > 
> > ...although I think you said that the DRM framework ignores errors
> > from prepare() and still calls unprepare(). I guess this is in
> > panel_bridge_atomic_pre_enable() where drm_panel_prepare()'s error
> > code is ignored?
> 

Hmm... Most of the drivers ignore the results of the drm_panel_prepare()
/ _unprepare() / _enable() / _disable(), but then the framework handles
error values of the callbacks and skips calling the corresponding
en/dis callback if the previous call has failed. Which means I was
incorrect here.

> 
> > This feels like a bug waiting to happen. Are you
> > saying that boe_bf060y8m_aj0_unprepare() has to be written such that
> > it doesn't hit regulator underflows no matter which fail path
> > boe_bf060y8m_aj0_prepare() hit? That feels wrong.
> 
> Let me try to fix that.
> 
> -- 
> With best wishes
> Dmitry

-- 
With best wishes
Dmitry