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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL_Jsq+sEYztJKdD0t8uPwwv1uKk_tac3MqQMUgxfrRjRZmz4A@mail.gmail.com>
Date: Thu, 16 Oct 2025 07:35:06 -0500
From: Rob Herring <robh@...nel.org>
To: Frank Li <Frank.li@....com>
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 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);


Rob