Re: [PATCH] soc: loongson: add Loongson Security Module driver

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Le 16/11/2024 à 09:57, Qunqin Zhao a écrit :
> This driver supports Loongson Security Module, which
> provides the control for it's hardware encryption
> acceleration child devices.
> 
> Only ACPI firmware is supported.
> 
> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> ---
>   MAINTAINERS                        |   7 +
>   drivers/soc/loongson/Kconfig       |   9 +
>   drivers/soc/loongson/Makefile      |   1 +
>   drivers/soc/loongson/loongson_se.c | 542 +++++++++++++++++++++++++++++
>   include/soc/loongson/se.h          | 135 +++++++
>   5 files changed, 694 insertions(+)
>   create mode 100644 drivers/soc/loongson/loongson_se.c
>   create mode 100644 include/soc/loongson/se.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fdeb3d12c..85fff2eb7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13379,6 +13379,13 @@ S:	Maintained
>   F:	Documentation/devicetree/bindings/hwinfo/loongson,ls2k-chipid.yaml
>   F:	drivers/soc/loongson/loongson2_guts.c
>   
> +LOONGSON SECURITY MODULE DRIVER
> +M:	Qunqin Zhao <zhaoqunqin@loongson.cn>
> +L:	loongarch@lists.linux.dev
> +S:	Maintained
> +F:	drivers/soc/loongson/loongson_se.c
> +F:	include/soc/loongson/se.h
> +
>   LOONGSON-2 SOC SERIES PM DRIVER
>   M:	Yinbo Zhu <zhuyinbo@loongson.cn>
>   L:	linux-pm@vger.kernel.org
> diff --git a/drivers/soc/loongson/Kconfig b/drivers/soc/loongson/Kconfig
> index 368344943..93ef1d205 100644
> --- a/drivers/soc/loongson/Kconfig
> +++ b/drivers/soc/loongson/Kconfig
> @@ -27,3 +27,12 @@ config LOONGSON2_PM
>   	  Disk), ACPI S5 (Soft Shutdown) and supports multiple wake-up methods
>   	  (USB, GMAC, PWRBTN, etc.). This driver was to add power management
>   	  controller support that base on dts for Loongson-2 series SoCs.
> +
> +config LOONGSON_SE
> +	tristate "LOONGSON SECURITY MODULE Interface"
> +	depends on LOONGARCH && ACPI
> +	help
> +	  The Loongson security module provides the control for hardware
> +	  encryption acceleration devices. Each device uses at least one
> +	  channel to interacts with security module, and each channel may

s/interacts/interact/

> +	  has its own buffer provided by security module.

s/has/have/

> diff --git a/drivers/soc/loongson/Makefile b/drivers/soc/loongson/Makefile
> index 4118f50f5..503075042 100644
> --- a/drivers/soc/loongson/Makefile
> +++ b/drivers/soc/loongson/Makefile
> @@ -5,3 +5,4 @@
>   
>   obj-$(CONFIG_LOONGSON2_GUTS)		+= loongson2_guts.o
>   obj-$(CONFIG_LOONGSON2_PM)		+= loongson2_pm.o
> +obj-$(CONFIG_LOONGSON_SE)		+= loongson_se.o
> diff --git a/drivers/soc/loongson/loongson_se.c b/drivers/soc/loongson/loongson_se.c
> new file mode 100644
> index 000000000..d85db8423
> --- /dev/null
> +++ b/drivers/soc/loongson/loongson_se.c

...

> +static int loongson_se_get_res(struct loongson_se *se, u32 int_bit, u32 cmd,
> +			       struct se_data *res)
> +{
> +	int err = 0;
> +
> +	res->int_bit = int_bit;
> +
> +	if (se_get_response(se, res)) {
> +		dev_err(se->dev, "Int 0x%x get response fail.\n", int_bit);
> +		return -EFAULT;
> +	}
> +
> +	/* Check response */
> +	if (res->u.res.cmd == cmd)
> +		err = 0;

Not needed, err is already 0.

> +	else {
> +		dev_err(se->dev, "Response cmd is 0x%x, not expect cmd 0x%x.\n",
> +			res->u.res.cmd, cmd);
> +		err = -EFAULT;
> +	}
> +
> +	return err;
> +}

...

> +static int loongson_se_set_msg(struct lsse_ch *ch)
> +{
> +	struct loongson_se *se = ch->se;
> +	struct se_data req = {0};
> +	struct se_data res = {0};
> +	int err;
> +
> +	req.int_bit = SE_INT_SETUP;
> +	req.u.gcmd.cmd = SE_CMD_SETMSG;
> +	/* MSG off */
> +	req.u.gcmd.info[0] = ch->id;
> +	req.u.gcmd.info[1] = ch->smsg - se->mem_base;
> +	req.u.gcmd.info[2] = ch->msg_size;
> +
> +	dev_dbg(se->dev, "Set Channel %d msg off 0x%x, msg size %d\n",
> +		ch->id, req.u.gcmd.info[1], req.u.gcmd.info[2]);
> +
> +	err = se_send_genl_cmd(se, &req, &res, 5);
> +	if (res.u.res.cmd_ret)

In the fnction above, we test (err || ...)
Is it needed here as well (or can it be removed in se_send_genl_cmd())

Also, below se_send_genl_cmd() errors are checked only with err.
Should it be consistent?

> +		return res.u.res.cmd_ret;
> +
> +	return err;
> +}

...

> +void se_deinit_ch(struct lsse_ch *ch)
> +{
> +	struct loongson_se *se = ch->se;
> +	unsigned long flag;
> +	int first, nr;
> +	int id = ch->id;
> +
> +	if (!se) {
> +		pr_err("SE has bot been initialized\n");
> +		return;
> +	}
> +
> +	if (id == 0 || id > SE_CH_MAX) {
> +		dev_err(se->dev, "Channel number %d is invalid\n", id);
> +		return;
> +	}
> +
> +	if (!se_ch_status(se, BIT(id))) {
> +		dev_err(se->dev, "Channel number %d has not been initialized\n", id);
> +		return;
> +	}
> +
> +	spin_lock_irqsave(&se->dev_lock, flag);
> +
> +	se->ch_status &= ~BIT(ch->id);
> +
> +	first = (ch->data_buffer - se->mem_base) / PAGE_SIZE;
> +	nr = round_up(ch->data_size, PAGE_SIZE) / PAGE_SIZE;
> +	bitmap_clear(se->mem_map, first, nr);
> +
> +	first = (ch->smsg - se->mem_base) / PAGE_SIZE;
> +	nr = round_up(ch->msg_size, PAGE_SIZE) / PAGE_SIZE;
> +	bitmap_clear(se->mem_map, first, nr);
> +
> +	se_disable_int(se, ch->int_bit);
> +
> +	spin_unlock_irqrestore(&se->dev_lock, flag);
> +

Uneeded empty line

> +}
> +EXPORT_SYMBOL_GPL(se_deinit_ch);
> +
> +static int loongson_se_probe(struct platform_device *pdev)
> +{
> +	struct loongson_se *se;
> +	struct device *dev = &pdev->dev;
> +	int nr_irq, irq, err, size;
> +
> +	se = devm_kmalloc(dev, sizeof(*se), GFP_KERNEL);
> +	if (!se)
> +		return -ENOMEM;
> +	se->dev = dev;
> +	dev_set_drvdata(dev, se);
> +	init_completion(&se->cmd_completion);
> +	spin_lock_init(&se->cmd_lock);
> +	spin_lock_init(&se->dev_lock);
> +	/* Setup DMA buffer */
> +	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +	if (device_property_read_u32(dev, "dmam_size", &size))
> +		return -ENODEV;
> +	size = roundup_pow_of_two(size);
> +	se->mem_base = dmam_alloc_coherent(dev, size, &se->mem_addr, GFP_KERNEL);
> +	if (!se->mem_base)
> +		return -ENOMEM;
> +	memset(se->mem_base, 0, size);

I don't think that it is needed. dmam_alloc_coherent() should return 
zeroed memory.

> +	se->mem_map_pages = size / PAGE_SIZE;
> +	se->mem_map = devm_bitmap_zalloc(dev, se->mem_map_pages, GFP_KERNEL);
> +	if (!se->mem_map)
> +		return -ENOMEM;
> +
> +	se->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(se->base))
> +		return PTR_ERR(se->base);
> +
> +	nr_irq = platform_irq_count(pdev);
> +	if (nr_irq <= 0)
> +		return -ENODEV;
> +	while (nr_irq) {
> +		irq = platform_get_irq(pdev, --nr_irq);
> +		if (irq < 0)
> +			return -ENODEV;
> +		/* Use the same interrupt handler address.
> +		 * Determine which irq it is accroding
> +		 * SE_S2LINT_STAT register.
> +		 */
> +		err = devm_request_irq(dev, irq, se_irq, 0,
> +				       "loongson-se", se);
> +		if (err)
> +			dev_err(dev, "failed to request irq: %d\n", err);
> +	}
> +
> +	err = se_init_hw(se, se->mem_addr, size);
> +	if (err)
> +		se_disable_hw(se);
> +
> +	return err;
> +}
> +
> +static struct acpi_device_id loongson_se_acpi_match[] = {

const?

> +	{"LOON0011", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, loongson_se_acpi_match);

...

CJ