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

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Mar 13, 2025 at 02:46:11PM +0530, Gupta, Nipun wrote:
> On 12-03-2025 15:48, Greg KH wrote:
> > On Wed, Mar 12, 2025 at 03:24:20PM +0530, Nipun Gupta wrote:
> > > +	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.
> 
> Will create separate entries for each.

Why is any of this needed in sysfs?  Why not just use debugfs as it
looks like debugging information, right?

> > > +	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.
> 
> Agree to some extent, but as Crypto AF_ALG does not support offloading
> asymmetric operations, we added this driver as misc driver. This device is
> supported in AMD Versal series devices: https://www.amd.com/en/products/adaptive-socs-and-fpgas/versal/premium-series.html.
> We would maintain the driver with minimal possible user interface changes.

Please work with the crypto developers to add support for async
operations.  And if that's not going to work out, we need an ack from
them explaining why this driver does not fit into the current framework
and that they are ok with this unique, custom, one-off, totally
out-of-the ordinary, custom userspace-software-requring, api that you
have created here.

thanks,

greg k-h