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: <y5l6gr7gdrz6syc3kxortl4p52bpygs2cqzkgayhnbsvrjcbcw@hxhel54zw372>
Date: Tue, 1 Apr 2025 04:01:03 +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 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?

Yes.

> 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