Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210723135239.388325-1-clement.leger@bootlin.com> <20210723135239.388325-3-clement.leger@bootlin.com>
In-Reply-To: <20210723135239.388325-3-clement.leger@bootlin.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Fri, 23 Jul 2021 18:07:44 +0200
Message-ID: <CAK8P3a1stoM2zRpkEBOLSFRxPy6Tabn6NgO-1sVRL4uq3wudaQ@mail.gmail.com>
Subject: Re: [PATCH 2/3] syscon: add support for "syscon-smc" compatible
To:     =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <clement.leger@bootlin.com>
Cc:     Lee Jones <lee.jones@linaro.org>, Rob Herring <robh+dt@kernel.org>,
        Mark Brown <broonie@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>,
        DTML <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Peng Fan <peng.fan@nxp.com>,
        Sudeep Holla <sudeep.holla@arm.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Fri, Jul 23, 2021 at 3:52 PM Clément Léger <clement.leger@bootlin.com> wrote:
>
> System controllers can be placed under secure monitor control when running
> under them. In order to keep existing code which accesses such system
> controllers using a syscon, add support for "syscon-smc" compatible.
>
> When enable, the syscon will handle this new compatible and look for an
> "arm,smc-id" property to execute the appropriate SMC. A SMC regmap is then
> created to forward register access to the secure monitor.
>
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>

I don't see anything wrong with the implementation, but this worries
me conceptually, because of the ways this might get abused:

- this creates one more way to keep device drivers hidden away
  behind firmware when they should be in the kernel. You can already
  do that with separate SMC calls, but adding an indirection makes it
  sneakier. If the 'registers' in here are purely

- This may be seen as an easy way out for firmware writers that just
   expose a bare register-level interface when the correct solution would
   be to create a high-level interface.

There is also a problem with locking: In the case that both firmware and
kernel have to access registers within a syscon area, you may need to
have a semaphore to protect an atomic sequence of accesses, but since
the interface only provides a single register load/store, there is no way for
a kernel driver to serialize against a firmware-internal driver.

        Arnd