linux-kernel - Re: [PATCH v2 1/4] drm/bridge: ti-sn65dsi86: Combine register accesses in ti_sn_aux

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]

Date:   Mon, 02 Nov 2020 09:06:38 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Andrzej Hajda <a.hajda@...sung.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        LKML <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Sean Paul <seanpaul@...omium.org>
Subject: Re: [PATCH v2 1/4] drm/bridge: ti-sn65dsi86: Combine register accesses in ti_sn_aux_transfer()

Quoting Doug Anderson (2020-11-02 08:18:47)
> Hi,
> 
> On Thu, Oct 29, 2020 at 6:17 PM Stephen Boyd <swboyd@...omium.org> wrote:
> >
> > These register reads and writes are sometimes directly next to each
> > other in the register address space. Let's use regmap bulk read/write
> > APIs to get the data with one transfer instead of multiple i2c
> > transfers. This helps cut down on the number of transfers in the case of
> > something like reading an EDID where we read in blocks of 16 bytes at a
> > time and the last for loop here is sending an i2c transfer for each of
> > those 16 bytes, one at a time. Ouch!
> >
> > Changes in v2:
> >  - Combined AUX_CMD register write
> 
> The change from v1 to v2 makes me slightly nervous, though I guess
> it's fine.  Specifically, all the examples in the datasheet show
> programming the CMD before the ADDR and LEN.  This change will make it
> programmed after.  Since there's a separate START bit I guess it's OK,
> though.  Nothing in the datasheet explicitly says that the order in
> the examples is the only order that will work...

Hmmm now that you mention it the SEND bit is explicitly being cleared in
the programming sequence by being there at the start. If I want to
combine that with the adjacent register writes then I should make sure
that the SEND bit is cleared at the beginning. Otherwise the hardware
may be in the middle of a transaction if the previous transaction is
still running, i.e. a timeout where the SEND bit never cleared.

I think we should go back to the previous patch I had here. Combining
this register write is wrong. If anything, we should clear the SEND bit
on a timeout and make sure during probe that this bit is clear and then
drop the programming of this register from this function entirely. That
would reduce the sequence by one register, but is more complicated vs.
just making sure it has the clear bit here to begin with.

> 
> Reviewed-by: Douglas Anderson <dianders@...omium.org>

Thanks, but I'll send another round picking up acks and such and your
previous review tag on the v1 of this patch.