linux-kernel - Re: [PATCH v5 2/2] drm/bridge: parade-ps8640: Add support for AUX channel

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: <CAD=FV=VgQWLmPEFBv=Ufnm8Gc4srRUd15GNbSrL-pYBGysCYqw@mail.gmail.com>
Date:   Tue, 21 Sep 2021 09:02:20 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Philip Chen <philipchen@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Jonas Karlman <jonas@...boo.se>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Robert Foss <robert.foss@...aro.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v5 2/2] drm/bridge: parade-ps8640: Add support for AUX channel

Hi,

On Sat, Sep 18, 2021 at 10:21 AM Philip Chen <philipchen@...omium.org> wrote:
>
> +static ssize_t ps8640_aux_transfer(struct drm_dp_aux *aux,
> +                                  struct drm_dp_aux_msg *msg)
> +{
> +       struct ps8640 *ps_bridge = aux_to_ps8640(aux);
> +       struct regmap *map = ps_bridge->regmap[PAGE0_DP_CNTL];
> +       struct device *dev = &ps_bridge->page[PAGE0_DP_CNTL]->dev;
> +
> +       unsigned int len = msg->size;

nit: usually no blank lines in the variable definition section.

> +       base = PAGE0_SWAUX_ADDR_7_0;
> +       addr_len[PAGE0_SWAUX_ADDR_7_0 - base] = msg->address;
> +       addr_len[PAGE0_SWAUX_ADDR_15_8 - base] = msg->address >> 8;
> +       addr_len[PAGE0_SWAUX_ADDR_23_16 - base] = (msg->address >> 16) &
> +                                                 SWAUX_ADDR_19_16_MASK;
> +       addr_len[PAGE0_SWAUX_ADDR_23_16 - base] |= (msg->request << 4) &
> +                                                  SWAUX_CMD_MASK;

optional nit: Probably you could get rid of the mask for the request.
After all, you're storing it to a thing that's a byte (so bits above
bit 7 will implicitly be masked) and you're left shifting by 4 (so
bits 0-3 will implicitly be masked) so this just makes it uglier. ;-)

optional nit: In theory you could also get rid of the
SWAUX_ADDR_19_16_MASK and if you really wanted to you could error
check that the address wasn't bigger than 20-bits since giving an
error for an invalid address would actually be better than silently
masking it anyway...

> +       if (len && (request == DP_AUX_NATIVE_READ ||
> +                   request == DP_AUX_I2C_READ)) {
> +               /* Read from the internal FIFO buffer */
> +               for (i = 0; i < len; i++) {
> +                       ret = regmap_read(map, PAGE0_SWAUX_RDATA,
> +                                         (unsigned int *)(buf + i));

The cast to "unsigned int *" looks wrong to me. You can't just cast
like this for a number of reasons. Go back to reading into a local
variable and copy the byte into your buffer.

Other than the regmap_read() this looks fine to me. If you send a v6
with that fixed I'll plan to wait a day or two and then apply it with
Sam's tags.

-Doug