This is a discussion on [PATCH v3] UIO: Implement a UIO interface for the SMX Cryptengine - Kernel ; This patch implements a UIO interface for the SMX Cryptengine.
The Cryptengine found on the Nias Digital SMX board is best suited
for a UIO interface. It is not wired in to the cryptographic API
as the engine handles it's ...

[PATCH v3] UIO: Implement a UIO interface for the SMX Cryptengine

This patch implements a UIO interface for the SMX Cryptengine.

The Cryptengine found on the Nias Digital SMX board is best suited
for a UIO interface. It is not wired in to the cryptographic API
as the engine handles it's own keys, algorithms, everything. All
that we know about is that if there's room in the buffer, you can
write data to it and when there's data ready, you read it out again.

There isn't necessarily even any direct correlation between data
going in and data coming out again, the engine may consume or
generate data all on its own.

This driver is for proprietary hardware but we're always told to
submit the drivers anyway; here you are. :-)

This is version 2 of this patch and addresses all issues raised by
Hans-Jürgen Koch and Paul Mundt in their reviews.

Signed-off-by: Ben Nizette
---
Apologies for the confusion around this patch, I didn't mean to give the
impression that I'd ignored Paul; I am of course grateful for his review
and bug catch. I received the review just a minute after sending out
the last version, made the change locally but didn't immediately resend.
I'll know better next time.

You may as well use DRV_NAME and DRV_VERSION, then you can toss that over
to MODULE_VERSION() if you're so inclined. It's generally nice to keep
modinfo happy.
> + info->irq = platform_get_irq(dev, 0);
> + info->irq_flags = IRQF_SHARED;
> + info->handler = smx_handler;
> +
platform_get_irq() can fail, which you don't presently handle (though I
guess uio_register_device() will error out if you hand off -ENXIO as your
IRQ anyways, so it's not technically an issue). That's a pretty minor
issue, though, but might be something you wish to fix up if you are
planning on using this as an example driver for others.
> + platform_set_drvdata(dev, info);
> +
> + if (uio_register_device(&dev->dev, info))
> + goto out_unmap;
> +
> + return 0;
> +out_unmap:
> + iounmap(info->mem[0].internal_addr);
> +out_free:
> + kfree(info);
> +
> + return -ENODEV;
> +}
> +
Your error paths are also pretty quiet. A dev_err() or so in the few
failure cases you do have might not be a terrible idea.

UIO name and version are intentionally independent of module name and
version. It's meant to be an information for the userspace part of the driver
and should be set to values that are meaningful for it.
>
> > + info->irq = platform_get_irq(dev, 0);
> > + info->irq_flags = IRQF_SHARED;
> > + info->handler = smx_handler;
> > +
> platform_get_irq() can fail, which you don't presently handle (though I
> guess uio_register_device() will error out if you hand off -ENXIO as your
> IRQ anyways, so it's not technically an issue). That's a pretty minor
> issue, though, but might be something you wish to fix up if you are
> planning on using this as an example driver for others.

uio_register_device() will silently not register an interrupt if the number
is negative. This allows us to register a driver without interrupts. Hmm.
Maybe we should explicitly check for UIO_IRQ_NONE there and fail if a
different negative irq is passed in. Thanks for pointing this out.