| 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
| ||
|
Message-ID: <5e45bf5c-4035-08fe-ef6d-e030941e98e9@amd.com>
Date: Thu, 13 Mar 2025 15:07:09 +0530
From: "Gupta, Nipun" <nipun.gupta@....com>
To: Krzysztof Kozlowski <krzk@...nel.org>, linux-kernel@...r.kernel.org,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
derek.kiernan@....com, dragan.cvetic@....com, arnd@...db.de,
gregkh@...uxfoundation.org
Cc: praveen.jain@....com, harpreet.anand@....com, nikhil.agarwal@....com,
srivatsa@...il.mit.edu, code@...icks.com, ptsm@...ux.microsoft.com
Subject: Re: [RFC PATCH 1/2] drivers/misc: add silex multipk driver
On 12-03-2025 16:24, Krzysztof Kozlowski wrote:
> On 12/03/2025 10:54, Nipun Gupta wrote:
>> +
>> SILICON LABS WIRELESS DRIVERS (for WFxxx series)
>> M: Jérôme Pouiller <jerome.pouiller@...abs.com>
>> S: Supported
>> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
>> index 56bc72c7ce4a..8c5c72c540a6 100644
>> --- a/drivers/misc/Kconfig
>> +++ b/drivers/misc/Kconfig
>> @@ -632,6 +632,12 @@ config MCHP_LAN966X_PCI
>> - lan966x-miim (MDIO_MSCC_MIIM)
>> - lan966x-switch (LAN966X_SWITCH)
>>
>> +config SILEX_MPK
>> + tristate "Silex MultiPK driver"
>> + depends on OF
>
> Why can't this be compile tested?
yes will add the COMPILE_TEST
>
> Which arch uses it? Which hardware?
It is supported on ARM. Will add ARM64 as dependent. It is supported on
AMD versal series devices.
>
>> + help
>> + Enable Silex MultiPK support
>> +
>> source "drivers/misc/c2port/Kconfig"
>> source "drivers/misc/eeprom/Kconfig"
>> source "drivers/misc/cb710/Kconfig"
>> diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
>> index 545aad06d088..456758b29f71 100644
>> --- a/drivers/misc/Makefile
>> +++ b/drivers/misc/Makefile
>> @@ -75,3 +75,4 @@ lan966x-pci-objs := lan966x_pci.o
>> lan966x-pci-objs += lan966x_pci.dtbo.o
>> obj-$(CONFIG_MCHP_LAN966X_PCI) += lan966x-pci.o
>> obj-y += keba/
>> +obj-$(CONFIG_SILEX_MPK) += silex_mpk.o
>> diff --git a/drivers/misc/silex_mpk.c b/drivers/misc/silex_mpk.c
>> new file mode 100644
>> index 000000000000..e03579780761
>> --- /dev/null
>> +++ b/drivers/misc/silex_mpk.c
>> @@ -0,0 +1,860 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018-2021 Silex Insight sa
>> + * Copyright (c) 2018-2021 Beerten Engineering scs
>> + * Copyright (c) 2025 Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/version.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/slab.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
>
> Drop, won't be needed.
>
>> +#include <linux/of_platform.h>
>
> Where do you use it?
>
>
>> +#include <linux/of_address.h>
>
> Where do you use it?
Sure will drop unused include files.
>
>> +#include <linux/cdev.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/io.h>
>> +#include <linux/idr.h>
>> +#include <linux/atomic.h>
>> +#include <linux/delay.h>
>> +#include <linux/eventfd.h>
>> +#include <linux/kthread.h>
>> +#include <linux/sched/signal.h>
>> +#include <linux/bitops.h>
>> +#include <uapi/linux/eventpoll.h>
>> +#include <uapi/misc/silex_mpk.h>
>> +
>> +#include "silex_mpk_defs.h"
>
>
> ...
>
>> +
>> +static int multipk_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct multipk_dev *mpkdev;
>> + struct resource *memres;
>> + int irq, ret;
>> +
>> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
>> + if (ret < 0)
>> + return ret;
>> +
>> + mpkdev = devm_kzalloc(dev, sizeof(*mpkdev), GFP_KERNEL);
>> + if (!mpkdev)
>> + return -ENOMEM;
>> + mpkdev->dev = dev;
>> +
>> + memres = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + mpkdev->regs = devm_ioremap_resource(dev, memres);
>
> Use wrapper for both.
>
>
>> + if (IS_ERR(mpkdev->regs))
>> + return PTR_ERR(mpkdev->regs);
>> + mpkdev->regsphys = memres->start;
>> + memres = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + mpkdev->dbrg_regs = devm_ioremap_resource(dev, memres);
>
> Use wrapper for both.
Will add a wrapper function.
>
>
>> + if (IS_ERR(mpkdev->dbrg_regs))
>> + return PTR_ERR(mpkdev->dbrg_regs);
>> + mpkdev->dbrg_regsphys = memres->start;
>> + platform_set_drvdata(pdev, mpkdev);
>> +
>> + /* Only a single IRQ is supported */
>> + if (platform_irq_count(pdev) != 1)
>> + return -ENODEV;
>> +
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq < 0)
>> + return -ENODEV;
>> +
>> + return multipk_create_device(mpkdev, dev, irq);
>> +}
>> +
>> +static void multipk_remove(struct platform_device *pdev)
>> +{
>> + struct multipk_dev *mpkdev = platform_get_drvdata(pdev);
>> +
>> + multipk_remove_device(mpkdev);
>> +}
>> +
>> +static const struct of_device_id multipk_match[] = {
>> + { .compatible = "multipk" },
>
> NAK, you do not have such compatible.
I will update to use xlnx/multipk as per the device tree (and yaml)
>
>> + { },
>> +};
>> +
>> +static struct platform_driver multipk_pdrv = {
>> + .probe = multipk_probe,
>> + .remove = multipk_remove,
>> + .driver = {
>> + .name = DRIVER_NAME,
>> + .of_match_table = of_match_ptr(multipk_match),
>
> Drop of_match_ptr, you have warnings here.
Sure will drop.
>
>> + },
>> +};
>> +
>> +static int __init multipk_init(void)
>> +{
>> + dev_t devt;
>> + int ret;
>> +
>> + multipk_class = class_create("multipk");
>> + if (IS_ERR(multipk_class)) {
>> + ret = PTR_ERR(multipk_class);
>> + pr_err("can't register class\n");
>> + goto err;
>> + }
>> + ret = alloc_chrdev_region(&devt, 0, MULTIPK_MAX_DEVICES, "multipk");
>> + if (ret) {
>> + pr_err("can't register character device\n");
>> + goto err_class;
>> + }
>> + multipk_major = MAJOR(devt);
>> + multipk_minor = MINOR(devt);
>> +
>> + ret = platform_driver_register(&multipk_pdrv);
>> + if (ret) {
>> + pr_err("can't register platform driver\n");
>> + goto err_unchr;
>> + }
>> +
>> + return 0;
>> +err_unchr:
>> + unregister_chrdev_region(devt, MULTIPK_MAX_DEVICES);
>> +err_class:
>> + class_destroy(multipk_class);
>> +err:
>> + return ret;
>> +}
>> +
>> +static void __exit multipk_exit(void)
>> +{
>> + platform_driver_unregister(&multipk_pdrv);
>> +
>> + unregister_chrdev_region(MKDEV(multipk_major, 0), MULTIPK_MAX_DEVICES);
>> +
>> + class_destroy(multipk_class);
>> +}
>> +
>> +module_init(multipk_init);
>> +module_exit(multipk_exit);
>
> I don't understand why loading a standard driver creates all this on my
> device.
Will move it to probe rather than driver init.
>
>> +
>> +MODULE_DESCRIPTION("Driver for Silex Multipk Asymmetric crypto accelerator");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:" DRIVER_NAME);
> Drop
Okay.
Thanks,
Nipun
>
> Best regards,
> Krzysztof
Powered by blists - more mailing lists