Re: [RFC PATCH 1/3] crypto: zip - Add ThunderX ZIP driver core
On Mon, Dec 12, 2016 at 10:04 AM, Jan Glauber wrote:
> +/* error messages */
> +#define zip_err(fmt, args...) pr_err("ZIP ERR:%s():%d: " \
> + fmt "\n", __func__, __LINE__, ## args)
> +
> +#ifdef MSG_ENABLE
> +/* Enable all messages */
> +#define zip_msg(fmt, args...) pr_info("ZIP_MSG:" fmt "\n", ## args)
> +#else
> +#define zip_msg(fmt, args...)
> +#endif
> +
> +#if defined(ZIP_DEBUG_ENABLE) && defined(MSG_ENABLE)
> +
> +#ifdef DEBUG_LEVEL
> +
> +#define FILE_NAME (strrchr(__FILE__, '/') ? strrchr(__FILE__, '/') + 1 : \
> + strrchr(__FILE__, '\\') ? strrchr(__FILE__, '\\') + 1 : __FILE__)
> +
> +#if DEBUG_LEVEL >= 4
> +
> +#define zip_dbg(fmt, args...) pr_info("ZIP DBG: %s: %s() : %d: " \
> + fmt "\n", FILE_NAME, __func__, __LINE__, ##
> args)
> +
> +#define zip_dbg_enter(fmt, args...) pr_info("ZIP_DBG: %s() in %s" \
> + fmt "\n", __func__, FILE_NAME, ## args)
> +
> +#define zip_dbg_exit(fmt, args...) pr_info("ZIP_DBG:Exit %s() in %s" \
> + fmt "\n", __func__, FILE_NAME, ## args)
> +
> +#elif DEBUG_LEVEL >= 3
> +
> +#define zip_dbg(fmt, args...) pr_info("ZIP DBG: %s: %s() : %d: " \
> + fmt "\n", FILE_NAME, __func__, __LINE__, ##
> args)
> +
> +#elif DEBUG_LEVEL >= 2
> +
> +#define zip_dbg(fmt, args...) pr_info("ZIP DBG: %s() : %d: " \
> + fmt "\n", __func__, __LINE__, ## args)
> +
> +#else
> +
> +#define zip_dbg(fmt, args...) pr_info("ZIP DBG:" fmt "\n", ## args)
> +
> +#endif /* DEBUG LEVEL >= */
> +
> +#if DEBUG_LEVEL <= 3
> +
> +#define zip_dbg_enter(fmt, args...)
> +#define zip_dbg_exit(fmt, args...)
> +
> +#endif /* DEBUG_LEVEL <= 3 */
> +#else
> +
> +#define zip_dbg(fmt, args...) pr_info("ZIP DBG:" fmt "\n", ## args)
> +
> +#define zip_dbg_enter(fmt, args...)
> +#define zip_dbg_exit(fmt, args...)
> +
> +#endif /* DEBUG_LEVEL */
> +#else
> +
> +#define zip_dbg(fmt, args...)
> +#define zip_dbg_enter(fmt, args...)
> +#define zip_dbg_exit(fmt, args...)
> +
> +#endif /* ZIP_DEBUG_ENABLE */
The whole zip_dbg_[enter,exit] thing can be just done with tracepoints
instead of reinventing the wheel. No reason to carry that code
> +u32 zip_load_instr(union zip_inst_s *instr,
> + struct zip_device *zip_dev)
> +{
> + union zip_quex_doorbell dbell;
> + u32 queue = 0;
> + u32 consumed = 0;
> + u64 *ncb_ptr = NULL;
> + union zip_nptr_s ncp;
> +
> + /*
> +* Distribute the instructions between the enabled queues based on
> +* the CPU id.
> +*/
> + if (raw_smp_processor_id() % 2 == 0)
> + queue = 0;
> + else
> + queue = 1;
Is this just simplistic load balancing scheme? There's no guarantee
that the cpu won't change later on.
> +
> + /* Poll for the IQ cmd completion code */
> + zip_dbg_exit();
The comment doesn't match with what actually happens?
> +static inline u64 zip_depth(void)
> +{
> + struct zip_device *zip_dev = zip_get_device(zip_get_node_id());
> +
> + if (!zip_dev)
> + return -ENODEV;
> +
> + return zip_dev->depth;
> +}
> +
> +static inline u64 zip_onfsize(void)
> +{
> + struct zip_device *zip_dev = zip_get_device(zip_get_node_id());
> +
> + if (!zip_dev)
> + return -ENODEV;
> +
> + return zip_dev->onfsize;
> +}
> +
> +static inline u64 zip_ctxsize(void)
> +{
> + struct zip_device *zip_dev = zip_get_device(zip_get_node_id());
> +
> + if (!zip_dev)
> + return -ENODEV;
> +
> + return zip_dev->ctxsize;
> +}
Where is it all used?
> +/*
> + * Allocates new ZIP device structure
> + * Returns zip_device pointer or NULL if cannot allocate memory for
> zip_device
> + */
> +static struct zip_device *zip_alloc_device(struct pci_dev *pdev)
> +{
> + struct zip_device *zip = NULL;
> + int idx = 0;
> +
> + for (idx = 0; idx < MAX_ZIP_DEVICES; idx++) {
> + if (!zip_dev[idx])
> + break;
> + }
> +
> + zip = kzalloc(sizeof(*zip), GFP_KERNEL);
> +
> + if (!zip)
> + return NULL;
> +
> + zip_dev[idx] = zip;
If for some odd reason we try to allocate more than MAX_ZIP_DEVICES
this will deref an invalid pointer.
> +/**
> + * zip_get_device - Get ZIP device based on node id of cpu
> + *
> + * @node: Node id of the current cpu
> + * Return: Pointer to Zip device structure
> + */
> +struct zip_device *zip_get_device(int node)
> +{
> + if ((node < MAX_ZIP_DEVICES) && (node >= 0))
> + return zip_dev[node];
> +
> + zip_err("ZIP device not found for node id %d\n", node);
> + return NULL;
> +}
> +
> +/**
> + * zip_get_node_id - Get the node id of the current cpu
> + *
> + * Return: Node id of the current cpu
> + */
> +int zip_get_node_id(void)
> +{
> + return cpu_to_node(raw_smp_processor_id());
>
Re: [RFC PATCH 1/3] crypto: zip - Add ThunderX ZIP driver core
Hi Corentin,
thanks for your review! Your comments all look reasonable to me,
Mahipal will address them.
Since I posted this series at the beginning of the merge window
I'd like to wait for some more time before we post an updated version.
--Jan
On Tue, Dec 13, 2016 at 02:39:00PM +0100, Corentin Labbe wrote:
> Hello
>
> I have some comment below
>
> On Mon, Dec 12, 2016 at 04:04:37PM +0100, Jan Glauber wrote:
> > From: Mahipal Challa
> >
> [...]
> > --- a/drivers/crypto/Makefile
> > +++ b/drivers/crypto/Makefile
> > @@ -27,6 +27,7 @@ obj-$(CONFIG_CRYPTO_DEV_MXC_SCC) += mxc-scc.o
> > obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o
> > obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/
> > obj-$(CONFIG_CRYPTO_DEV_QAT) += qat/
> > +obj-$(CONFIG_CRYPTO_DEV_CAVIUM_ZIP) += cavium/
> > obj-$(CONFIG_CRYPTO_DEV_QCE) += qce/
> > obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
> > obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
>
> Try to keep some alphabetical order
>
> [...]
> > +/* ZIP invocation result completion status codes */
> > +#define ZIP_NOTDONE0x0
> > +
> > +/* Successful completion. */
> > +#define ZIP_SUCCESS0x1
> > +
> > +/* Output truncated */
> > +#define ZIP_DTRUNC 0x2
> > +
> > +/* Dynamic Stop */
> > +#define ZIP_DYNAMIC_STOP 0x3
> > +
> > +/* Uncompress ran out of input data when IWORD0[EF] was set */
> > +#define ZIP_ITRUNC 0x4
> > +
> > +/* Uncompress found the reserved block type 3 */
> > +#define ZIP_RBLOCK 0x5
> > +
> > +/* Uncompress found LEN != ZIP_NLEN in an uncompressed block in the input
> > */
> > +#define ZIP_NLEN 0x6
> > +
> > +/* Uncompress found a bad code in the main Huffman codes. */
> > +#define ZIP_BADCODE0x7
> > +
> > +/* Uncompress found a bad code in the 19 Huffman codes encoding lengths. */
> > +#define ZIP_BADCODE2 0x8
> > +
> > +/* Compress found a zero-length input. */
> > +#define ZIP_ZERO_LEN 0x9
> > +
> > +/* The compress or decompress encountered an internal parity error. */
> > +#define ZIP_PARITY 0xA
>
> Perhaps all errors could begin with ZIP_ERR_xxx ?
>
> [...]
> > +static inline u64 zip_depth(void)
> > +{
> > + struct zip_device *zip_dev = zip_get_device(zip_get_node_id());
> > +
> > + if (!zip_dev)
> > + return -ENODEV;
> > +
> > + return zip_dev->depth;
> > +}
>
> This function is not used.
>
> > +
> > +static inline u64 zip_onfsize(void)
> > +{
> > + struct zip_device *zip_dev = zip_get_device(zip_get_node_id());
> > +
> > + if (!zip_dev)
> > + return -ENODEV;
> > +
> > + return zip_dev->onfsize;
> > +}
>
> Same for this
>
> > +
> > +static inline u64 zip_ctxsize(void)
> > +{
> > + struct zip_device *zip_dev = zip_get_device(zip_get_node_id());
> > +
> > + if (!zip_dev)
> > + return -ENODEV;
> > +
> > + return zip_dev->ctxsize;
> > +}
>
> Again
>
> [...]
> > +
> > +/*
> > + * Allocates new ZIP device structure
> > + * Returns zip_device pointer or NULL if cannot allocate memory for
> > zip_device
> > + */
> > +static struct zip_device *zip_alloc_device(struct pci_dev *pdev)
>
> pdev is not used, so you can remove it from arglist.
> Or keep it and use devm_kzalloc for allocating zip
>
> > +{
> > + struct zip_device *zip = NULL;
> > + int idx = 0;
> > +
> > + for (idx = 0; idx < MAX_ZIP_DEVICES; idx++) {
> > + if (!zip_dev[idx])
> > + break;
> > + }
> > +
> > + zip = kzalloc(sizeof(*zip), GFP_KERNEL);
> > +
> > + if (!zip)
> > + return NULL;
> > +
> > + zip_dev[idx] = zip;
> > + zip->index = idx;
> > + return zip;
> > +}
>
> [...]
> > +static int zip_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct zip_device *zip = NULL;
> > + interr;
> > +
> > + zip_dbg_enter();
> > +
> > + zip = zip_alloc_device(pdev);
> > +
> > + if (!zip)
> > + return -ENOMEM;
> > +
> > + pr_info("Found ZIP device %d %x:%x on Node %d\n", zip->index,
> > + pdev->vendor, pdev->device, dev_to_node(dev));
>
> You use lots of pr_info, why not using more dev_info/dev_err ?
>
> > +
> > + zip->pdev = pdev;
> > +
> > + pci_set_drvdata(pdev, zip);
> > +
> > + err = pci_enable_device(pdev);
> > + if (err) {
> > + zip_err("Failed to enable PCI device");
> > + goto err_free_device;
> > + }
> > +
> > + err = pci_request_regions(pdev, DRV_NAME);
> > + if (err) {
> > + zip_err("PCI request regions failed 0x%x", err);
> > + goto err_disable_device;
> > + }
> > +
> > + err = pci_set_dma_mask(pdev, DMA_BIT_MASK(48));
> > + if (err) {
> > + dev_err(dev, "Unable to get usable DMA configuration\n");
> > + goto err_release_regions;
> > + }
> > +
> > + err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(48));
> > + if (err) {
> > + dev_err(dev, "Unable to get 48-bit DMA for allocati
Re: [RFC PATCH 1/3] crypto: zip - Add ThunderX ZIP driver core
Hello
I have some comment below
On Mon, Dec 12, 2016 at 04:04:37PM +0100, Jan Glauber wrote:
> From: Mahipal Challa
>
[...]
> --- a/drivers/crypto/Makefile
> +++ b/drivers/crypto/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_CRYPTO_DEV_MXC_SCC) += mxc-scc.o
> obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o
> obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/
> obj-$(CONFIG_CRYPTO_DEV_QAT) += qat/
> +obj-$(CONFIG_CRYPTO_DEV_CAVIUM_ZIP) += cavium/
> obj-$(CONFIG_CRYPTO_DEV_QCE) += qce/
> obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/
> obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/
Try to keep some alphabetical order
[...]
> +/* ZIP invocation result completion status codes */
> +#define ZIP_NOTDONE 0x0
> +
> +/* Successful completion. */
> +#define ZIP_SUCCESS 0x1
> +
> +/* Output truncated */
> +#define ZIP_DTRUNC 0x2
> +
> +/* Dynamic Stop */
> +#define ZIP_DYNAMIC_STOP 0x3
> +
> +/* Uncompress ran out of input data when IWORD0[EF] was set */
> +#define ZIP_ITRUNC 0x4
> +
> +/* Uncompress found the reserved block type 3 */
> +#define ZIP_RBLOCK 0x5
> +
> +/* Uncompress found LEN != ZIP_NLEN in an uncompressed block in the input */
> +#define ZIP_NLEN 0x6
> +
> +/* Uncompress found a bad code in the main Huffman codes. */
> +#define ZIP_BADCODE 0x7
> +
> +/* Uncompress found a bad code in the 19 Huffman codes encoding lengths. */
> +#define ZIP_BADCODE2 0x8
> +
> +/* Compress found a zero-length input. */
> +#define ZIP_ZERO_LEN 0x9
> +
> +/* The compress or decompress encountered an internal parity error. */
> +#define ZIP_PARITY 0xA
Perhaps all errors could begin with ZIP_ERR_xxx ?
[...]
> +static inline u64 zip_depth(void)
> +{
> + struct zip_device *zip_dev = zip_get_device(zip_get_node_id());
> +
> + if (!zip_dev)
> + return -ENODEV;
> +
> + return zip_dev->depth;
> +}
This function is not used.
> +
> +static inline u64 zip_onfsize(void)
> +{
> + struct zip_device *zip_dev = zip_get_device(zip_get_node_id());
> +
> + if (!zip_dev)
> + return -ENODEV;
> +
> + return zip_dev->onfsize;
> +}
Same for this
> +
> +static inline u64 zip_ctxsize(void)
> +{
> + struct zip_device *zip_dev = zip_get_device(zip_get_node_id());
> +
> + if (!zip_dev)
> + return -ENODEV;
> +
> + return zip_dev->ctxsize;
> +}
Again
[...]
> +
> +/*
> + * Allocates new ZIP device structure
> + * Returns zip_device pointer or NULL if cannot allocate memory for
> zip_device
> + */
> +static struct zip_device *zip_alloc_device(struct pci_dev *pdev)
pdev is not used, so you can remove it from arglist.
Or keep it and use devm_kzalloc for allocating zip
> +{
> + struct zip_device *zip = NULL;
> + int idx = 0;
> +
> + for (idx = 0; idx < MAX_ZIP_DEVICES; idx++) {
> + if (!zip_dev[idx])
> + break;
> + }
> +
> + zip = kzalloc(sizeof(*zip), GFP_KERNEL);
> +
> + if (!zip)
> + return NULL;
> +
> + zip_dev[idx] = zip;
> + zip->index = idx;
> + return zip;
> +}
[...]
> +static int zip_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> + struct device *dev = &pdev->dev;
> + struct zip_device *zip = NULL;
> + interr;
> +
> + zip_dbg_enter();
> +
> + zip = zip_alloc_device(pdev);
> +
> + if (!zip)
> + return -ENOMEM;
> +
> + pr_info("Found ZIP device %d %x:%x on Node %d\n", zip->index,
> + pdev->vendor, pdev->device, dev_to_node(dev));
You use lots of pr_info, why not using more dev_info/dev_err ?
> +
> + zip->pdev = pdev;
> +
> + pci_set_drvdata(pdev, zip);
> +
> + err = pci_enable_device(pdev);
> + if (err) {
> + zip_err("Failed to enable PCI device");
> + goto err_free_device;
> + }
> +
> + err = pci_request_regions(pdev, DRV_NAME);
> + if (err) {
> + zip_err("PCI request regions failed 0x%x", err);
> + goto err_disable_device;
> + }
> +
> + err = pci_set_dma_mask(pdev, DMA_BIT_MASK(48));
> + if (err) {
> + dev_err(dev, "Unable to get usable DMA configuration\n");
> + goto err_release_regions;
> + }
> +
> + err = pci_set_consistent_dma_mask(pdev, DMA_BIT_MASK(48));
> + if (err) {
> + dev_err(dev, "Unable to get 48-bit DMA for allocations\n");
> + goto err_release_regions;
> + }
> +
> + /* MAP configuration registers */
> + zip->reg_base = pci_ioremap_bar(pdev, PCI_CFG_ZIP_PF_BAR0);
> + if (!zip->reg_base) {
> + zip_err("ZIP: Cannot map BAR0 CSR memory space, aborting");
> + err = -ENOMEM;
> + goto err_release_regions;
> + }
> +
> + /* Initialize ZIP Hardware */
> + err = zip_init_hw(zip);
> + if (err)
> + goto err_release_regions;
> +
> + return 0;
> +
> +err_release_regions:
> + if (zip->reg_base)
>
[RFC PATCH 1/3] crypto: zip - Add ThunderX ZIP driver core
From: Mahipal Challa Add a driver for the ZIP engine found on Cavium ThunderX SOCs. The ZIP engine supports hardware accelerated compression and decompression. It includes 2 independent ZIP cores and supports: - DEFLATE compression and decompression (RFC 1951) - LZS compression and decompression (RFC 2395 and ANSI X3.241-1994) - ADLER32 and CRC32 checksums for ZLIB (RFC 1950) and GZIP (RFC 1952) The ZIP engine is presented as a PCI device. It supports DMA and scatter-gather. Signed-off-by: Mahipal Challa Signed-off-by: Vishnu Nair Signed-off-by: Jan Glauber --- drivers/crypto/Kconfig |7 + drivers/crypto/Makefile|1 + drivers/crypto/cavium/Makefile |4 + drivers/crypto/cavium/zip/Makefile |8 + drivers/crypto/cavium/zip/common.h | 258 +++ drivers/crypto/cavium/zip/zip_crypto.h | 61 ++ drivers/crypto/cavium/zip/zip_device.c | 208 + drivers/crypto/cavium/zip/zip_device.h | 138 drivers/crypto/cavium/zip/zip_main.c | 500 drivers/crypto/cavium/zip/zip_main.h | 126 +++ drivers/crypto/cavium/zip/zip_mem.c| 120 +++ drivers/crypto/cavium/zip/zip_mem.h| 78 ++ drivers/crypto/cavium/zip/zip_regs.h | 1326 13 files changed, 2835 insertions(+) create mode 100644 drivers/crypto/cavium/Makefile create mode 100644 drivers/crypto/cavium/zip/Makefile create mode 100644 drivers/crypto/cavium/zip/common.h create mode 100644 drivers/crypto/cavium/zip/zip_crypto.h create mode 100644 drivers/crypto/cavium/zip/zip_device.c create mode 100644 drivers/crypto/cavium/zip/zip_device.h create mode 100644 drivers/crypto/cavium/zip/zip_main.c create mode 100644 drivers/crypto/cavium/zip/zip_main.h create mode 100644 drivers/crypto/cavium/zip/zip_mem.c create mode 100644 drivers/crypto/cavium/zip/zip_mem.h create mode 100644 drivers/crypto/cavium/zip/zip_regs.h diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig index 4d2b81f..da48d93 100644 --- a/drivers/crypto/Kconfig +++ b/drivers/crypto/Kconfig @@ -485,6 +485,13 @@ config CRYPTO_DEV_MXS_DCP source "drivers/crypto/qat/Kconfig" +config CRYPTO_DEV_CAVIUM_ZIP + tristate "Cavium ZIP driver" + depends on PCI && 64BIT && (ARM64 || COMPILE_TEST) + ---help--- + Select this option if you want to enable compression/decompression + acceleration on Cavium's ARM based SoCs + config CRYPTO_DEV_QCE tristate "Qualcomm crypto engine accelerator" depends on (ARCH_QCOM || COMPILE_TEST) && HAS_DMA && HAS_IOMEM diff --git a/drivers/crypto/Makefile b/drivers/crypto/Makefile index ad7250f..3d152d4 100644 --- a/drivers/crypto/Makefile +++ b/drivers/crypto/Makefile @@ -27,6 +27,7 @@ obj-$(CONFIG_CRYPTO_DEV_MXC_SCC) += mxc-scc.o obj-$(CONFIG_CRYPTO_DEV_TALITOS) += talitos.o obj-$(CONFIG_CRYPTO_DEV_UX500) += ux500/ obj-$(CONFIG_CRYPTO_DEV_QAT) += qat/ +obj-$(CONFIG_CRYPTO_DEV_CAVIUM_ZIP) += cavium/ obj-$(CONFIG_CRYPTO_DEV_QCE) += qce/ obj-$(CONFIG_CRYPTO_DEV_VMX) += vmx/ obj-$(CONFIG_CRYPTO_DEV_SUN4I_SS) += sunxi-ss/ diff --git a/drivers/crypto/cavium/Makefile b/drivers/crypto/cavium/Makefile new file mode 100644 index 000..641268b --- /dev/null +++ b/drivers/crypto/cavium/Makefile @@ -0,0 +1,4 @@ +# +# Makefile for Cavium crypto device drivers +# +obj-$(CONFIG_CRYPTO_DEV_CAVIUM_ZIP) += zip/ diff --git a/drivers/crypto/cavium/zip/Makefile b/drivers/crypto/cavium/zip/Makefile new file mode 100644 index 000..2c07508 --- /dev/null +++ b/drivers/crypto/cavium/zip/Makefile @@ -0,0 +1,8 @@ +# +# Makefile for Cavium's ZIP Driver. +# + +obj-$(CONFIG_CRYPTO_DEV_CAVIUM_ZIP) += thunderx_zip.o +thunderx_zip-y := zip_main.o\ + zip_device.o \ + zip_mem.o diff --git a/drivers/crypto/cavium/zip/common.h b/drivers/crypto/cavium/zip/common.h new file mode 100644 index 000..f0694f4 --- /dev/null +++ b/drivers/crypto/cavium/zip/common.h @@ -0,0 +1,258 @@ +/***license start + * Copyright (c) 2003-2016 Cavium, Inc. + * All rights reserved. + * + * License: one of 'Cavium License' or 'GNU General Public License Version 2' + * + * This file is provided under the terms of the Cavium License (see below) + * or under the terms of GNU General Public License, Version 2, as + * published by the Free Software Foundation. When using or redistributing + * this file, you may do so under either license. + * + * Cavium License: Redistribution and use in source and binary forms, with + * or without modification, are permitted provided that the following + * conditions are met: + * + * * Redistributions of source code must retain the above copyright + *notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above + *copyright notice, this list of conditions and the following + *disclaimer in the documentation and/or other
