linux-kernel - Re: [PATCH v7 3/6] drivers/auxdisplay: sensehat: Raspberry Pi Sense HAT display driver

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:   Thu, 3 Mar 2022 21:19:35 +0100
From:   Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:     Charles Mirabile <cmirabil@...hat.com>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Serge Schneider <serge@...pberrypi.org>,
        Stefan Wahren <stefan.wahren@...e.com>,
        Nicolas Saenz Julienne <nsaenzju@...hat.com>,
        Mattias Brugger <mbrugger@...e.com>,
        linux-rpi-kernel@...ts.infradead.org,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        fedora-rpi@...glegroups.com, Miguel Ojeda <ojeda@...nel.org>,
        Daniel Bauman <dbauman@...hat.com>,
        Mwesigwa Guma <mguma@...hat.com>,
        Joel Savitz <jsavitz@...hat.com>
Subject: Re: [PATCH v7 3/6] drivers/auxdisplay: sensehat: Raspberry Pi Sense
 HAT display driver

On Thu, Mar 3, 2022 at 6:40 PM Charles Mirabile <cmirabil@...hat.com> wrote:
>
> +       u8 temp[8][3][8];
> +
> +       for (i = 0; i < 8; ++i) {
> +               for (j = 0; j < 8; ++j)
> +                       temp[i][0][j] = display->vmem[i][j].r;
> +               for (j = 0; j < 8; ++j)
> +                       temp[i][1][j] = display->vmem[i][j].g;
> +               for (j = 0; j < 8; ++j)
> +                       temp[i][2][j] = display->vmem[i][j].b;
> +       }

I assume the hardware uses only rgb555, right? I think this is OK, but
please consider if it would make sense to read a 8x8x3 bytes matrix
anyway, even if some bits/values are ignored/discarded.

> +       struct sensehat_display *sensehat_display =
> +               devm_kmalloc(&pdev->dev, sizeof(*sensehat_display), GFP_KERNEL);

Missing some error handling, e.g. here and in `dev_get_regmap`.

Also, if `sensehat_update_display` could return an optional error, you
could also fail if the first update did not work (assuming it is
supposed to work), or maybe warn (if it can fail for some reason but
it does not mean future updates would fail).

> +MODULE_AUTHOR("Serge Schneider <serge@...pberrypi.org>");

Please consider if you should/want to appear here alongside Serge.

Cheers,
Miguel