linux-kernel - Re: [PATCH v2 1/6] drm/bridge: ti-sn65dsi86: Export bridge GPIOs to Linux

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

Message-ID: <CAD=FV=UeZS94rgmjBDUfgt3B5f9WkAy-7jrLAcNCe-v4nZB_fg@mail.gmail.com>
Date:   Wed, 22 Apr 2020 16:31:24 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Andrzej Hajda <a.hajda@...sung.com>,
        David Airlie <airlied@...ux.ie>, bgolaszewski@...libre.com,
        Daniel Vetter <daniel@...ll.ch>,
        LinusW <linus.walleij@...aro.org>,
        Neil Armstrong <narmstrong@...libre.com>,
        Rob Herring <robh+dt@...nel.org>,
        Sandeep Panda <spanda@...eaurora.org>,
        Jonas Karlman <jonas@...boo.se>,
        Jeffrey Hugo <jeffrey.l.hugo@...il.com>,
        open list:GPIO SUBSYSTEM <linux-gpio@...r.kernel.org>, linux-arm-msm <linux-arm-msm@...r.kernel.org>, dri-devel <dri-devel@...ts.freedesktop.org>, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@...r.kernel.org>, Jernej Skrabec <jernej.skrabec@...l.net>, Bjorn Andersson <bjorn.andersson@...aro.org>, Rob Clark <robdclark@...omium.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/6] drm/bridge: ti-sn65dsi86: Export bridge GPIOs to Linux

Hi,

On Wed, Apr 22, 2020 at 2:07 PM Stephen Boyd <swboyd@...omium.org> wrote:
>
> Quoting Doug Anderson (2020-04-22 09:09:17)
> > Hi,
> >
> > On Wed, Apr 22, 2020 at 3:23 AM Stephen Boyd <swboyd@...omium.org> wrote:
> > >
> > > Quoting Douglas Anderson (2020-04-20 22:06:17)
> > >
> > > > Changes in v2:
> > > > - ("Export...GPIOs") is 1/2 of replacement for ("Allow...bridge GPIOs")
> > > >
> > > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 165 ++++++++++++++++++++++++++
> > > >  1 file changed, 165 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > index 6ad688b320ae..d04c2b83d699 100644
> > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > @@ -874,6 +886,153 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
> > > >         return 0;
> > > >  }
> > > >
> > > > +static struct ti_sn_bridge *gchip_to_pdata(struct gpio_chip *chip)
> > > > +{
> > > > +       return container_of(chip, struct ti_sn_bridge, gchip);
> > > > +}
> > > > +
> > > > +static int ti_sn_bridge_gpio_get_direction(struct gpio_chip *chip,
> > > > +                                          unsigned int offset)
> > > > +{
> > > > +       struct ti_sn_bridge *pdata = gchip_to_pdata(chip);
> > > > +
> > > > +       return (atomic_read(&pdata->gchip_output) & BIT(offset)) ?
> > >
> > > Any reason this isn't a bitmap?
> >
> > Don't bitmaps need an external lock to protect against concurrent
> > access?
>
> Bitmaps are sometimes atomic. See Documentation/atomic_bitops.txt for
> more info. From what I see here it looks like we can have a bitmap for
> this and then use set_bit(), test_and_set_bit(), etc. and it will be the
> same and easier to read.

Hrm.  Somehow I found the wrong place when trying to search for this
previously.  Thanks for pointing me in the right directions.  Much
nicer.


> > When I looked I wasn't convinced that the GPIO subsystem
> > prevented two callers from changing two GPIOs at the same time.  See
> > below for a bigger discussion.
> >
> >
> > > > +               GPIOF_DIR_OUT : GPIOF_DIR_IN;
> > >
> > > And why can't we read the hardware to figure out if it's in output or
> > > input mode?
> >
> [...]
> >
> > In the next version of the patch I'll plan to add a kerneldoc comment
> > to "struct ti_sn_bridge" and add a summary of the above for
> > "gchip_output".
>
> Yeah it deserves a comment in the code indicating why it doesn't read
> the hardware.

OK, added a whole bunch more comments.

Barring something coming up, I'll plan to send v3 tomorrow morning
with all your feedback addressed.  If anyone wants me to wait because
they think I need some more major change, please yell.


-Doug