Re: [RFC PATCH 1/2] drivers/misc: add silex multipk driver

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Mar 12, 2025 at 03:24:20PM +0530, Nipun Gupta wrote:
> Silex MultiPK device provides a char device interface to interact
> with Silex device for offloading asymmetric crypto operations.
> 
> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> ---
> 
> Silex MultiPK device is a PKI offload device, which has multiple PK engines to
> perform asymmetric crypto operations like ECDSA, RSA, Point Multiplication etc.
> 
> Following provides the brief overview of the device interface with the software:
> 
>  +------------------+
>  |    Software      |
>  +------------------+
>      |          |
>      |          v
>      |     +-----------------------------------------------------------+
>      |     |                     RAM                                   |
>      |     |  +----------------------------+   +---------------------+ |
>      |     |  |           RQ pages         |   |       CQ pages      | |
>      |     |  | +------------------------+ |   | +-----------------+ | |
>      |     |  | |   START (cmd)          | |   | | req_id | status | | |
>      |     |  | |   TFRI (addr, sz)---+  | |   | | req_id | status | | |
>      |     |  | | +-TFRO (addr, sz)   |  | |   | | ...             | | |
>      |     |  | | | NTFY (req_id)     |  | |   | +-----------------+ | |
>      |     |  | +-|-------------------|--+ |   |                     | |
>      |     |  |   |                   v    |   +---------------------+ |
>      |     |  |   |         +-----------+  |                           |
>      |     |  |   |         | input     |  |                           |
>      |     |  |   |         | data      |  |                           |
>      |     |  |   v         +-----------+  |                           |
>      |     |  |  +----------------+        |                           |
>      |     |  |  |  output data   |        |                           |
>      |     |  |  +----------------+        |                           |
>      |     |  +----------------------------+                           |
>      |     |                                                           |
>      |     +-----------------------------------------------------------+
>      |
>      |
>  +---|----------------------------------------------------+
>  |   v                Silex MultiPK device                |
>  |  +-------------------+     +------------------------+  |
>  |  | New request FIFO  | --> |       PK engines       |  |
>  |  +-------------------+     +------------------------+  |
>  +--------------------------------------------------------+
> 
> To perform a crypto operation, the software writes a sequence of descriptors,
> into the RQ memory. This includes input data and designated location for the
> output data. After preparing the request, request offset (from the RQ memory
> region) is written into the NEW_REQUEST register. Request is then stored in a
> common hardware FIFO shared among all RQs. When a PK engine becomes available,
> device pops the request from the FIFO and fetches the descriptors. It DMAs the
> input data from RQ memory and executes the necessary computations. After
> computation is complete, the device writes output data back to RAM via DMA.
> Device then writes a new entry in CQ ring buffer in RAM, indicating completion
> of the request. Device also generates an interrupt for notifying completion to
> the software.
> 
> As Crypto AF_ALG does not support offloading asymmetric operations from 
> user-space (which was attempted to be added earlier in Linux at:
> https://lore.kernel.org/all/146672253157.23101.15291203749122389409.stgit@tstruk-mobl1.ra.intel.com/),
> RQ memory is provided to the user-space via mmap, so that application can
> directly write to the descriptors.
> 
> P.S. Most of the above text is also added as part of silex driver file.

All of that should be in the changelog text, don't put it down here
where it will never be seen again.

> +static ssize_t hardware_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{

Where is the Documentation/ABI/ entries for your new sysfs files?

> +	int v, hwv, cnt, maxtotalreqs, rqmaxpending, mults;
> +	struct multipk_dev *mpkdev = dev_get_drvdata(dev);
> +
> +	v = (int)sx_rdreg(mpkdev->regs, REG_SEMVER);
> +	hwv = (int)sx_rdreg(mpkdev->regs, REG_HW_VERSION);
> +	cnt = (int)sx_rdreg(mpkdev->regs, REQ_CFG_REQ_QUEUES_CNT);
> +	maxtotalreqs = (int)sx_rdreg(mpkdev->regs, REQ_CFG_MAX_PENDING_REQ);
> +	rqmaxpending = (int)sx_rdreg(mpkdev->regs, REQ_CFG_MAX_REQ_QUEUE_ENTRIES);
> +	mults = (int)sx_rdreg(mpkdev->regs, REQ_CFG_PK_INST);
> +
> +	return sprintf(buf,
> +		"Hardware interface version: %d.%d.%d\n"
> +		"Hardware implementation version: %d.%d.%d\n"
> +		"Count request queues: %d\n"
> +		"Total max pending requests: %d\n"
> +		"Max pending requests per request queue: %d\n"
> +		"Pkcores 64 multipliers: %d\n"
> +		"Pkcores 256 multipliers: %d\n",
> +		MPK_SEMVER_MAJOR(v), MPK_SEMVER_MINOR(v), MPK_SEMVER_PATCH(v),
> +		MPK_HWVER_MAJOR(hwv), MPK_HWVER_MINOR(hwv), MPK_HWVER_SVN(hwv),
> +		cnt, maxtotalreqs, rqmaxpending,
> +		mults >> 16, mults & 0xFFFF);

No!

sysfs is "one value per file", which this is not at all.

Also, did you run checkpatch.pl?  It should have told you that you
should not be calling sprintf() here either.

> +static int __init multipk_init(void)
> +{
> +	dev_t devt;
> +	int ret;
> +
> +	multipk_class = class_create("multipk");

Why do you need a whole new class?  Why not just use a misc device?

> +	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");

Again, why not a dynamic misc device per device in the system?

No need to make this harder than it is.

But again, this really should use the in-kernel apis we already have for
this type of hardware, don't make a custom user/kernel api at all.
That's not going to scale or be easy to maintain for any amount of time.

thanks,

greg k-h