linux-kernel - Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly

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]

Date:   Tue, 3 May 2022 16:23:56 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        Robert Foss <robert.foss@...aro.org>,
        Hsin-Yi Wang <hsinyi@...omium.org>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Sankeerth Billakanti <quic_sbillaka@...cinc.com>,
        Philip Chen <philipchen@...omium.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Linus Walleij <linus.walleij@...aro.org>,
        Lyude Paul <lyude@...hat.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to
 use DP AUX bus properly

Hi,

On Mon, Apr 18, 2022 at 4:10 PM Doug Anderson <dianders@...omium.org> wrote:
>
> > > 5. In general I've been asserting that it should be up to the panel to
> > > power things on and drive all AUX transactions. ...but clearly my
> > > model isn't reality. We certainly do AUX transactions from the DP
> > > driver because the DP driver needs to know things about the connected
> > > device, like the number of lanes it has, the version of eDP it
> > > supports, and the available bit rates to name a few. Those things all
> > > work today by relying on the fact that pre-enable powers the panel on.
> > > It's pretty easy to say that reading the EDID (and I guess AUX
> > > backlight) is the odd one out. So right now I guess my model is:
> > >
> > > 5a) If the panel code wants to access the AUX bus it can do so by
> > > powering itself on and then just doing an AUX transaction and assuming
> > > that the provider of the AUX bus can power itself on as needed.
> > >
> > > 5b) If the DP code wants to access the AUX bus it should make sure
> > > that the next bridge's pre_enable() has been called. It can then
> > > assume that the device is powered on until the next bridge's
> > > post_disable() has been called.
> > >
> > > So I guess tl;dr: I'm not really a huge fan of the DP driver powering
> > > the panel on by doing a pm_runtime_get() on it. I'd prefer to keep
> > > with the interface that we have to pre_enable() the panel to turn it
> > > on.
> >
> > Again, thank for the explanation. Your concerns make more sense now.
> > As much as I hate writing docs, maybe we should put at least basic notes
> > (regarding panel requirements) somewhere to the DP/DP AUX documentation?
>
> Sure. I actually don't mind writing docs, but my problem is trying to
> figure out where the heck to put them where someone would actually
> notice them. I could throw a blurb in the kernel doc for `struct
> drm_dp_aux` I guess? How about a deal: if you can tell me where to put
> the above facts (essentially 5a and 5b) then I'm happy to post a patch
> adding them.
>
> Huh, actually, maybe the right place is in the "transfer" function of
> that structure which, as of commit bacbab58f09d ("drm: Mention the
> power state requirement on side-channel operations"), actually has a
> blurb. ...but I don't think the blurb there is totally complete. What
> if I changed it to this:
>
>  * Also note that this callback can be called no matter the
>  * state @dev is in and also no matter what state the panel is
>  * in. It's expected:
>  * - If the @dev providing the AUX bus is currently unpowered then
>  *   it will power itself up for the transfer.
>  * - If the panel is not in a state where it can respond (it's not
>  *   powered or it's in a low power state) then this function will
>  *   fail. Note that if a panel driver is initiating a DP AUX transfer
>  *   it may power itself up however it wants. All other code should
>  *   use the pre_enable() bridge chain (which eventually calls the
>  *   panel prepare function) to power the panel.

I didn't ignore this documentation request. Please take a look here
and see what you think:

https://lore.kernel.org/r/20220503162033.1.Ia8651894026707e4fa61267da944ff739610d180@changeid

-Doug