Message-ID: <CAAhV-H7nyKHS70BGh7nwjuGwSWayCbUY=1-zWMU4N3bJZtH1gQ@mail.gmail.com>
Date: Thu, 26 Jun 2025 20:48:35 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Jarkko Sakkinen <jarkko@...nel.org>
Cc: Lee Jones <lee@...nel.org>, Qunqin Zhao <zhaoqunqin@...ngson.cn>, herbert@...dor.apana.org.au, 
	linux-kernel@...r.kernel.org, loongarch@...ts.linux.dev, davem@...emloft.net, 
	linux-crypto@...r.kernel.org, peterhuewe@....de, jgg@...pe.ca, 
	linux-integrity@...r.kernel.org, Yinggang Gu <guyinggang@...ngson.cn>, 
	Huacai Chen <chenhuacai@...ngson.cn>
Subject: Re: [PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

On Thu, Jun 26, 2025 at 6:59 PM Jarkko Sakkinen <jarkko@...nel.org> wrote:
>
> On Thu, Jun 26, 2025 at 11:30:30AM +0100, Lee Jones wrote:
> > On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> >
> > > On Wed, Jun 25, 2025 at 02:40:47PM +0100, Lee Jones wrote:
> > > > On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> > > >
> > > > > On Wed, Jun 25, 2025 at 09:05:27AM +0100, Lee Jones wrote:
> > > > > > On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> > > > > >
> > > > > > > On Thu, Jun 19, 2025 at 10:51:37AM +0800, Qunqin Zhao wrote:
> > > > > > > > Loongson Security Engine supports random number generation, hash,
> > > > > > > > symmetric encryption and asymmetric encryption. Based on these
> > > > > > > > encryption functions, TPM2 have been implemented in the Loongson
> > > > > > > > Security Engine firmware. This driver is responsible for copying data
> > > > > > > > into the memory visible to the firmware and receiving data from the
> > > > > > > > firmware.
> > > > > > > >
> > > > > > > > Co-developed-by: Yinggang Gu <guyinggang@...ngson.cn>
> > > > > > > > Signed-off-by: Yinggang Gu <guyinggang@...ngson.cn>
> > > > > > > > Signed-off-by: Qunqin Zhao <zhaoqunqin@...ngson.cn>
> > > > > > > > Reviewed-by: Huacai Chen <chenhuacai@...ngson.cn>
> > > > > > > > ---
> > > > > > > >  drivers/char/tpm/Kconfig        |  9 ++++
> > > > > > > >  drivers/char/tpm/Makefile       |  1 +
> > > > > > > >  drivers/char/tpm/tpm_loongson.c | 84 +++++++++++++++++++++++++++++++++
> > > > > > > >  3 files changed, 94 insertions(+)
> > > > > > > >  create mode 100644 drivers/char/tpm/tpm_loongson.c
> > > > > > > >
> > > > > > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > > > > > index dddd702b2..ba3924eb1 100644
> > > > > > > > --- a/drivers/char/tpm/Kconfig
> > > > > > > > +++ b/drivers/char/tpm/Kconfig
> > > > > > > > @@ -189,6 +189,15 @@ config TCG_IBMVTPM
> > > > > > > >     will be accessible from within Linux.  To compile this driver
> > > > > > > >     as a module, choose M here; the module will be called tpm_ibmvtpm.
> > > > > > > >
> > > > > > > > +config TCG_LOONGSON
> > > > > > > > + tristate "Loongson TPM Interface"
> > > > > > > > + depends on MFD_LOONGSON_SE
> > > > > > > > + help
> > > > > > > > +   If you want to make Loongson TPM support available, say Yes and
> > > > > > > > +   it will be accessible from within Linux. To compile this
> > > > > > > > +   driver as a module, choose M here; the module will be called
> > > > > > > > +   tpm_loongson.
> > > > > > > > +
> > > > > > > >  config TCG_XEN
> > > > > > > >   tristate "XEN TPM Interface"
> > > > > > > >   depends on TCG_TPM && XEN
> > > > > > > > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> > > > > > > > index 9de1b3ea3..5b5cdc0d3 100644
> > > > > > > > --- a/drivers/char/tpm/Makefile
> > > > > > > > +++ b/drivers/char/tpm/Makefile
> > > > > > > > @@ -46,3 +46,4 @@ obj-$(CONFIG_TCG_ARM_CRB_FFA) += tpm_crb_ffa.o
> > > > > > > >  obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
> > > > > > > >  obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
> > > > > > > >  obj-$(CONFIG_TCG_SVSM) += tpm_svsm.o
> > > > > > > > +obj-$(CONFIG_TCG_LOONGSON) += tpm_loongson.o
> > > > > > > > diff --git a/drivers/char/tpm/tpm_loongson.c b/drivers/char/tpm/tpm_loongson.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000..5cbdb37f8
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/char/tpm/tpm_loongson.c
> > > > > > > > @@ -0,0 +1,84 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > +/* Copyright (c) 2025 Loongson Technology Corporation Limited. */
> > > > > > > > +
> > > > > > > > +#include <linux/device.h>
> > > > > > > > +#include <linux/mfd/loongson-se.h>
> > > > > > > > +#include <linux/platform_device.h>
> > > > > > > > +#include <linux/wait.h>
> > > > > > > > +
> > > > > > > > +#include "tpm.h"
> > > > > > > > +
> > > > > > > > +struct tpm_loongson_cmd {
> > > > > > > > + u32 cmd_id;
> > > > > > > > + u32 data_off;
> > > > > > > > + u32 data_len;
> > > > > > > > + u32 pad[5];
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static int tpm_loongson_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> > > > > > > > +{
> > > > > > > > + struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > > > > > > + struct tpm_loongson_cmd *cmd_ret = tpm_engine->command_ret;
> > > > > > > > +
> > > > > > > > + if (cmd_ret->data_len > count)
> > > > > > > > +         return -EIO;
> > > > > > > > +
> > > > > > > > + memcpy(buf, tpm_engine->data_buffer, cmd_ret->data_len);
> > > > > > > > +
> > > > > > > > + return cmd_ret->data_len;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int tpm_loongson_send(struct tpm_chip *chip, u8 *buf, size_t count)
> > > > > > > > +{
> > > > > > > > + struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > > > > > > + struct tpm_loongson_cmd *cmd = tpm_engine->command;
> > > > > > > > +
> > > > > > > > + if (count > tpm_engine->buffer_size)
> > > > > > > > +         return -E2BIG;
> > > > > > > > +
> > > > > > > > + cmd->data_len = count;
> > > > > > > > + memcpy(tpm_engine->data_buffer, buf, count);
> > > > > > > > +
> > > > > > > > + return loongson_se_send_engine_cmd(tpm_engine);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static const struct tpm_class_ops tpm_loongson_ops = {
> > > > > > > > + .flags = TPM_OPS_AUTO_STARTUP,
> > > > > > > > + .recv = tpm_loongson_recv,
> > > > > > > > + .send = tpm_loongson_send,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static int tpm_loongson_probe(struct platform_device *pdev)
> > > > > > > > +{
> > > > > > > > + struct loongson_se_engine *tpm_engine;
> > > > > > > > + struct device *dev = &pdev->dev;
> > > > > > > > + struct tpm_loongson_cmd *cmd;
> > > > > > > > + struct tpm_chip *chip;
> > > > > > > > +
> > > > > > > > + tpm_engine = loongson_se_init_engine(dev->parent, SE_ENGINE_TPM);
> > > > > > > > + if (!tpm_engine)
> > > > > > > > +         return -ENODEV;
> > > > > > > > + cmd = tpm_engine->command;
> > > > > > > > + cmd->cmd_id = SE_CMD_TPM;
> > > > > > > > + cmd->data_off = tpm_engine->buffer_off;
> > > > > > > > +
> > > > > > > > + chip = tpmm_chip_alloc(dev, &tpm_loongson_ops);
> > > > > > > > + if (IS_ERR(chip))
> > > > > > > > +         return PTR_ERR(chip);
> > > > > > > > + chip->flags = TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_IRQ;
> > > > > > > > + dev_set_drvdata(&chip->dev, tpm_engine);
> > > > > > > > +
> > > > > > > > + return tpm_chip_register(chip);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static struct platform_driver tpm_loongson = {
> > > > > > > > + .probe   = tpm_loongson_probe,
> > > > > > > > + .driver  = {
> > > > > > > > +         .name  = "loongson-tpm",
> > > > > > >
> > > > > > > This patch looks otherwise great but I'd prefer here tho use
> > > > > > > "tpm_loongson_probe" for the value of the name field.
> > > > > >
> > > > > > Where does this stipulation come from?  No other driver does this [0].
> > > > > > driver.name should be a nicely formatted, human readable string
> > > > > > describing the name of the device.  Not a function name.
> > > > >
> > > > > What defines "human-readable" here? I see both as somewhat the
> > > > > same level of "readability" ;-)
> > > > >
> > > > > >
> > > > > > [0] git grep -A15 "static struct platform_driver" | grep ".name = .*probe"
> > > > >
> > > > > What I'm getting:
> > > > >
> > > > > $ git grep -l -e platform_driver_register --or -e module_platform_driver
> > > > > drivers/char/tpm | xargs git grep "\.name"
> > > > > drivers/char/tpm/tpm_atmel.c:           .name = "tpm_atmel",
> > > > > drivers/char/tpm/tpm_ftpm_tee.c:                .name = "ftpm-tee",
> > > > > drivers/char/tpm/tpm_ftpm_tee.c:                .name           =
> > > > > "optee-ftpm",
> > > > > drivers/char/tpm/tpm_nsc.c:             .name    = "tpm_nsc",
> > > > > drivers/char/tpm/tpm_svsm.c:            .name = "tpm-svsm",
> > > > > drivers/char/tpm/tpm_tis.c:     .name = "tpm_tis",
> > > > > drivers/char/tpm/tpm_tis.c:             .name           = "tpm_tis",
> > > > > drivers/char/tpm/tpm_tis_synquacer.c:           .name           =
> > > > > "tpm_tis_synquacer",
> > > > >
> > > > > Do you consider e.g, "tpm_tis" as "less human-readable".
> > > > >
> > > > > I don't necessarily fight against the name chosen. Your arguments
> > > > > just plain no make sense, so I just merely want to understand this.
> > > > > That's all.
> > > >
> > > > In 64% of cases '-' is preferred to '_' for device names.
> > > >
> > > > Human readable is probably a bit of a stretch in this context, so I'll
> > > > retract that part of the statement.  However, we should be using device
> > > > names, not names of functions which remain meaningless (which is what I
> > > > really meant by 'readable') to the user.  Where else do you see the
> > > > .probe() function name being used as a device name?
> > >
> > > Oops now I see what you mean. I meant to write "tpm_loongson", i.e.
> > > matching tpm_tis, tpm_crb etc. Sorry my bad.
> >
> > Ah, gotcha.  No worries.
> >
> > "tpm_loongson" wouldn't be my preference, but is acceptable.
>
> It's more like that I'm worried about coherency. There's now bunch
> of convention (looking at grep). So I need to pick one and not
> increase chaos further :-) tpm_* is the preference as it is still
> dominating convention.
But there is another coherency, you can see this in the 1st patch:

+static const struct mfd_cell engines[] = {
+ { .name = "loongson-rng" },
+ { .name = "loongson-tpm" },
+};

Huacai

>
>
> >
> > --
> > Lee Jones [李琼斯]
>
> BR, Jarkko
>

Powered by blists - more mailing lists