linux-kernel - Re: [PATCH 3/3] drm/vc4: Correct generation check in vc4_hvs_lut

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: <CAPY8ntDTY-ybd8vViTP4vGuxT8DQsssTY_KJ2EQ6xhk2sV98UA@mail.gmail.com>
Date: Wed, 9 Oct 2024 12:32:21 +0100
From: Dave Stevenson <dave.stevenson@...pberrypi.com>
To: Maíra Canal <mcanal@...lia.com>
Cc: Maxime Ripard <mripard@...nel.org>, 
	Raspberry Pi Kernel Maintenance <kernel-list@...pberrypi.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	Marek Szyprowski <m.szyprowski@...sung.com>, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] drm/vc4: Correct generation check in vc4_hvs_lut_load

Hi Maira

Thanks for the review

On Wed, 9 Oct 2024 at 02:30, Maíra Canal <mcanal@...lia.com> wrote:
>
> Hi Dave,
>
> On 10/8/24 13:44, Dave Stevenson wrote:
> > Commit 24c5ed3ddf27 ("drm/vc4: Introduce generation number enum")
> > incorrectly swapped a check of hvs->vc4->is_vc5 to
> > hvs->vc4->gen == VC4_GEN_4 in vc4_hvs_lut_load, hence breaking
> > loading the gamma look up table on Pi0-3.
> >
> > Correct that conditional.
> >
> > Fixes: 24c5ed3ddf27 ("drm/vc4: Introduce generation number enum")
> > Signed-off-by: Dave Stevenson <dave.stevenson@...pberrypi.com>
> > ---
> >   drivers/gpu/drm/vc4/vc4_hvs.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> > index c5ebc317188a..1edf6e3fa7e6 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> > @@ -224,7 +224,7 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
> >       if (!drm_dev_enter(drm, &idx))
> >               return;
> >
> > -     if (hvs->vc4->gen == VC4_GEN_4)
> > +     if (hvs->vc4->gen != VC4_GEN_4)
>
> Again, another nit: I believe `hvs->vc4->gen > VC4_GEN_4` is more
> semantic and it's the standard I usually see around the driver.

With 24c5ed3ddf27 which this is fixing, all instances in vc4_hvs.c
were changed to == VC4_GEN_4 or == VC4_GEN_5. So I guess == VC4_GEN_5
would have been the better option here.

Yes downstream we now have GEN_6 (which I'm nearly at a point to send
to the list), and I'll need to update this conditional when that's
added.
I'll leave it as is for now, and ensure I've done an audit of all of
them before pushing GEN_6.

Thanks
  Dave

> But this isn't critical, so:
>
> Reviewed-by: Maíra Canal <mcanal@...lia.com>
>
> Best Regards,
> - Maíra
>
> >               goto exit;
> >
> >       /* The LUT memory is laid out with each HVS channel in order,
> >