linux-kernel - Re: [PATCH v4 2/2] accel: Add Arm Ethos-U NPU 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] [day] [month] [year] [list]

Message-ID: <aPEFoDrTBxH7Y2FL@lizhi-Precision-Tower-5810>
Date: Thu, 16 Oct 2025 10:48:00 -0400
From: Frank Li <Frank.li@....com>
To: Rob Herring <robh@...nel.org>
Cc: Tomeu Vizoso <tomeu@...euvizoso.net>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Oded Gabbay <ogabbay@...nel.org>,
	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>,
	Sumit Semwal <sumit.semwal@...aro.org>,
	Christian König <christian.koenig@....com>,
	Robin Murphy <robin.murphy@....com>,
	Steven Price <steven.price@....com>,
	Daniel Stone <daniel@...ishbar.org>,
	Sui Jingfeng <sui.jingfeng@...ux.dev>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org
Subject: Re: [PATCH v4 2/2] accel: Add Arm Ethos-U NPU driver

On Thu, Oct 16, 2025 at 07:35:06AM -0500, Rob Herring wrote:
> On Wed, Oct 15, 2025 at 4:02 PM Frank Li <Frank.li@....com> wrote:
> >
> > On Wed, Oct 15, 2025 at 03:36:05PM -0500, Rob Herring wrote:
> > > On Wed, Oct 15, 2025 at 2:39 PM Frank Li <Frank.li@....com> wrote:
> > > >
> > > > On Wed, Oct 15, 2025 at 12:47:40PM -0500, Rob Herring (Arm) wrote:
> > > > > Add a driver for Arm Ethos-U65/U85 NPUs. The Ethos-U NPU has a
> > > > > relatively simple interface with single command stream to describe
> > > > > buffers, operation settings, and network operations. It supports up to 8
> > > > > memory regions (though no h/w bounds on a region). The Ethos NPUs
> > > > > are designed to use an SRAM for scratch memory. Region 2 is reserved
> > > > > for SRAM (like the downstream driver stack and compiler). Userspace
> > > > > doesn't need access to the SRAM.
>
> > > > > +static int ethosu_init(struct ethosu_device *ethosudev)
> > > > > +{
> > > > > +     int ret;
> > > > > +     u32 id, config;
> > > > > +
> > > > > +     ret = devm_pm_runtime_enable(ethosudev->base.dev);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     ret = pm_runtime_resume_and_get(ethosudev->base.dev);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     pm_runtime_set_autosuspend_delay(ethosudev->base.dev, 50);
> > > > > +     pm_runtime_use_autosuspend(ethosudev->base.dev);
> > > > > +
> > > > > +     /* If PM is disabled, we need to call ethosu_device_resume() manually. */
> > > > > +     if (!IS_ENABLED(CONFIG_PM)) {
> > > > > +             ret = ethosu_device_resume(ethosudev->base.dev);
> > > > > +             if (ret)
> > > > > +                     return ret;
> > > > > +     }
> > > >
> > > > I think it should call ethosu_device_resume() unconditional before
> > > > devm_pm_runtime_enable();
> > > >
> > > > ethosu_device_resume();
> > > > pm_runtime_set_active();
> > > > pm_runtime_set_autosuspend_delay(ethosudev->base.dev, 50);
> > > > devm_pm_runtime_enable();
> > >
> > > Why do you think this? Does this do a get?
> > >
> > > I don't think it is good to call the resume hook on our own, but we
> > > have no choice with !CONFIG_PM. With CONFIG_PM, we should only use the
> > > pm_runtime API.
> >
> > Enable clock and do some init work at probe() is quite common. But I never
> > seen IS_ENABLED(CONFIG_PM) check. It is quite weird and not necessary to
> > check CONFIG_PM flags. The most CONFIG_PM is enabled, so the branch !CONFIG_PM
> > almost never tested.
>
> Okay, I get what you meant.
>
> >
> > probe()
> > {
> >         devm_clk_bulk_get_all_enabled();
> >
> >         ... did some init work
> >
> >         pm_runtime_set_active();
> >         devm_pm_runtime_enable();
> >
> >         ...
> >         pm_runtime_put_autosuspend(ethosudev->base.dev);
> > }
>
> I think we still need a pm_runtime_get_noresume() in here since we do
> a put later on. Here's what I have now:
>
>     ret = ethosu_device_resume(ethosudev->base.dev);
>     if (ret)
>         return ret;
>
>     pm_runtime_set_autosuspend_delay(ethosudev->base.dev, 50);
>     pm_runtime_use_autosuspend(ethosudev->base.dev);
>     ret = devm_pm_runtime_set_active_enabled(ethosudev->base.dev);
>     if (ret)
>         return ret;
>     pm_runtime_get_noresume(ethosudev->base.dev);

Look good.

Frank
>
>
> Rob