| 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
| ||
|
Message-ID: <CAPaKu7So4BeEyUGSS3ZAHi+Z=Sh6Kpy_W8eEtX4UQs=__MpPEA@mail.gmail.com>
Date: Fri, 19 Dec 2025 16:45:16 -0800
From: Chia-I Wu <olvaffe@...il.com>
To: Steven Price <steven.price@....com>
Cc: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>,
Boris Brezillon <boris.brezillon@...labora.com>, Liviu Dudau <liviu.dudau@....com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
Ulf Hansson <ulf.hansson@...aro.org>, Chen-Yu Tsai <wenst@...omium.org>, kernel@...labora.com,
dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH 3/4] drm/panthor: Implement reading shader_present from nvmem
On Fri, Dec 19, 2025 at 7:31 AM Steven Price <steven.price@....com> wrote:
>
> On 17/12/2025 17:03, Nicolas Frattaroli wrote:
> > On some platforms, notably MediaTek MT8196, the shader_present bitmask
> > in the Mali GPU register for it has cores enabled that may be faulty.
> > The true shader_present bitmask is found in an efuse instead.
> >
> > Implement reading shader_present from an nvmem cell if one is present,
> > falling back to the Mali register if it's absent. The error codes are
> > trickled up through to the probe function so that probe deferral works.
> >
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
>
> Reviewed-by: Steven Price <steven.price@....com>
>
> [Although I really hope other vendors don't do this - the hardware is
> broken!]
>
> Although one NIT below if you respin for other reasons...
>
> > ---
> > drivers/gpu/drm/panthor/panthor_hw.c | 63 ++++++++++++++++++++++++++++++++----
> > 1 file changed, 57 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
> > index 87ebb7ae42c4..eb44c8b108aa 100644
> > --- a/drivers/gpu/drm/panthor/panthor_hw.c
> > +++ b/drivers/gpu/drm/panthor/panthor_hw.c
> > @@ -1,6 +1,7 @@
> > // SPDX-License-Identifier: GPL-2.0 or MIT
> > /* Copyright 2025 ARM Limited. All rights reserved. */
> >
> > +#include <linux/nvmem-consumer.h>
> > #include <drm/drm_print.h>
> >
> > #include "panthor_device.h"
> > @@ -109,7 +110,52 @@ static char *get_gpu_model_name(struct panthor_device *ptdev)
> > return "(Unknown Mali GPU)";
> > }
> >
> > -static void panthor_gpu_info_init(struct panthor_device *ptdev)
> > +static int overload_shader_present(struct panthor_device *ptdev)
> > +{
> > + struct device *dev = ptdev->base.dev;
> > + struct nvmem_cell *cell = nvmem_cell_get(dev, "shader-present");
> > + ssize_t len;
> > + void *buf;
> > + int ret;
> > +
> > + if (IS_ERR(cell)) {
> > + /* On platforms without this cell, use the Mali register */
> > + if (PTR_ERR(cell) == -ENOENT)
> > + return 0;
> > +
> > + return dev_err_probe(dev, PTR_ERR(cell),
> > + "Failed to get shader-present nvmem cell\n");
> > + }
> > +
> > + buf = nvmem_cell_read(cell, &len);
> > + if (IS_ERR(buf)) {
> > + ret = dev_err_probe(dev, PTR_ERR(buf),
> > + "Failed to read shader-present nvmem cell\n");
> > + goto err_put_cell;
> > + }
> > +
> > + if (!len || len > 8) {
> > + ret = dev_err_probe(dev, -EINVAL, "shader-present cell can't be length %ld\n",
> > + len);
> > + goto err_free;
> > + }
> > +
> > + memcpy(&ptdev->gpu_info.shader_present, buf, len);
When len is 4, this is neither overriding nor masking. I might suggest
switching to nvmem_cell_read_variable_le_u64 for simplicity.
Either way, Reviewed-by: Chia-I Wu <olvaffe@...il.com>
> > +
> > + kfree(buf);
> > + nvmem_cell_put(cell);
> > +
> > + return 0;
> > +
> > +err_free:
> > + kfree(buf);
> > +err_put_cell:
> > + nvmem_cell_put(cell);
> > +
> > + return ret;
> > +}
>
> Rather than repeating the clean up, you can do something like:
>
> {
> void *buf = NULL;
> int ret = 0
>
> if (IS_ERR(cell)) {
> ret = dev_err_probe(...);
> goto out;
> }
>
> buf = nvmem_cell_read();
> if (IS_ERR(buf)) {
> ret = dev_err_probe(...);
> goto out;
> }
>
> if (!len || len > 8) {
> ret = dev_err_probe(...);
> goto out;
> }
>
> memcpy();
>
> out:
> if (!IS_ERR(buf))
> kfree(buf);
> if (!IS_ERR(cell))
> nvmem_cell_put(cell);
>
> return ret;
> }
>
> That avoids mistakes when adding a new operation into the sequence. Or
> you can use the fancy new cleanup helpers, but that feels overkill here.
> But equally I'm ok if you leave the code as is - it's simple enough and
> the conversation can be done later if we need it.
>
> Thanks,
> Steve
>
> > +
> > +static int panthor_gpu_info_init(struct panthor_device *ptdev)
> > {
> > unsigned int i;
> >
> > @@ -143,13 +189,18 @@ static void panthor_gpu_info_init(struct panthor_device *ptdev)
> > ptdev->gpu_info.tiler_present = gpu_read64(ptdev, GPU_TILER_PRESENT);
> > ptdev->gpu_info.l2_present = gpu_read64(ptdev, GPU_L2_PRESENT);
> > }
> > +
> > + return overload_shader_present(ptdev);
> > }
> >
> > -static void panthor_hw_info_init(struct panthor_device *ptdev)
> > +static int panthor_hw_info_init(struct panthor_device *ptdev)
> > {
> > u32 major, minor, status;
> > + int ret;
> >
> > - panthor_gpu_info_init(ptdev);
> > + ret = panthor_gpu_info_init(ptdev);
> > + if (ret)
> > + return ret;
> >
> > major = GPU_VER_MAJOR(ptdev->gpu_info.gpu_id);
> > minor = GPU_VER_MINOR(ptdev->gpu_info.gpu_id);
> > @@ -172,6 +223,8 @@ static void panthor_hw_info_init(struct panthor_device *ptdev)
> > "shader_present=0x%0llx l2_present=0x%0llx tiler_present=0x%0llx",
> > ptdev->gpu_info.shader_present, ptdev->gpu_info.l2_present,
> > ptdev->gpu_info.tiler_present);
> > +
> > + return 0;
> > }
> >
> > static int panthor_hw_bind_device(struct panthor_device *ptdev)
> > @@ -218,7 +271,5 @@ int panthor_hw_init(struct panthor_device *ptdev)
> > if (ret)
> > return ret;
> >
> > - panthor_hw_info_init(ptdev);
> > -
> > - return 0;
> > + return panthor_hw_info_init(ptdev);
> > }
> >
>
Powered by blists - more mailing lists