Re: [PATCH v2 2/3] accel/amdpk: add driver for AMD PKI accelerator

Krzysztof Kozlowski
2025-04-10T07:36:20Z

On Wed, Apr 09, 2025 at 11:00:32PM GMT, Nipun Gupta wrote:
> The AMD PKI accelerator driver provides a accel interface to interact
> with the device for offloading and accelerating asymmetric crypto
> operations.
>

For me this is clearly a crypto driver and you are supposed to:
1. Cc crypto maintaners,
2. Put it actually into crypto and use crypto API.

Limited review follows.

> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> ---
>
> Changes RFC->v2:
> - moved from misc to accel
> - added architecture and compile test dependency in Kconfig
> - removed sysfs (and added debugfs in new patch 3/3)
> - fixed platform compat
> - removed redundant resource index 1 configuration (which was there in
> RFC patch)
>
> MAINTAINERS | 2 +
> drivers/accel/Kconfig | 1 +
> drivers/accel/Makefile | 1 +
> drivers/accel/amdpk/Kconfig | 18 +
> drivers/accel/amdpk/Makefile | 8 +
> drivers/accel/amdpk/amdpk_drv.c | 736 ++++++++++++++++++++++++++++++++
> drivers/accel/amdpk/amdpk_drv.h | 271 ++++++++++++
> include/uapi/drm/amdpk.h | 49 +++
> 8 files changed, 1086 insertions(+)
> create mode 100644 drivers/accel/amdpk/Kconfig
> create mode 100644 drivers/accel/amdpk/Makefile
> create mode 100644 drivers/accel/amdpk/amdpk_drv.c
> create mode 100644 drivers/accel/amdpk/amdpk_drv.h
> create mode 100644 include/uapi/drm/amdpk.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 11f8815daa77..cdc305a206aa 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1161,6 +1161,8 @@ L: dri-devel@lists.freedesktop.org
> S: Maintained
> T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
> F: Documentation/devicetree/bindings/accel/amd,versal-net-pki.yaml
> +F: drivers/accel/amdpk/
> +F: include/uapi/drm/amdpk.h
>
> AMD PMC DRIVER
> M: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> diff --git a/drivers/accel/Kconfig b/drivers/accel/Kconfig
> index 5b9490367a39..5632c6c62c15 100644
> --- a/drivers/accel/Kconfig
> +++ b/drivers/accel/Kconfig
> @@ -28,5 +28,6 @@ source "drivers/accel/amdxdna/Kconfig"
> source "drivers/accel/habanalabs/Kconfig"
> source "drivers/accel/ivpu/Kconfig"
> source "drivers/accel/qaic/Kconfig"
> +source "drivers/accel/amdpk/Kconfig"

Why placing at the end?

>
> endif
> diff --git a/drivers/accel/Makefile b/drivers/accel/Makefile
> index a301fb6089d4..caea6d636ac8 100644
> --- a/drivers/accel/Makefile
> +++ b/drivers/accel/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_DRM_ACCEL_AMDXDNA) += amdxdna/
> obj-$(CONFIG_DRM_ACCEL_HABANALABS) += habanalabs/
> obj-$(CONFIG_DRM_ACCEL_IVPU) += ivpu/
> obj-$(CONFIG_DRM_ACCEL_QAIC) += qaic/
> +obj-$(CONFIG_DRM_ACCEL_AMDPK) += amdpk/

Did you just add it to the end breaking any ordering? Look, there is
already AMD entry in the context.

> diff --git a/drivers/accel/amdpk/Kconfig b/drivers/accel/amdpk/Kconfig
> new file mode 100644
> index 000000000000..c0b459bb66a7
> --- /dev/null
> +++ b/drivers/accel/amdpk/Kconfig
> @@ -0,0 +1,18 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Makefile for AMD PKI accelerator for versal-net
> +#
> +
> +config DRM_ACCEL_AMDPK
> + tristate "AMD PKI accelerator for versal-net"
> + depends on DRM_ACCEL
> + depends on ARM64 || COMPILE_TEST

What do you need from arm64? I don't see it from the code at all.

> + help
> + Enables platform driver for AMD PKI accelerator that are designed
> + for high performance Public Key asymmetric crypto operations on AMD
> + versal-net.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called amdpk.

...

> +static void amdpk_remove_device(struct amdpk_dev *pkdev)
> +{
> + drm_dev_unplug(&pkdev->ddev);
> + pk_wrreg(pkdev->regs, REG_IRQ_ENABLE, 0);
> + ida_destroy(&pkdev->avail_queues);
> +}
> +
> +static int amdpk_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct amdpk_dev *pkdev;
> + struct resource *memres;
> + int irq, ret;
> +
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> + if (ret < 0)
> + return ret;
> +
> + pkdev = devm_drm_dev_alloc(dev, &amdpk_accel_driver, typeof(*pkdev), ddev);
> + if (IS_ERR(pkdev))
> + return PTR_ERR(pkdev);
> + pkdev->dev = dev;
> +
> + memres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + pkdev->regs = devm_ioremap_resource(dev, memres);

Use wrapper for these two.

> + if (IS_ERR(pkdev->regs))
> + return PTR_ERR(pkdev->regs);
> + pkdev->regsphys = memres->start;
> + platform_set_drvdata(pdev, pkdev);
> +
> + if (platform_irq_count(pdev) != 1)

Drop, what's the benefit? DT schema ensures you have only one entry.

> + return -ENODEV;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return -ENODEV;
> +
> + ret = drm_dev_register(&pkdev->ddev, 0);
> + if (ret) {
> + dev_err(&pdev->dev, "DRM register failed, ret %d", ret);
> + return ret;
> + }
> +
> + return amdpk_create_device(pkdev, dev, irq);
> +}
> +
> +static void amdpk_remove(struct platform_device *pdev)
> +{
> + struct amdpk_dev *pkdev = platform_get_drvdata(pdev);
> +
> + amdpk_remove_device(pkdev);
> +}
> +
> +static void amdpk_shutdown(struct platform_device *pdev)
> +{
> + amdpk_remove(pdev);

I am not sure why this is necessary. Please provide comment WHY you
need it. IOW, why do you need to disable IRQ manually here if the entire
system is shutting down?

> +}
> +
> +static const struct of_device_id amdpk_match_table[] = {
> + { .compatible = "amd,versal-net-pki" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, amdpk_match_table);
> +
> +static struct platform_driver amdpk_pdrv = {
> + .probe = amdpk_probe,
> + .remove = amdpk_remove,
> + .shutdown = amdpk_shutdown,
> + .driver = {
> + .name = DRIVER_NAME,
> + .of_match_table = amdpk_match_table,
> + },
> +};
> +
> +static int __init amdpk_init(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&amdpk_pdrv);
> + if (ret) {
> + pr_err("can't register platform driver\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void __exit amdpk_exit(void)
> +{
> + platform_driver_unregister(&amdpk_pdrv);
> +}
> +
> +module_init(amdpk_init);
> +module_exit(amdpk_exit);

Why do you need to open code module_platform_driver?

Best regards,
Krzysztof